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

TokenManager:withdraw() will revert if capitalPool hasn't given allowance before the call

0xlrivo

Summary

TokenManager:withdraw() will revert if capitalPool hasn't given allowance before the call.

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L247

Vulnerability Details

In TokenManager:withdraw(), if the withdraw token is the wrappedNative (WETH), the _transfer() function is called:

if (_tokenAddress == wrappedNativeToken) {
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
}

If the transfer comes from the capitalPool and the allowance hasn't been previously garanted the CapitalPool:approve() function is called.

The issue is that, in TokenManager:_transfer(), the call parameter is incorrectly set to address(this) which is the TokenManagerProxy in this case.

if (_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
ICapitalPool(_capitalPoolAddr).approve(address(this));
}

As we can see, the CapitalPool:approve() function accepts a ERC20 token as a parameter and then infitite-approves TokenManager for that token:

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

But, in this case, it is actually trying to call approve() on the proxy which doesn't have such function so the call simply reverts as we can see in the following foundry output:

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

Impact

If the allowance CapitalPool -> TokenManager isn't previously garanted the withdrawal will fail.

Fortunately, since CapitalPool:approve(), is publicly exposed users can make the approve call themselves and fix the issue.

PoC

Add the following test inside PreMarkets.t.sol:

import {ICapitalPool} from "../src/interfaces/ICapitalPool.sol";
​
function test_tokenManager_fails_without_prior_approval() public {
vm.startPrank(user);
​
preMarktes.createOffer{value: 0.01 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Bid,
OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker{value: 0.006175 * 1e18}(offerAddr, 500);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
vm.stopPrank();
​
vm.prank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1,
3600
);
​
vm.startPrank(user);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
​
deliveryPlace.settleAskTaker(stock1Addr, 500);
​
vm.expectRevert(ICapitalPool.ApproveFailed.selector);
tokenManager.withdraw(address(weth9), TokenBalanceType.RemainingCash);
​
vm.stopPrank();
}

Tools Used

  • Manual review

  • Foundry

Recommendations

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 12 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.