Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Incorrect Contract Approval Prevents WETH Withdrawals

pina

Summary

When a user attempts to call TokenManager::withdraw to withdraw the claimAbleAmount WETH, the transaction reverts because the CapitalPool::approve function receives an incorrect address.

Vulnerability Details

The flow for TokenManager::withdraw is as follows:

  1. Check if claimAbleAmount > 0. If true, proceed.

  2. The token address is wrappedNativeToken.

  3. The _transfer function is called.

    The issue arises because the approve function expects to receive a token address but instead receives the address of the TokenManager. The TokenManager does not have any approve(address,uint256) function that matches the call, causing the function to throw a CapitalPool::ApproveFailed() error.

solidity

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)); // <== ISSUE HERE
}

PoC

Add the following test to PreMarket.t.sol:

function test_approve_failed() public {
vm.startPrank(user);
​
preMarktes.createOffer{value: 0.012 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker{value: 0.005175 * 1e18}(offerAddr, 500);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer(stock1Addr, 0.006 * 1e18, 12000);
​
address offer1Addr = GenerateAddress.generateOfferAddress(1);
preMarktes.closeOffer(stock1Addr, offer1Addr);
preMarktes.relistOffer(stock1Addr, offer1Addr);
​
vm.stopPrank();
​
vm.prank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1,
3600
);
​
// Withdraw funds
vm.startPrank(user);
​
uint256 balance = tokenManager.userTokenBalanceMap(address(user),address(weth9),TokenBalanceType.SalesRevenue);
assertGt(balance,0);
vm.expectRevert();
tokenManager.withdraw(address(weth9),TokenBalanceType.SalesRevenue);
vm.stopPrank();
}

Impact

This vulnerability prevents users from withdrawing ETH funds.

Tools Used

  • Manual Code Review

  • Foundry

Recommendations

To resolve this issue, update the _transfer function to pass the correct address:

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));
+ ICapitalPool(_capitalPoolAddr).approve(_token));
}
​
Updates

Lead Judging Commences

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