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

The call to withdraw function will fail because a wrong call to CapitalPool approve will revert and Lock all ETH withdraws for all users.

mikebello

Summary

All ETH withdraws from the TokenManager will fail because the TokenManager will try to call the approve function in the CapitalPool contract sending the TokenManager address which would cause a revert in the transaction as the TokenManager contract doesn't have any approve function, this would block all ETH withdraws from the users.

Vulnerability Details

When a user wants to withdraw his tokens (ETH, USDC, or any ERC20 accepted) from the tadle protocol, they have to call the withdraw function in the TokenManager contract, so the token manager contract can send the tokens to the user.

If a user tries to withdraw ETH from the TokenManager contract they have to send the address of the wrapped ETH, and then the withdraw function will try to transfer the WETH from the Capital Pool to the Token Manager, so later it can convert the WETH to ETH and send it to the user, but this flow has a problem in the internal _transfer function that will cause a revert and block all the ETH withdrawals from any user.

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L137-L189

let's see the problem:

1.- First, the User tries to withdraw his ETH from the protocol and calls the withdraw function sending the WETH address as the token address.

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) {
/**
* @dev token is native token
* @dev transfer from capital pool to msg sender
* @dev withdraw native token to token manager contract
* @dev transfer native token to msg sender
*/
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
​
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
​
​
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);
}

2.- Then the withdraws function calls the internal _transfer function sending this data.

_transfer(wrappedNativeToken, capitalPoolAddr, address(this), claimAbleAmount, capitalPoolAddr);

3.- if the TokenManager contract doesn't have an allowance, it will try to call the approve function in the CapitalPool contract, but it will send the address(this) instead of the WETH address to approve the TokenManager to transfer from the CapitalPool.

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L233-L262

if (_from == _capitalPoolAddr && IERC20(_token).allowance(_from, address(this)) == 0x0) {
/*console2.log("balance capital pool:", fromBalanceBef);
console2.log("balance token manager:", toBalanceBef);
console2.log("Address of token manager:", address(this));*/
//@audit wrong approve data
ICapitalPool(_capitalPoolAddr).approve(address(this));
}
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();
}
}

this mistake will cause a revert as the TokenManager doesn't have any approve function to be called, so the Tx will fail and block the ETH withdrawal from any user.

To show the bug I modified the test_ask_offer_turbo_eth test in the PreMarkets.t.sol contract adding a call to the TokenManager - withdraw function and add some console logs to the contracts to show the status of the contracts during the revert of the withdraw function call.

function test_ask_offer_turbo_eth() public {
vm.startPrank(user);
​
preMarktes.createOffer{value: 0.012 * 1e18}(
CreateOfferParams(
marketPlace, address(weth9), 1000, 0.01 * 1e18, 12000, 300, OfferType.Ask, OfferSettleType.Turbo
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker{value: 0.005175 * 1e18}(offerAddr, 500);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer(stock1Addr, 0.006 * 1e18, 12000);
​
address offer1Addr = GenerateAddress.generateOfferAddress(1);
preMarktes.closeOffer(stock1Addr, offer1Addr);
preMarktes.relistOffer(stock1Addr, offer1Addr);
​
vm.stopPrank();
​
vm.prank(user1);
systemConfig.updateMarket("Backpack", address(mockPointToken), 0.01 * 1e18, block.timestamp - 1, 3600);
​
vm.startPrank(user);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
deliveryPlace.settleAskMaker(offerAddr, 500);
deliveryPlace.closeBidTaker(stock1Addr);
​
//*** adding to test the capitalPool fails to call approve in the token manager.
TokenBalanceType makerRefund = TokenBalanceType.PointToken;
vm.expectRevert(ICapitalPool.ApproveFailed.selector);
tokenManager.withdraw(address(weth9), makerRefund);
​
vm.stopPrank();
}

and this is the result of the function showing the call to withdraw revert with the ApproveFailed error blocking the ETH of the users in the platform.

​
% forge test --mt test_ask_offer_turbo_eth -vvv
[⠊] Compiling...
[⠊] Compiling 2 files with Solc 0.8.25
[⠒] Solc 0.8.25 finished in 2.06s
Compiler run successful!
​
Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] test_ask_offer_turbo_eth() (gas: 1399236)
​
​
Logs:
​
claimable amount: 5000000000000000000
balance capital pool: 17175000000000000
balance token manager: 0
Address of token manager: 0x6891e60906DEBeA401F670D74d01D117a3bEAD39
calling approve on address: 0x6891e60906DEBeA401F670D74d01D117a3bEAD39
approving call result: false
​
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 16.00ms (3.90ms CPU time)

Impact

Users can't withdraw ETH from the platform, because the Withdraw function of the TokenManager will revert and lock the user's ETH in the contract.

Tools Used

Manual Code Review

Recommendations

correct the call to the CapitalPool Approve function sending the correct address of the token to approve instead of the address of the TokenManager

Updates

Lead Judging Commences

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