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

Insufficient Allowance Prevents User Withdrawals

schereo

Summary

The TokenManager contract fails to properly manage allowances when withdrawing tokens from the CapitalPool, leading to locked user funds.

Vulnerability Details

The vulnerability exists in the TokenManager::withdraw function. When a user attempts to withdraw their tokens, the function tries to transfer tokens directly from the CapitalPool to the user without ensuring proper allowances are set.

Specifically:

  1. In the withdraw function (line 137-189), the contract attempts to transfer tokens using _safe_transfer_from:

_safe_transfer_from(_tokenAddress, capitalPoolAddr, _msgSender(), claimAbleAmount);

  1. This call fails because the TokenManager does not have sufficient allowance to transfer tokens on behalf of the CapitalPool.

  2. The contract does have a mechanism to check and set allowances in the _transfer function (lines 233-262), but this is not utilized in the withdraw function.

Proof of code

Insert the following code snippet into PreMarkets.t.sol. It will revert the transaction due to insufficient allowance:

function testInsufficientAllowanceWhenWithdrawing() public {
address testSeller = makeAddr("testSeller");
address testBuyer = makeAddr("testBuyer");
deal(address(mockPointToken), testSeller, 10 ether);
deal(address(mockUSDCToken), testSeller, 12e15);
deal(address(mockUSDCToken), testBuyer, 1 ether);
​
vm.startPrank(testSeller);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
mockPointToken.approve(address(tokenManager), type(uint256).max);
​
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask, // Sell points
OfferSettleType.Turbo
)
);
vm.stopPrank();
​
vm.startPrank(testBuyer);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 1000);
vm.stopPrank();
​
vm.prank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1, // This sets the TGE to the past so MarketPlaceStatus will be AskSettling and no new offers can be created
3600
);
​
vm.startPrank(testSeller);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
mockPointToken.approve(address(tokenManager), type(uint256).max);
deliveryPlace.settleAskMaker(offerAddr, 1000);
// The testSeller wants to withdraw the revenue from selling 1000 points to testBuyer
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
vm.stopPrank();
}

Impact

Users are unable to withdraw their rightful tokens, effectively locking their funds in the contract.

Tools Used

  • Manual code review

  • Forge unit tests

Recommendations

  1. Modify the withdraw function to approve the TokenManager to transfer tokens on the behalf of the CapitalPool before transferring tokens to the user.

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
// ... (existing code)
if (_tokenAddress == wrappedNativeToken) {
// ... (existing native token handling)
} else {
+ ICapitalPool(capitalPoolAddr).approve(_tokenAddress);
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
​
// ... (remaining code)
}
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.