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

User can not withdraw tokens from `TokenManager` because of insufficient allowance

baz1ka

Relevant GitHub Links

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L170-L181

Vulnerability Details

When users want to withdraw non-WETH tokens they would get TransferFailed error. All tokens deposited into the protocol are stored in CapitalPool contract. This contract is responsible for storing and redeeming tokens. When users want to withdraw tokens they call TokenManager.withdraw(). This function has a condition that checks whether the desirable token is WETH or not. If not it executes this block:

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

This code tries to transfer tokens from CapitalPool to the user. But it can not do that as _safe_transfer_from doesn't call CapitalPool.approve() to get enough allowance to withdraw tokens as by default allowance is 0.

Impact

User tokens are stuck in CapitalPool.

Proof of Concept

Modify test/PreMarkets.t.sol test with the next code and execute this test with forge test --match-test test_user_can_not_withdraw_non_weth_token:

function test_user_can_not_withdraw_non_weth_token() public {
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 500);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer(stock1Addr, 0.006 * 1e18, 12000);
​
address offer1Addr = GenerateAddress.generateOfferAddress(1);
preMarktes.closeOffer(stock1Addr, offer1Addr);
​
vm.stopPrank();
​
uint256 userTokenManagerUSDCBalance = tokenManager.userTokenBalanceMap(
user,
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
​
// user has tokens in TokenManager
assert(userTokenManagerUSDCBalance != 0);
​
// user can not withdraw these tokens
bytes4 errorSelector = bytes4(keccak256("TransferFailed()"));
​
vm.prank(user);
vm.expectRevert(errorSelector);
tokenManager.withdraw(
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
}

Recommended Mitigation Steps

Change _safe_transfer_from to _transfer:

* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
- _safe_transfer_from(
+ _transfer(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
- claimAbleAmount
+ claimAbleAmount,
+ capitalPoolAddr
);
}
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.