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

The withdraw function cannot operate if the token is not the native token due to a lack of allowance.

0xphantom

Summary

When a user has a claimable balance, he call the withdraw function (L-137) in the tokenManager, which transfers the tokens from the capital pool to the msg.sender and calls the approve function in the capital pool, allowing the capital pool to approve the token manager to transfer the tokens from the capital pool to the sender. However, this call will consistently revert if the token is not the native token because the token manager don’t approve the token before doing the call.

Vulnerability Details

If the token to be withdrawn is not the native token, the _safe_transfer_from function (L-95 in the Rescuable contract) will be called internally int the token Manager. However, this call will revert because the tokenManager does not call the approve function of the capital pool, and this block of code will be executed:

else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
​
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);
​

The approve function of the capital pool will not be called, causing the call to revert.

You can copy paste this test in the PreMarkets.t.sol you just have to import the Rescuable contract to have the selector of the error:

function test_withdrawPOC2() public {
vm.startPrank(user);
preMarktes.createOffer{value: 22214494796106255}(
CreateOfferParams(marketPlace, address(mockUSDCToken), 1, 1, 10000, 0, OfferType(0), OfferSettleType(0))
);
preMarktes.createTaker{value: 3}(0xE619a2899a8db14983538159ccE0d238074a235d, 1);
vm.expectRevert(Rescuable.TransferFailed.selector);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType(2));
vm.stopPrank();
}

Impact

The users will not be able to withdraw their tokens.

Tools Used

Manual Review

Recommendations

Add a check to ensure that there is enought allowance to perform the transferFrom like this:

if (IERC20(_tokenAddress).allowance(capitalPoolAddr, address(this)) < claimAbleAmount) {
ICapitalPool(capitalPoolAddr).approve(_tokenAddress);
}
_safe_transfer_from(_tokenAddress, capitalPoolAddr, _msgSender(), claimAbleAmount);
Updates

Lead Judging Commences

0xnevi Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-safeTransferFrom-withdraw-missing-approve

This issue's severity has similar reasonings to #252, whereby If we consider the correct permissioned implementation for the `approve()` function within `CapitalPool.sol`, this would be a critical severity issue, because the withdrawal of funds will be permanently blocked and must be rescued by the admin via the `Rescuable.sol` contract, given it will always revert [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/CapitalPool.sol#L36-L38) when attempting to call a non-existent function selector `approve` within the TokenManager contract. Similarly, the argument here is the approval function `approve` was made permisionless, so if somebody beforehand calls approval for the TokenManager for the required token, the transfer will infact not revert when a withdrawal is invoked. I will leave open for escalation discussions, but based on my first point, I believe high severity is appropriate. It also has a slightly different root cause and fix whereby an explicit approval needs to be provided before a call to `_safe_transfer_from()`, if not, the alternative `_transfer()` function should be used to provide an approval, assuming a fix was implemented for issue #252

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.