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

[HIGH-01] If `TokenManager::owner()` sets a weirdERC20 that takes an Fee on Transfer as Our `whitelisted` token, Users cannot `PreMarktes::createOffer()` with that specific token and transaction will `revert()` everytime.

justawanderkid

Description

The TokenManager::_transfer() function in the TokenManager contract fails when interacting with ERC20 tokens that charge a transfer fee. This is due to if statements after _safe_transfer_from() being called in _transfer() function that causes the function call to revert().

Now when user creates an offer with PreMarktes::createOffer() function with a CreateOfferParams struct that has that _takeFeeOnTranferERC20 as an CreateOfferParams::tokenaddress, later on inside the PreMarktes::createOffer() we do make call to TokenManager::tillIn(address, address _tokenAddress,uint256, bool) with passing that weirdERC20 token address an _tokenAddress.

TokenManager::tillIn(address, address _tokenAddress,uint256, bool) function if statment takes an look at _tokenAddress, if it's not WETH address, then executes the else block resulting in calling _transfer() function.

in _transfer() function we have two checks after doing _safe_transfer_from(), which is:

if (fromBalanceAft != fromBalanceBef - _amount) {
revert TransferFailed();
}
​
if (toBalanceAft != toBalanceBef + _amount) {
revert TransferFailed();
}

now this checks work correctly on standard ERC20's but not on weird ERC20's.

let's say _amount being transfered is 100e18, and fromBalanceBef is 200e18 but this weird ERC20 contract subtracts 100 from _amount being transfered, which will result in 99999999999999999900 being transfered to _to address. this will result in first if statement after _safe_transfer_from() call fail and revert().

Impact

If the TokenManager::owner() sets an ERC20 token that charges a transfer fee as a whitelisted token, users will face transaction failures when creating offers with PreMarktes::createOffer() function with this specific token. This results in transaction reverts, reduced platform usability and operational disruptions.

Proof of Concepts

Here's How our weirdERC20 Contract Looks like that subtracts 100 units from each transfer value amount:

// Sample fee-charging ERC20 token
​
contract FeeOnTransferERC20 is ERC20Mock {
// lets say this token takes 100 units of token each transfer.
// token decimals is 18.
// in such weird ERC20s there will be a `feeReceiver` i guess, but this is only to demonstrate how it looks.
// more on this you can read on: https://github.com/d-xo/weird-erc20?tab=readme-ov-file#fee-on-transfer
​
function transfer(address to, uint256 value) public override returns (bool) {
address owner = _msgSender();
uint256 fee = _calculateFee(value);
_transfer(owner, to, value - fee);
return true;
}
​
function transferFrom(address from, address to, uint256 value) public override returns (bool) {
address spender = _msgSender();
uint256 fee = _calculateFee(value);
_spendAllowance(from, spender, value - fee);
_transfer(from, to, value - fee);
return true;
}
​
function _calculateFee(uint256 _value) internal pure returns(uint256) {
return _value - 100;
}
}

let's attempt to use the PreMarktes::createOffer() function to create Offer with this specific token. The transfer will revert() due to if statements TokenManager::_transfer() function.

Put the Code Below inside your PreMarkets.t.sol:

function test_transferRevertsIfTokenTakesFeeOnTransfer() external {
vm.startPrank(user1); // user1 is the `owner()` that can call `updateTokenWhiteListed()` function, inside the test files.
FeeOnTransferERC20 feeOnTransferERC20 = new FeeOnTransferERC20();
​
// lets say `user1` sets an weird ERC20 as whitelisted token. this token subtracts 100 from the `value` being transfered.
address[] memory tokenAddressList = new address[](1);
tokenAddressList[0] = address(feeOnTransferERC20);
tokenManager.updateTokenWhiteListed(tokenAddressList, true);
vm.stopPrank();
​
​
vm.startPrank(user2);
// fund the `user2` so has enough tokens to `createOffer()`
deal(address(feeOnTransferERC20), user2, 100000000 * 10 ** 18);
feeOnTransferERC20.approve(address(tokenManager), type(uint256).max);
​
// we expect next line to revert with `TransferFailed()` error from `Rescuable` Contract.
vm.expectRevert(Rescuable.TransferFailed.selector);
​
// we will see that when `user2` wants to create an offer with `feeOnTransferERC20` token, this will revert due to `fee` amount getting subtracted from the `value` being transfered.
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(feeOnTransferERC20),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
​
vm.stopPrank();
}

Run the Test with Following Command inside your terminal:

forge test --match-test test_transferRevertsIfTokenTakesFeeOnTransfer -vvvv

Boom! We See that indeed the PreMarktes::createOffer() function call revert() due to fee amount being subtracted from value amount being transfered.

Recommended Mitigation

remove the following lines from the TokenManager::_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)); // approve this contract to type(uint256).max if not already approved by `CapitalPool` contract.
}
​
_safe_transfer_from(_token, _from, _to, _amount);
​
- uint256 fromBalanceAft = IERC20(_token).balanceOf(_from);- uint256 toBalanceAft = IERC20(_token).balanceOf(_to);
- uint256 toBalanceAft = IERC20(_token).balanceOf(_to);
​
- if (fromBalanceAft != fromBalanceBef - _amount) {
- revert TransferFailed();
- }
​
- if (toBalanceAft != toBalanceBef + _amount) {
- revert TransferFailed();
- }
}

since in Rescuable::_safe_transfer_from() function we do an external call to transferFrom() function of the corresponding token and then we check the returned value of the external
.call() to be true, we can ensure that transfer is done successfully:

function _safe_transfer_from( address token, address from, address to, uint256 amount ) internal {
(bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
);
​
@> if (!success) { // we check here if returned value from external call is `false`, if it is, then `revert()`, otherwise the transfer was successfull.
revert TransferFailed();
}
}
Updates

Lead Judging Commences

0xnevi Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-FOT-Rebasing

Valid medium, there are disruptions to the ability to take market actions. The following functions will be disrupted without the possibiliy of reaching settlement, since the respective offers cannot be created/listed regardless of mode when transferring collateral token required to the CapitalPool contract or when refunding token from user to capital pool during relisting. So withdrawal is not an issue - `createOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L96-L102) - `listOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L355-L362) - `relistOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L515-L521) - `createTaker()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L831-L836) I believe medium severity is appropriate although the likelihood is high and impact is medium (only some level of disruption i.e. FOT tokens not supported and no funds at risk)

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.