idexio/idex-contracts-whistler

IDEX 2.0 smart contracts

idex-contracts-whistler's Issues

QSP-6 Old ERC20 Tokens May Not Work With IDEX

Severity: Informational

Description: Since Solidity 0.4.22, some non-ERC20 compliant token contracts that used to work in older versions no longer work now. Since many token contracts implement the transfer function by either returning true or reverting the call altogether, some contract developers deemed the return value unnecessary, writing contracts with a transfer function with signature function transfer(address to, uint amount) public { ... }. All non-ERC20 compliant contracts like the latter case that used to work before 0.4.22 will fail in IDEX, as it relies on Solidity 0.6.8. More info can be found here.

Recommendation: This is merely an educational point. Nonetheless, we suggest scripting token registration to check whether target tokens contracts implement the ERC20 standard, with matching function signatures. Needless to say this check is restricted to tokens with a known ABI and/or source code (e.g., as published in some blockchain explorers, like Etherscan).

QSP-4 Missing Input Validation

Severity: Low Risk

File(s) affected: contracts/Exchange.sol, contracts/Governance.sol, contracts/libraries/AssetRegistry.sol

Description:

• contracts/Exchange.sol: wallet should be checked to be different from 0x0 in loadBalanceInAssetUnitsByAddress, loadBalanceInPipsBySymbol, and loadBalanceInAssetUnitsBySymbol.
• contracts/Governance.sol: Functions setCustodian, initiateExchangeUpgrade, and initiateGovernanceUpgrade do not check if the given address parameter is a contract.
• libraries/AssetRegistry.sol: Function registerToken does not check if tokenAddress is different from 0x0. Moreover, it does not check if tokenAddress is a contract address.
• libraries/AssetRegistry.sol: Function registerToken does not check if symbol is different from the empty string.

Recommendation: Add checks fixing the enumerated cases. For checking whether an address is a contract, refer to AddressUtils.

QSP-1 Integer Overflow / Underflow

Severity: High Risk

File(s) affected: contracts/libraries/AssetUnitConversions.sol, contracts/libraries/UUID.sol

Description: Integer overflow/underflow occur when an integer hits its bit-size limit. Every integer has a set range; when that range is passed, the value loops back around. A clock is a good analogy: at 11:59, the minute hand goes to 0, not 60, because 59 is the largest possible minute. Integer overflow and underflow may cause many unexpected kinds of behavior and was the core reason for the batchOverflow attack.

Exploit Scenario:

• AssetUnitConversions.sol (L22): the statement return uint256(quantityInPips) * uint256(10)**(assetDecimals - 8) can overflow, as there is no upper bound check on the value of assetDecimals and quantityInPips.
• UUID.sol (L26): the subtraction statement could lead to an integer underflow, especially when called by [unaware] end users via invalidateOrderNonce().

Recommendation: Use SafeMath to perform arithmetic operations - if overflow/underflow occurs, transactions safely revert.

QSP-7 Unlocked Pragma

Severity: Informational

File(s) affected: contracts/*.sol, contracts/libraries/*.sol

Description: Every Solidity file specifies in the header a version number of the format pragma solidity (^)0.4.*. The caret (^) before the version number implies an unlocked pragma, meaning that the compiler will use the specified version and above, hence the term "unlocked." For consistency and to prevent unexpected behavior in the future, it is recommended to remove the caret to lock the file onto a specific Solidity version.

Recommendation: Except for Migrations.sol, where the Solidity version is already locked, all other files DO NOT lock the Solidity version. For those, lock the version to 0.6.8.