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

Native token withdrawal fails until manually approved

izcoser

Summary

TokenManager::withdraw uses a spending allowance on the capital pool to move its funds.

When doing a withdrawal, the internal function TokerManager::_transfer checks for this allowance and attempts to approve spending, but incorrectly passes address(this) instead of the token address.

Hence, withdrawal fails.

Vulnerability Details

Let us use a simple example of creating an offer, closing it and attempting to withdraw.

function test_native_token_withdrawal_fails() public {
deal(user, INITIAL_TOKEN_VALUE);
​
vm.startPrank(user);
preMarktes.createOffer{value: 0.012 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
// Close the offer.
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stockAddr = GenerateAddress.generateStockAddress(0);
​
preMarktes.closeOffer(stockAddr, offerAddr);
​
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
vm.stopPrank();
}

We get:

│ │ ├─ [9999] UpgradeableProxy::approve(UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39])
│ │ │ ├─ [4983] CapitalPool::approve(UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39]) [delegatecall]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(5) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39]
│ │ │ │ ├─ [708] UpgradeableProxy::approve(UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ │ │ │ │ ├─ [192] TokenManager::approve(UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77]) [delegatecall]
│ │ │ │ │ │ └─ ← [Revert] EvmError: Revert
│ │ │ │ │ └─ ← [Revert] EvmError: Revert
│ │ │ │ └─ ← [Revert] ApproveFailed()

This approve fails because we're calling approve on a non-ERC20 contract, the function simply does not exist.

Here is the mistake, in TokenManager::_transfer:

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
) {
ICapitalPool(_capitalPoolAddr).approve(address(this));
}

address(this) is TokenManager itself, but CapitalPool::approve expects a token address.

Impact

No funds are at risk, but people won't be able to withdraw native tokens until someone manually calls the approve correctly. Because there's a disruption of protocol functionality, I'm submitting this as MEDIUM.

Tools Used

Forge.

Recommendations

Change ICapitalPool(_capitalPoolAddr).approve(address(this)) to ICapitalPool(_capitalPoolAddr).approve(_token) in line 247, TokenManager.sol.

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-approve-wrong-address-input

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. The argument up in the air is since the approval function `approve` was made permisionless, the `if` block within the internal `_transfer()` function will never be invoked if somebody beforehand calls approval for the TokenManager for the required token, so 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.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.