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

TokenManager::withdraw() will revert every time for non-wrappedNativeTokens due to insufficient allowance

itsabinashb

Summary

TokenManager::withdraw() will revert every time if _tokenAddress is not wrappedNativeToken.

Vulnerability Details

TokenManager::withdraw() will never work for ERC20 tokens like USDC i.e which is not wrappedNativeToken. Because, when _tokenAddress is not wrappedNativeToken then Rescuable::_safe_transfer_from() is used to send asset from capitalPool, behind the scene the _safe_transfer_from() uses transferFrom() to transfer assets which needs approval. But the CapitalPool contract is not approving the TokenManager contract to transfer funds from itself, as a result all call to withdraw() for ERC20 tokens will revert.

POC

Run this test in PreMarkets.t.sol:

function test_withdraw() public {
deal(address(mockUSDCToken), address(preMarktes), 1000 * 1e18);
assertEq(mockUSDCToken.balanceOf(address(preMarktes)), 1000 * 1e18);
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stockAddr = GenerateAddress.generateStockAddress(0);
preMarktes.closeOffer(stockAddr,offerAddr);
uint userRefundAmount = tokenManager.userTokenBalanceMap(address(user),address(mockUSDCToken), TokenBalanceType.MakerRefund);
console2.log("refund amount: ", userRefundAmount);
uint balanceBeforeWithdraw = mockUSDCToken.balanceOf(user);
console2.log("USDC balance before withdraw: ", balanceBeforeWithdraw);
​
//@audit the call to withdraw() will revert
vm.expectRevert();
tokenManager.withdraw(address(mockUSDCToken),TokenBalanceType.MakerRefund);

Logs:

├─ [8862] UpgradeableProxy::withdraw(MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 4)
│ ├─ [8343] TokenManager::withdraw(MockERC20Token: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 4) [delegatecall]
│ │ ├─ [534] TadleFactory::relatedContracts(4) [staticcall]
│ │ │ └─ ← [Return] UpgradeableProxy: [0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0]
│ │ ├─ [2963] MockERC20Token::transferFrom(UpgradeableProxy: [0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0], 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 12000000000000000 [1.2e16])
│ │ │ └─ ← [Revert] ERC20InsufficientAllowance(0x6891e60906DEBeA401F670D74d01D117a3bEAD39, 0, 12000000000000000 [1.2e16])
│ │ └─ ← [Revert] TransferFailed()
│ └─ ← [Revert] TransferFailed()

You can see that the transfer was failed due to insufficient allowance.

Tool Used

Manual review

Recommendation

Call the CapitalPool::approve() by passing the _tokenAddress before calling _safe_transfer_from().

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
​
if (claimAbleAmount == 0) {
return;
}
​
​
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
​
​
if (_tokenAddress == wrappedNativeToken) {
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
​
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
} else {
+ ICapitalPool(capitalPoolAddr).approve(_tokenAddress);
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
​
​
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);
}

Run the mentioned test in POC after adding the approval line, the tokens will be successfully sent.

Related Links

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

Updates

Lead Judging Commences

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