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

User Funds Locked Due to Incorrect Token Approval in Withdrawal Process

tinnohofficial

Summary

Within the internal _transfer() function, in TokenManager.sol the capital pool is approved the wrong token address as a result TokenManager::withdraw() always reverts when user claims WETH.

Vulnerability Details

In TokenManger::_transfer, The critical issue here is that in the line ICapitalPool(_capitalPoolAddr).approve(address(this)), address(this) refers to the TokenManager contract address and not the token address _token. As a result any withdraw that fulfills the condition if (_tokenAddress == wrappedNativeToken) always reverts.

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
) {
// @audit address(this) is not a token address but rather the address of the TokenManager contract
ICapitalPool(_capitalPoolAddr).approve(address(this));
}
​
// ...
}

To further prove this issue the ICapitalPool::approve or CapitalPool::approve function can be reviewed and it indeed expects a tokenAddr and not a contract address

/**
* @dev Approve token for token manager
* @notice only can be called by token manager
* @param tokenAddr address of token
*/
function approve(address tokenAddr) external; // @audit-info see it's tokenAddr not contract address
​
/// @dev Error when approve failed
error ApproveFailed(); // @audit-info PoC below will revert with this error

PoC

Add this function to the pre existing PreMarketsTest test suite and add import {ICapitalPool} from "../src/interfaces/ICapitalPool.sol"; at the top of test file

function test_withdraw() public {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
​
deal(alice, 1.5 ether);
deal(bob, 1.5 ether);
​
vm.prank(alice);
preMarktes.createOffer{value: 1.5 ether}(
CreateOfferParams(
marketPlace,
address(weth9), // tokenAddress
1000, // points
1 ether, // amount
12000, // collateralRate
0, // eachTradeTax
OfferType.Ask,
OfferSettleType.Protected
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
vm.prank(bob); // bob buys 500 points from Alice
preMarktes.createTaker{value: 0.6 ether}(offerAddr, 500);
​
// alice can't withdraw her funds
vm.expectRevert(ICapitalPool.ApproveFailed.selector);
vm.prank(alice);
tokenManager.withdraw(address(weth9), TokenBalanceType.SalesRevenue);
}

Impact

Any user who uses wrappedNativeToken is unable to withdraw meaning that funds are stuck in the contract.

Tools Used

Manual review

Recommendations

Make changes as shown below

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(address(_token));
}
​
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!