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

Token withdrawal fails until someone manually approves spending

izcoser

Summary

The protocol uses a contract called TokenManager to control a capital pool that stores tokens.

When a user wants to withdraw, the TokenManager needs spender allowance on the capital pool, but this is not checked for, so the withdrawal fails.

Vulnerability Details

We simulate a user creating an offer, closing it and then trying to withdraw. The withdrawal fails because of zero allowance for the TokenManager as a spender of the capital pool.

function test_token_withdrawal_fails() public {
// Data for creating an offer, not relevant.
uint256 points = 1000;
uint256 amountToken = 1000000 * 1e18;
uint256 collateralRate = 12000;
uint256 eachTradeTax = 300;
​
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
points,
amountToken,
collateralRate,
eachTradeTax,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
// Close the offer.
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stockAddr = GenerateAddress.generateStockAddress(0);
​
preMarktes.closeOffer(stockAddr, offerAddr);
​
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
vm.stopPrank();
}
├─ [8858] UpgradeableProxy::withdraw(MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 4)
│ ├─ [8339] TokenManager::withdraw(MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 4) [delegatecall]
│ │ ├─ [534] TadleFactory::relatedContracts(4) [staticcall]
│ │ │ └─ ← [Return] UpgradeableProxy: [0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0]
│ │ ├─ [2959] MockERC20Token::transferFrom(UpgradeableProxy: [0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0], 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 1200000000000000000000000 [1.2e24])
│ │ │ └─ ← [Revert] ERC20InsufficientAllowance(0x6891e60906DEBeA401F670D74d01D117a3bEAD39, 0, 1200000000000000000000000 [1.2e24])
│ │ └─ ← [Revert] TransferFailed()
│ └─ ← [Revert] TransferFailed()
└─ ← [Revert] TransferFailed()

Impact

For every new token whitelisted into the protocol, users will be unable to withdraw them until somebody calls ``capitalPool.approve(address(token))```. Because there's a disruption of protocol functionality, I am reporting this as MEDIUM.

When someone calls the approve, TokenManager gets an allowance of uint.MAX on one of the capital pool's tokens, which means it withdrawals should work for the foreseeable future.

Tools Used

Forge.

Recommendations

In TokenManager::withdraw, line 175, before the safe transfer from call, check and add allowance if necessary:

if (IERC20(_tokenAddress).allowance(capitalPoolAddr, address(this)) == 0x0) {
ICapitalPool(capitalPoolAddr).approve(_tokenAddress);
}

By doing that, the withdrawal no longer fails:

[PASS] test_token_withdrawal_fails() (gas: 612289)
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.