Deposits don't work with fee-on transfer tokens

[code423n4]

Handle

cmichel

Vulnerability details

Vulnerability Details

There are ERC20 tokens that may make certain customizations to their ERC20 contracts.
One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

Impact

The deposit() function will introduce unexpected balance inconsistencies when comparing internal asset records with external ERC20 token contracts.

Recommended Mitigation Steps

One possible mitigation is to measure the asset change right before and after the asset-transferring routines

[mcplums]

I think balancedBooks modifier should handle this?

Of course it means we are unable to use such tokens, but that is ok

[Splidge]

oh, trying the same one again..? 😁
code-423n4/2021-05-88mph-findings#16

I'll fight this one though, I'd argue that we are using ERC20 tokens and according to the ERC20 spec for transferFrom:

Transfers _value amount of tokens from address _from to address _to

A deflationary token therefore isn't compliant to ERC20 as it doesn't transfer the full _value and so it isn't what we are planning to use and not relevant here.

[dmvt]

If you plan not to support these tokens it should be very clearly documented. Keep in mind that "we don't support that" still has massive impact on the users involved. See: imBTC / ERC777 on Uniswap v1. The issue is valid and should stand in the audit report, in part so that future users see it.

[Steiner-254]

Interesting ...haha