search

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

0 stars 0 forks source link

Requiring a decimals method for ERC-20 tokens is non-standard #136

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

hrkrshnn

Vulnerability details

Requiring a decimals method for ERC-20 tokens is non-standard

The ERC-20 token standard specifies that

decimals

Returns the number of decimals the token uses - e.g. 8, means to divide the token amount by
100000000 to get its user representation.

OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT
expect these values to be present.

However, there are several instances where this is required. Some of the instances (non-exhaustive) are given below:

./contracts/Auctions/BatchAuction.sol:143:        require(IERC20(_token).decimals() == 18, "BatchAuction: Token does not have 18 decimals");
./contracts/Auctions/BatchAuction.sol:145:            require(IERC20(_paymentCurrency).decimals() > 0, "BatchAuction: Payment currency is not ERC20");
./contracts/Auctions/Crowdsale.sol:169:        require(IERC20(_token).decimals() == 18, "Crowdsale: Token does not have 18 decimals");
./contracts/Auctions/Crowdsale.sol:171:            require(IERC20(_paymentCurrency).decimals() > 0, "Crowdsale: Payment currency is not ERC20");
./contracts/Auctions/DutchAuction.sol:158:        require(IERC20(_token).decimals() == 18, "DutchAuction: Token does not have 18 decimals");
./contracts/Auctions/DutchAuction.sol:160:            require(IERC20(_paymentCurrency).decimals() > 0, "DutchAuction: Payment currency is not ERC20");
./contracts/Auctions/HyperbolicAuction.sol:161:        require(IERC20(_token).decimals() == 18, "HyperbolicAuction: Token does not have 18 decimals");
./contracts/Auctions/HyperbolicAuction.sol:163:            require(IERC20(_paymentCurrency).decimals() > 0, "HyperbolicAuction: Payment currency is not ERC20");
./contracts/Helper/CalculationsSushiswap.sol:20:    function decimals() external view returns (uint8);

In particular, according to the standard the revert comment "Payment currency is not ERC20" is incorrect. It is recommended to change this comment. It may also be more pedantic to use a second interface ExtendedERC20 which has the function decimals() and replace the above uses with that.

Clearwood commented 2 years ago

While the original EIP makes it optional, I do believe that it is reasonable to build a system that only works with the extended standard. An extended interface might make this more obvious but would not be a high priority.

ghoul-sol commented 2 years ago

In practice, one always expects decimals() to be there. Making this a non-critical as it is best practices recommendation