search

code-423n4 / 2021-09-defiprotocol-findings

1 stars 0 forks source link

Cannot handle compliant ERC20 tokens that takes fee on transfer #106

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

itsmeSTYJ

Vulnerability details

Impact

USDT is the most popular example of this where there exists a flag to turn on a "fee per transaction". It is currently turned off but you do not know when it'll turn on. There might be more tokens like these in the future where a portion of the amount will be taken as fees whenever the tokens are transferred.

You want to make sure your protocol can handle edgecases like these where the amount that is requested to be transferred is less than the actual amount transferred.

Recommended Mitigation Steps

I don't think there's a correct way to mitigate against this but as a start, you should check the tokenBalance before and after the transfer to get the actual amount transferred. Once you have the actual amount of tokens transferred you can either:

  1. revert the entire transaction if it's less than the amount specified
  2. keep track of the lowest amount of tokens transferred, refund the excess tokens
  3. keep track of the lowest amount of tokens, ignore excess

I would go for option 2 because it allows your protocol to use these tokens in the basket as opposed to rejecting them and it's easier to explain why the excess are refunded than to explain why the excess are kept.

Note that this fix should be applied to all lines of code where safeTransferFrom is used.

frank-beard commented 2 years ago

https://github.com/code-423n4/2021-09-defiprotocol-findings/issues/236

GalloDaSballo commented 2 years ago

Duplicate of #236