ERC20Wrapper does not pass through underlying token's decimals()

[matthewdgreen]

The ERC20Wrapper contract is designed to create "wrapped" tokens based on some underlying ERC20 token. This should expose the basic properties of the underlying token wrapped on a one-for-one basis.

However, the ERC20Wrapper contract does not expose the result of the underlying token's decimals() method in its own decimals() implementation. Instead, it inherits the standard implementation from ERC20.sol which is hard-coded to simply output 18. Hence an underlying token can have a different decimals() result than its wrapped counterpart.

In practice it is, of course, very unusual for tokens to have decimals() != 18, but in practice it can pose a problem for front-ends that respect the decimals() field of a token and expect its wrapped counterpart to use the same value. Somebody could create an ERC20 with weird decimals() and then the UI for wrapping would receive a different decimals value from the wrapped and underlying tokens. This could cause strange math errors that would confuse the user, and convince them that the wrapped token was malfunctioning.

💻 Environment

Latest (in Git)

📝 Details

If this is worth fixing (and I don't know), the main issue here is with tokens that don't implement a decimals() method. This edge-case would need to be handled, but doesn't seem like a big deal.

🔢 Code to reproduce bug

1. Make a token that has decimals() != 18
2. Create an instance of an ERC20Wrapper based on this token.
3. Call decimals() on the original and underlying, they won't be the same.

[tromer]

Theoretically this could even be exploited by an attacker, confusing a user to send more of the underlying asset than they intended to. E.g., the user sends "1" of the wrapped tokens, which the frontend converts to 1e18 according to ERC20Wrapper.decimals()==18, but actually this represents 100 of the underlying token which happens to have decimals()==16.

[Amxx]

Thank you @tromer for pointing this out.

We are definitely going to fix that, even though we might deprecate the ERC20Wrapper in favor of ERC4626 vaults in the future.