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

Lack of token approval from `CapitalPool` to `TokenManager` in `TokenManager::withdraw` leads to DoS of all withdrawals

irondevx

Summary

TokenManager::withdraw uses _safe_transfer_from instead of _transfer, which doesn't handle the necessary approvals. This causes all withdrawal attempts from the CapitalPool to fail, effectively locking user funds in the protocol.

TokenManager.sol#L175-L180

// src/core/TokenManager.sol
function withdraw(address _tokenAddress, TokenBalanceType _tokenBalanceType) external whenNotPaused {
// existing code...
​
if (_tokenAddress == wrappedNativeToken) {
// existing code...
} else {
@> _safe_transfer_from(_tokenAddress, capitalPoolAddr, _msgSender(), claimAbleAmount);
}
}

Rescuable.sol#L104-L117

// src/utils/Rescuable.sol
function _safe_transfer_from(address token, address from, address to, uint256 amount) internal {
@> (bool success,) = token.call(abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount));
​
if (!success) {
revert TransferFailed();
}
}

Impact

This bug renders the withdrawal functionality completely inoperable. Users are unable to retrieve their funds from the protocol, leading to a loss of trust and potential financial losses if users need immediate access to their assets.

Tools Used

Foundry, Manual code review

Proof of Concept

Place the following test in PreMarkets.t.sol:

// import {Rescuable} from "../src/utils/Rescuable.sol";
function test_withdraw_insufficientAllowance() public {
// ~~~~~~~~~~~~~~~~~~~~ Setup ~~~~~~~~~~~~~~~~~~~~
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 INITIAL_BALANCE = 1000 * 1e18;
uint256 MAKER_POINTS = 10_000;
uint256 TAKER_POINTS = 5_000;
uint256 TOKEN_AMOUNT = 100 * 1e18;
uint256 COLLATERAL_RATE = 10_000 * 1.2; // 120% collateral rate
uint256 EACH_TRADE_TAX = 10_000 * 0.03; // 3% trade tax
​
// Provide Alice and Bob with initial token balances
deal(address(mockUSDCToken), alice, INITIAL_BALANCE);
deal(address(mockUSDCToken), bob, INITIAL_BALANCE);
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
​
// ~~~~~~~~~~ 1) Create a maker offer ~~~~~~~~~~
vm.startPrank(alice);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createMaker(
CreateMakerParams(
marketPlace,
address(mockUSDCToken),
MAKER_POINTS,
TOKEN_AMOUNT,
COLLATERAL_RATE,
EACH_TRADE_TAX,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address offerAddr = GenerateAddress.generateOfferAddress(0);
vm.stopPrank();
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
​
// ~~~~~~~~~~ 2) Create a taker order ~~~~~~~~~~
vm.startPrank(bob);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, TAKER_POINTS);
vm.stopPrank();
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
​
// ~~~~~~~~~~ 3) Attempt withdrawal (should fail due to insufficient allowance) ~~~~~~~~~~
vm.startPrank(alice);
uint256 aliceBalanceBeforeWithdrawal = mockUSDCToken.balanceOf(alice);
uint256 aliceWithdrawableBalance =
tokenManager.userTokenBalanceMap(alice, address(mockUSDCToken), TokenBalanceType.SalesRevenue);
​
console.log("> Alice's actual balance BEFORE withdrawal attempt: %18e", aliceBalanceBeforeWithdrawal);
console.log("> Alice's withdrawable balance: %18e\n", aliceWithdrawableBalance);
​
vm.expectRevert(Rescuable.TransferFailed.selector);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
uint256 aliceBalanceAfterWithdrawal = mockUSDCToken.balanceOf(alice);
console.log(">> Alice's actual balance AFTER withdrawal attempt: %18e\n", aliceBalanceAfterWithdrawal);
​
assertEq(
aliceBalanceBeforeWithdrawal,
aliceBalanceAfterWithdrawal,
"Alice's balance should not change due to failed withdrawal"
);
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
​
// ~~~~~~~~~~ 4) Check capital pool allowance ~~~~~~~~~~
uint256 capitalPoolAllowance = mockUSDCToken.allowance(address(capitalPool), address(tokenManager));
console.log(">>> Capital Pool allowance for TokenManager: %18e", capitalPoolAllowance);
assertEq(capitalPoolAllowance, 0, "Capital Pool should not have given allowance to TokenManager");
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
}

Recommendations

Replace _safe_transfer_from with _transfer in the TokenManager::withdraw function, so that necessary token approvals are properly handled:

function withdraw(address _tokenAddress, TokenBalanceType _tokenBalanceType) external whenNotPaused {
// existing code...
​
if (_tokenAddress == wrappedNativeToken) {
// existing code...
} else {
- _safe_transfer_from(_tokenAddress, capitalPoolAddr, _msgSender(), claimAbleAmount);
+ _transfer(_tokenAddress, capitalPoolAddr, _msgSender(), claimAbleAmount, capitalPoolAddr);
}
}

NOTE: This still won't fully fix the allowance issue, due to the next vulnerability: "Incorrect approval in TokenManager::_transfer leads to DoS of all withdrawals", which is covered in a different submission.

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!