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

Incorrect Token Address Argument in `TokenManager::_transfer` Causes Native Token Withdrawal Failure

4lifemen

Summary

The TokenManager::_transfer internal function passes an incorrect argument to the ICapitalPool::approve function, causing native token withdrawals to fail. Result in user funds being locked in the contract.

Vulnerability Details

The TokenManager::_transfer function attempts to approve the TokenManager for spending tokens from the CapitalPool. However, it passes the wrong address to the CapitalPool::approve function:

In TokenManager.sol contract:

function _transfer() internal {
...
​
if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
ICapitalPool(_capitalPoolAddr).approve(address(this));
}
​
_safe_transfer_from(_token, _from, _to, _amount);
​
...
}

The CapitalPool::approve expects a token address as an argument:

In CapitalPool.sol contract:

function approve(address tokenAddr) external {
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
​
// approve the token for token manager with max value
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
type(uint256).max
)
);
​
if (!success) {
revert ApproveFailed();
}
}

The vulnerability arises because TokenManager::_transfer passes address(this) (the TokenManager's address) instead of the token address to ICapitalPool::approve.

Proof of code
Add the following function into the PreMarkets.t.sol contract to demonstrate the issue:

function test_with_native_eth() public {
vm.startPrank(user);
​
preMarktes.createOffer{value: 0.02 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
​
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
preMarktes.closeOffer(stockAddr, offerAddr);
​
vm.expectRevert();
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
​
vm.stopPrank();
}

The test execution trace shows the failure.

│ │ ├─ [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()
│ │ │ └─ ← [Revert] ApproveFailed()

Shows that the withdrawal fails due to the incorrect argument passed to the ICapitalPool::approve function.

Impact

This vulnerability prevents users from withdrawing native tokens. When a user attempts to withdraw native tokens, the transaction will revert due to the failed approval process. This effectively locks user funds in the contract, rendering a withdrawal feature of the platform inoperable.

Tools Used

  • Manual review

  • Foundry

Recommendations

Modify the TokenManager::_transfer function to pass the correct token address:

function _transfer() internal {
...
if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
- ICapitalPool(_capitalPoolAddr).approve(address(this));
+ ICapitalPool(_capitalPoolAddr).approve(_token);
}
_safe_transfer_from(_token, _from, _to, _amount);
...
}
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.