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

All users cannot withdraw ERC20 tokens(except WETH)

dinkras

Summary

All participants who deposited ERC20 tokens inside the protocol are expected to be able to withdraw them at some point in time. However, the transfer functionality is broken and that withdraw is not possible(for all tokens except WETH).

Vulnerability Details

Users call TokenManager.withdraw()to withdraw their deposited assets from the protocol. There are 2 different types of assets according to the protocol logic: WETH and all other standard ERC20 tokens. For all standard token(except WETH) , this logic is executed inside withdraw()

} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(//@audit-issue function always reverts because TokenManager does not have allowance for capital pool. Implement the same mechanisms inside safeTransfer as in TokenManager._transfer() which calls approve.
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}

_safe_transfer_from() is being called. Looking at the specific imp, it can be seen that there is no increase of allowance logic from the CapitalPool to TokenManager. Such mechanism exists for WETH, but not for all other ERC20 tokens.

function _safe_transfer_from(
address token,
address from,
address to,
uint256 amount
) internal {
(bool success, ) = token.call(//@audit does not handle false return value. check OZ SafeERC and see what they are not doing here. This won't work with tokens returning false. Use OZ lib
abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}

As a result TokenManager.withdraw() always reverts when called with any ERC20 token different than WETH. This is because CapitalPool never approved TokenManager to transfer it's ERC20 tokens(except WETH)

Coded POC

1) Add the test inside PreMarkets.t.sol

2) Run: forge test --mt test_offerMakers_cannot_withdraw_POC -vvvvv

function test_offerMakers_cannot_withdraw_POC() external {
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken), // USDC as collateral
1000, // sells 1000 points
0.01 * 1e18,
12000, // collateral is 120%
300, // taxes for subsequent trades
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
​
vm.startPrank(user2);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
preMarktes.createTaker(offerAddr, 500);
vm.stopPrank();
​
// the offer maker aborts the offer -> he can withdraw now the funds deposited as collateral
vm.startPrank(user);
preMarktes.abortAskOffer(stockAddr, offerAddr);
​
​
// the user is not able to withdraw his assets, due to missing allowance( of TokenManager to operate with Capital pool funds)
vm.expectRevert(); // reverts with ERC20InsufficientAllowance, this is because TokenManager does not have an allowance to operate with CapitalPool tokens
tokenManager.withdraw(
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
}

Code Snippets

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/TokenManager.sol#L175-L180

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/utils/Rescuable.sol#L104-L116

Impact

Stuck ERC20 tokens in the protocol, permanent DOS of the withdraw functionality

Tools Used

Manual review, foundry

Recommendations

Implement a similiar mechanism as in TokenManager._transfer() which calls approve. Keep in mind that some ERC20 token allowances(like USDC) when set to uint256.max decrease over time, so it's possible to increase the allowance in the future again for that particular token (when _amount to transfer < allowance)

Updates

Lead Judging Commences

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