Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Users can bypass withdrawal limits through `TokenManager::addTokenBalance()` and `TokenManager::withdraw()`

4gontuk

Summary

Vulnerability Detail

The TokenManager contract contains two key functions: addTokenBalance() and withdraw(). The addTokenBalance() function allows certain privileged contracts to increase a user's token balance in the userTokenBalanceMap, while the withdraw() function allows users to withdraw their recorded balance. However, there's a critical flaw in how these functions interact, potentially allowing users to withdraw more tokens than they should be able to.

The addTokenBalance() function updates the user's balance in the contract's state:

function addTokenBalance(
TokenBalanceType _tokenBalanceType,
address _accountAddress,
address _tokenAddress,
uint256 _amount
) external onlyRelatedContracts(tadleFactory, _msgSender()) {
userTokenBalanceMap[_accountAddress][_tokenAddress][
_tokenBalanceType
] += _amount;
​
emit AddTokenBalance(
_accountAddress,
_tokenAddress,
_tokenBalanceType,
_amount
);
}

The withdraw() function allows users to withdraw their recorded balance:

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

The issue is that the withdraw() function does not update the user's balance after a successful withdrawal. This means that a user can potentially withdraw their entire balance multiple times, as long as the addTokenBalance() function has been called at least once to set a non-zero balance.

Impact

This vulnerability allows malicious users to drain the contract of funds far beyond their actual entitlement. A user could potentially withdraw the same balance multiple times, leading to significant financial losses for the protocol.

Proof of Concept

  1. Alice has a legitimate balance of 100 tokens recorded in userTokenBalanceMap.

  2. Alice calls withdraw() and successfully withdraws 100 tokens.

  3. Alice's balance in userTokenBalanceMap remains unchanged at 100 tokens.

  4. Alice calls withdraw() again and successfully withdraws another 100 tokens.

  5. This process can be repeated until the contract is drained of funds.

Tools Used

Manual review

Recommendation

The withdraw() function should update the user's balance after a successful withdrawal. Here's a suggested fix:

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
if (claimAbleAmount == 0) {
return;
}
​
+ // Reset the user's balance before transfer
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] = 0;
​
// ... transfer logic ...
​
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);
}

This change ensures that a user's balance is set to zero immediately before the transfer is initiated, preventing multiple withdrawals of the same balance. Additionally, consider implementing a reentrancy guard to prevent potential reentrancy attacks during the withdrawal process.

Updates

Lead Judging Commences

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