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

Withdrawal Function Fails for Non-Native Tokens Due to Missing Approval

MinhTriet

Vulnerability Details

The TokenManager::withdraw function fails to check for or handle the required ERC20 approval before attempting to transfer non-native tokens or non wrapped native token, specifically, there's no approval from capital pool to msg sender. This will cause all withdrawal attempts for non-native tokens or non wrapped native token to fail, effectively locking user funds in the contract.

[The affected code](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L170-L181):

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

Impact

  1. Users are unable to withdraw their non-native tokens.

  2. Core contract functionality is broken.

  3. Potential loss of user trust and funds lockup.

PoC

function test_withdraw_erc20_token() public {
// set up code so there's balance for user to withdraw
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
​
vm.startPrank(user1);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
preMarktes.createTaker(offerAddr, 500);
vm.stopPrank();
​
vm.startPrank(user);
preMarktes.abortAskOffer(stockAddr, offerAddr);
​
// Check if balance of user to able to withdraw > 0
uint balanceToWithdraw = tokenManager.userTokenBalanceMap(
address(user),
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
console.log("user's token balance they can withdraw: ", balanceToWithdraw);
assertGt(balanceToWithdraw, 0);
vm.expectRevert();
// Try to withdraw but fail because of ERC20InsufficientAllowance error
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
​
vm.stopPrank();
}

Tools Used

Foundry

Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year 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.

Give us feedback!