Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

No User will be able to withdraw wrapped token due to wrong approval

0xaman

Summary

When a user calls the withdraw function to retrieve their assets from the Tadle Protocol, if they attempt to withdraw a WrappedToken, the approval process is executed on an incorrect token address, which will always result in a revert.

Vulnerability Details

All the user balances are stored in userTokenBalanceMap with the tokenAddress and paymentType. The balance for wrapped token will also be stored in same way.

function addTokenBalance(
TokenBalanceType _tokenBalanceType,
address _accountAddress,
address _tokenAddress,
uint256 _amount
) external onlyRelatedContracts(tadleFactory, _msgSender()) {
userTokenBalanceMap[_accountAddress][_tokenAddress][
_tokenBalanceType
] += _amount;
emit AddTokenBalance(
_accountAddress,
_tokenAddress,
_tokenBalanceType,
_amount
);
}

For withdraw of these balances (the focus of this report is on wrapped token) the user will call withdraw function.

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
*/
@1> _transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
​

withdraw function will call transfer function to transfer wrapped token from capital pool to tokenManager contract.

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
) {
@2> ICapitalPool(_capitalPoolAddr).approve(address(this)); // @audit : it will never approve
// ICapitalPool(_capitalPoolAddr).approve(_token);
}
​
_safe_transfer_from(_token, _from, _to, _amount);
​
uint256 fromBalanceAft = IERC20(_token).balanceOf(_from);
uint256 toBalanceAft = IERC20(_token).balanceOf(_to);
​
if (fromBalanceAft != fromBalanceBef - _amount) {
revert TransferFailed();
}
​
if (toBalanceAft != toBalanceBef + _amount) {
revert TransferFailed();
}
}

Now lets have look at approve function of CapitalPool contract :

function approve(address tokenAddr) external {
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
@3> (bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
type(uint256).max
)
);
​
if (!success) {
revert ApproveFailed();
}
}

Now lets wrap up complete flow :

  1. Bob calls withdraw function to withdraw weth token.

  2. The withdraw function calls _transfer function @1>.

  3. The _transfer function calls approve function of CapitalPool @2> and pass the address of address(this) which is tokenManger contract address.

  4. @3> treats tokenManager address as ERC20 token address and calls the approve function. At this step the execution will revert because tokenManager does not have approve function and approval will be failed.

The following coded POC will Illustrate the Bug :

Add following test code to PerMarket.t.sol file :

function testTokenManager_Wrong_Approval() external {
deal(address(tokenManager), 100 ** 18);
vm.startPrank(user);
​
preMarktes.createOffer{value: 0.012 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker{value: 0.005175 * 1e18}(offerAddr, 500);
​
address stock1Addr = GenerateAddress.generateStockAddress(1);
preMarktes.listOffer{value: 0.0072 * 1e18}(
stock1Addr,
0.006 * 1e18,
12000
);
​
address offer1Addr = GenerateAddress.generateOfferAddress(1);
preMarktes.closeOffer(stock1Addr, offer1Addr);
vm.expectRevert(ICapitalPool.ApproveFailed.selector); // ApproveFailed
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
}

Run with command : forge test --mt testTokenManager_Wrong_Approval -vvv

Note : This finding assume that the approval function in CapitalPool will only be called by tokenManager as stated in @dev comments.

Impact

The user will never be able to withdraw the wrapped token. if team fix the issue of CapitalPool::approve only callable by tokenManager contract.

Tools Used

Manual review

Recommendations

add following changes to the _transfer function:

@@ -239,12 +247,13 @@ contract TokenManager is
) 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);
}
​
Updates

Lead Judging Commences

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