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

`_safe_transfer_from()` is used in tokenManager:withdraw() instead of `_transfer()`, breaking the withdraw() functionality

0xaraj

Summary

_safe_transfer_from() is used in tokenManager:withdraw() instead of _transfer(), breaking the withdraw() functionality

Vulnerability Details

A user can withdraw his tokens from capitalPool using tokenManager:withdraw(). If tokenAddress is not wrappedNativeToken ie tokenAddress != wrappedNativeToken then it goes to else statement, where it calls _safe_transfer_from()

function withdraw(address _tokenAddress, TokenBalanceType _tokenBalanceType) external whenNotPaused {
...
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);
}
}

Now this is a problem because _safe_transfer_from() is directly transferring token from capitalPool to _msg.sender() but there is no approval from capitalPool to tokenManager. As result, if token is not wrappedNativeToken then withdraw() will revert due to insufficient allowance issue

//Here is PoC

function test_withdrawIsBroken() public {
//User created an offer
vm.startPrank(user);
preMarktes.createOffer(
CreateOfferParams(
marketPlace, address(mockUSDCToken), 1000, 1e18, 10000, 300, OfferType.Ask, OfferSettleType.Turbo
)
);
vm.stopPrank();
​
//User2 bought all 1000 points
vm.startPrank(user2);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker(offerAddr, 1000);
vm.stopPrank();
​
vm.prank(user1);
systemConfig.updateMarket("Backpack", address(mockPointToken), 0.01 * 1e18, block.timestamp - 1, 3600);
​
//User should be able to withdraw his tokens that are generate as tradeTax & salesRevenue
vm.startPrank(user);
vm.expectRevert();
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.TaxIncome);
vm.expectRevert();
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.SalesRevenue);
vm.stopPrank();
}

Impact

Users will not be able to withdraw tokens, if token is not wrappedNativeToken

Tools Used

Manual Review

Recommendations

Use _transfer() instead of _safe_transfer_from(), just like we used in if statement ie when token = wrappedNativeToken

- _safe_transfer_from(_tokenAddress, capitalPoolAddr, _msgSender(), claimAbleAmount);
+ _transfer(_tokenAddress, capitalPoolAddr, _msgSender(), claimAbleAmount, capitalPoolAddr);
Updates

Lead Judging Commences

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