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

`msg.sender` can repeatedly call `withdraw` function to claim multiples of `claimAbleAmount` that do not belong to him from capital pool

rbserver

Summary

Because the withdraw function does not reset the corresponding userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] to 0, msg.sender is able to repeatedly call it to claim the same claimAbleAmount each time. This allows such msg.sender to steal multiples of such claimAbleAmount, other than his first claim of such claimAbleAmount, that do not belong to him from the capital pool.

Vulnerability Details

Calling the following withdraw function executes uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] in which such claimAbleAmount is then claimed by msg.sender through either the payable(msg.sender).transfer or _safe_transfer_from function call. However, since such userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] is not reset to 0, such msg.sender can repeatedly call the withdraw function to claim the same claimAbleAmount each time until the capital pool's token balance of the _tokenAddress is depleted.

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

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
@> uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
@> _tokenAddress
@> ][_tokenBalanceType];
​
if (claimAbleAmount == 0) {
return;
}
​
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
​
if (_tokenAddress == wrappedNativeToken) {
/**
* @dev token is native token
* @dev transfer from capital pool to msg sender
* @dev withdraw native token to token manager contract
* @dev transfer native token to msg sender
*/
@> _transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
@> claimAbleAmount,
capitalPoolAddr
);
​
@> IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
@> payable(msg.sender).transfer(claimAbleAmount);
} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
@> _safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
@> claimAbleAmount
);
}
...
}

Impact

After the corresponding msg.sender repeatedly calls the withdraw function to claim the same claimAbleAmount each time, he has stolen multiples of such claimAbleAmount, other than his first claim of such claimAbleAmount, that do not belong to him from the capital pool.

Tools Used

Manual Review

Recommended Mitigation

The withdraw function can be updated to reset the corresponding userTokenBalanceMap[_msgSender()][_tokenAddress][_tokenBalanceType] to 0 before transferring the corresponding claimAbleAmount to msg.sender.

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.