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

Incorrect approval in `TokenManager::_transfer` leads to DoS of all withdrawals

irondevx

Summary

TokenManager::_transfer incorrectly approves the TokenManager's address instead of the token address when setting up allowances for the capital pool. This error causes all withdrawal attempts to fail, effectively locking user funds in the protocol.

TokenManager.sol#L247

// src/core/TokenManager.sol
function _transfer(address _token, address _from, address _to, uint256 _amount, address _capitalPoolAddr) internal {
uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
uint256 toBalanceBef = IERC20(_token).balanceOf(_to);
​
if (_from == _capitalPoolAddr && IERC20(_token).allowance(_from, address(this)) == 0x0) {
@> ICapitalPool(_capitalPoolAddr).approve(address(this));
}
​
// rest of the function...
}

CapitalPool.sol#L28-L34

// src/core/CapitalPool.sol
contract CapitalPool is CapitalPoolStorage, Rescuable, ICapitalPool {
bytes4 private constant APPROVE_SELECTOR = bytes4(keccak256(bytes("approve(address,uint256)")));
​
constructor() Rescuable() {}
​
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();
}
}
}

Impact

This bug renders the withdrawal functionality completely inoperable. Users are unable to retrieve their funds from the protocol.

Tools Used

Foundry, Manual code review

Proof of Concept

Place the following test in PreMarkets.t.sol:

// import {ICapitalPool} from "../src/interfaces/ICapitalPool.sol";
function test_transfer_invalidApprovalForCapitalPool() public {
// ~~~~~~~~~~~~~~~~~~~~ Setup ~~~~~~~~~~~~~~~~~~~~
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 INITIAL_BALANCE = 1000 * 1e18;
uint256 MAKER_POINTS = 1000;
uint256 TAKER_POINTS = 500;
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
​
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);
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
​
// ~~~~~~~~~~ 2) Create a taker order ~~~~~~~~~~
vm.startPrank(bob);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createTaker(offerAddr, TAKER_POINTS);
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
​
// ~~~ 3) Withdrawal attempt - should revert due to bad 'approve' parameter ~~~
vm.startPrank(alice);
vm.expectRevert(ICapitalPool.ApproveFailed.selector);
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
}

Recommended Mitigation Steps

Correct the approval in the _transfer function:

function _transfer(address _token, address _from, address _to, uint256 _amount, address _capitalPoolAddr) internal {
uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
uint256 toBalanceBef = IERC20(_token).balanceOf(_to);
​
if (_from == _capitalPoolAddr && IERC20(_token).allowance(_from, address(this)) == 0x0) {
- ICapitalPool(_capitalPoolAddr).approve(address(this));
+ ICapitalPool(_capitalPoolAddr).approve(_token);
}
​
// rest of the function...
}
Updates

Lead Judging Commences

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

Give us feedback!