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

User cannot withdraw ETH from CapitalPool

h2134

Summary

User cannot withdraw ETH from CapitalPool, due to approve() is not called with correct argument.

Vulnerability Details

User can call withdraw() to receive ETH if their claimable balance of wrappedNativeToken is larger than 0.

if (_tokenAddress == wrappedNativeToken) {
/**
* @dev token is native token
* @dev transfer from capital pool to msg sender
* @dev withdraw native token to token manager contract
* @dev transfer native token to msg sender
*/
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
} else {

_transfer() is called to transfer WETH from CapitalPool to TokenManager, then WETH is unwrapped and ETH is transferred to the user. To transfer WETH from CapitalPool, _transfer() will call approve() in CapitalPool to get token allowance.

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

However, approve() is called with wrong argument address(this), intead of WETH address, CapitalPool will then try to call approve(address,uint256) in TokenManager, and the transaction will be reverted because there is no such function in TokenManager.

/**
* @dev Approve token for token manager
* @notice only can be called by token manager
* @param tokenAddr address of 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();
}
}

Please run the PoC to verify:

function testAudit_UserCannotWithdrawETH() public {
address wrappedNativeToken = tokenManager.wrappedNativeToken();
​
// Create Market
address auditMarketPlace = GenerateAddress.generateMarketPlaceAddress("AuditMarket");
vm.startPrank(user1);
systemConfig.createMarketPlace("AuditMarket", false);
systemConfig.updateMarket("AuditMarket", address(mockPointToken), 1e18, block.timestamp + 30 days, 2 days);
vm.stopPrank();
​
address alice = makeAddr("Alice");
deal(alice, 1000e18);
​
address aliceStockAddr = GenerateAddress.generateStockAddress(preMarktes.offerId());
address aliceOfferAddr = GenerateAddress.generateOfferAddress(preMarktes.offerId());
​
// Alice creates an Ask offer and abort
vm.startPrank(alice);
preMarktes.createOffer{value: 1000e18}(CreateOfferParams({
marketPlace: auditMarketPlace,
tokenAddress: wrappedNativeToken,
points: 1000,
amount: 1000e18,
collateralRate: 10000,
eachTradeTax: 0,
offerType: OfferType.Ask,
offerSettleType: OfferSettleType.Turbo
}));
preMarktes.abortAskOffer(aliceStockAddr, aliceOfferAddr);
vm.stopPrank();
​
// Alice's claimable balance is 1000e18
assertEq(tokenManager.userTokenBalanceMap(alice, wrappedNativeToken, TokenBalanceType.MakerRefund), 1000e18);
​
// Alice withdraws ETH but failed
vm.prank(alice);
vm.expectRevert(abi.encodeWithSelector(ICapitalPool.ApproveFailed.selector));
tokenManager.withdraw(wrappedNativeToken, TokenBalanceType.MakerRefund);
}

Impact

User won't be able to withdraw ETH from CapitalPool.

Based on the current codebase, anyone can call approve() to grant WETH allowance to TokenManager, however, according to the NatSpect, the approve() is only supposed to be called by TokenManager, so no one else is expected to call this function (actually this is another issue).

@notice only can be called by token manager

Tools Used

Manual Review

Recommendations

Pass token address to approve():

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
...
​
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 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.