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

No approval during withdrawal in TokenManager

tinnohofficial

Summary

TokenManager::witdhraw Withdrawal with wrapped native token does the approval but all other tokens are not approved leading to always reverting unless the user explicitly calls CapitalPool::aprove to approve the tokenManager to spend funds in the capitalPool.

Vulnerability Details

When withdrawing a particular ERC20 token, the tokenManager needs to be approved to take funds from the capital pool by calling the ERC20 approve function. Now the capital pool has an ICapitalPool::approve function to do exactly that. The issue is that the tokenManager itself does not call this approve on the capital pool in a consistent manner. See below.

function withdraw(address _tokenAddress, TokenBalanceType _tokenBalanceType) external whenNotPaused {
​
// ...
​
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
*/
// @audit there is an approval happening within this `_transfer` function
_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
*/
// @audit no any approval in this function's implementation.
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}

To prove the point looking at the _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
) {
// @audit there is the approval
ICapitalPool(_capitalPoolAddr).approve(address(this));
}

but the implementation of Rescuable::_safe_transfer_from does not have any approval. so there is inconsistency. https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/utils/Rescuable.sol#L84

PoC

Add this line import {Rescuable} from "../src/utils/Rescuable.sol"; at top of PreMarketsTest as well as the test function below

function test_no_approval() public {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
​
deal(address(mockUSDCToken), alice, 1e18);
deal(address(mockUSDCToken), bob, 1e18);
​
// alice creates offer to sell 1000 points for 0.5 * 1e18usdc
vm.startPrank(alice);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.5 * 1e18,
12000,
0,
OfferType.Ask,
OfferSettleType.Protected
)
);
vm.stopPrank();
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
// bob buys 500 points from alice and alice can withdraw 0.25 * 1e18
vm.startPrank(bob);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, 500);
vm.stopPrank();
​
vm.expectRevert(Rescuable.TransferFailed.selector);
vm.prank(alice);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
}

Impact

Users can't withdraw any of their token claimable amounts unless the approve is done using CapitalPool::approve

Tools Used

Manual review

Recommendations

Be consistent with implementing approvals. You can do it just once in the TokenManager::withdraw instead of doing it internal functions like _transfer as shown in the vulnerability details but this option is still gas expensive since while everyone is withdrawing they have to approve the tokenManager to take money from the capitalPool.

Another better alternative would be to have a separate function in the tokenManager to this approval per token or just do it in the TokenManager::updateTokenWhiteListed while adding tokens to the whitelist so that they are approved

Updates

Lead Judging Commences

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