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

Users unable to withdraw() ERC20 tokens due to missing approve()

wellbyt3

Summary

When a user wants to withdraw()their funds from the protocol, TokenManager facilitates the transfer from CapitalPool to user.

CapitalPool has an approve() function that can be called externally that approves TokenManager to transfer tokens on behalf of CapitalPool.

approve() isn't called when the token isn't the wrappedNativeToken causing users to be unable to withdraw their ERC20s.

Vulnerability Details

The problem is that approve() is only called when _transfer is called in TokenManager:

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));
}
​
...
}

But _transfer is only called in withdraw() when the token is the wrappedNativeToken, not other ERC20s:

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
...
if (_tokenAddress == wrappedNativeToken) {
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
} else {
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
...
​
}

Here's a POC (add to PreMarket.t.sol) showing the failed transfer:

function testMissingApproveTokenManagerFromCapitalPool() public {
// 1. User creates an offer
vm.startPrank(user);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
20000000e18,
10000,
0,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
vm.stopPrank();
​
// 2. User aborts the offer
vm.startPrank(user);
address stockAddr = GenerateAddress.generateStockAddress(0);
preMarktes.abortAskOffer(stockAddr, offerAddr);
vm.stopPrank();
​
// 3. User withdraws mockUSDCToken, but transfer fails due to ERC20InsufficientAllowance
vm.startPrank(user);
bytes4 selector = bytes4(keccak256("TransferFailed()"));
vm.expectRevert(abi.encodeWithSelector(selector));
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
vm.stopPrank();
}

Since approve() is external, the protocol could create another contract after deployment to approve() TokenManager to spend on behalf of CapitalPool, but if this wasn't caught, the protocol could have launched without this approval causing users to be unable to withdraw() their funds when they want.

Impact

Breaks intended protocol functionality.

Tools Used

Manual Review / Foundry

Recommendations

Make sure to approve() TokenManager to spend on behalf of CapitalPool when the token is NOT the wrappedNativeToken.

Updates

Lead Judging Commences

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

finding-TokenManager-safeTransferFrom-withdraw-missing-approve

This issue's severity has similar reasonings to #252, whereby 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. Similarly, the argument here is the approval function `approve` was made permisionless, so if somebody beforehand calls approval for the TokenManager for the required token, 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. It also has a slightly different root cause and fix whereby an explicit approval needs to be provided before a call to `_safe_transfer_from()`, if not, the alternative `_transfer()` function should be used to provide an approval, assuming a fix was implemented for issue #252

Support

FAQs

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

Give us feedback!