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:
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()
.
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.
Here's How our weirdERC20
Contract Looks like that subtracts 100
units from each transfer value
amount:
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
:
Run the Test with Following Command inside your terminal:
Boom! We See that indeed the PreMarktes::createOffer()
function call revert()
due to fee
amount being subtracted from value
amount being transfered.
remove the following lines from the TokenManager::_transfer()
function:
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:
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)
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.