Existing style for naming variables is misleading and decreases code maintainability. Only constant variable names should be in all capital letters. A lot of projects use all capital letters not only for constant but also for immutable variables.
Do not use the same naming convention for state variables and constants. Below is a example with wrong naming convention:
https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L54-L63
/// @dev WETH address of a chain; set at deploy time to the WETH address of the chain that this contract is deployed to
address public immutable WETH;
/// @dev Used in order signing with EIP-712
bytes32 public immutable DOMAIN_SEPARATOR;
/// @dev This is the adress that is used to send auto sniped orders for execution on chain
address public MATCH_EXECUTOR;
/// @dev Gas cost for auto sniped orders are paid by the buyers and refunded to this contract in the form of WETH
uint32 public WETH_TRANSFER_GAS_UNITS = 50000;
/// @notice Exchange fee in basis points (250 bps = 2.5%)
uint16 public PROTOCOL_FEE_BPS = 250;
To:
Local and state variables should use mixedCase. Examples: totalSupply, remainingSupply, balancesOf, creatorAddress, isPreSale, tokenExchangeRate.
Constants should be named with all capital letters with underscores separating words. Examples: MAX_BLOCKS, TOKEN_NAME, TOKEN_TICKER, CONTRACT_VERSION.
Additionally I would like to recommend to use Solidity language naming convention as base and extend it with prefixes:
“i_” prefix for immutable variables
“s” prefix for storage variables
And don’t use prefix “” for private variables because all data on the blockchain is public and the only difference is automatically generated getter function.
Below are naming convention examples:
address public immutable i_weth;
address public s_matchExecutor;
EnumerableSet.AddressSet private s_complications;
Title: Events aren't indexed
Impact
Events are not indexed, so their filtering is disabled, which makes it harder to programmatically use the system
Empty fallback() and receive() do not serve any purpose and unnecessary increases amount of code. It is always possible to send ether from a self-destruct function so empty fallback, receive are not necessary.
Three functions has the same same duplicate code to check for access control. Best practice is to have access control in a modifier and not to copy the logic to each function.
Create a new modifier and add it to above mentioned functions:
modifier onlyExecutor() {
require(msg.sender == MATCHEXECUTOR, 'OME');
;
}
Title: Remove MakerOrder.execParams[]
Impact
MakerOrder.execParams[] makes code hard to read and it stores only two variables:
MakerOrder.execParams[0] - complication address
MakerOrder.execParams[1] - currency
There are many instances of the value 10000. Currently this integer value is used directly without a clear indication that this value relates to the decimals value, which could lead to one of these values being modified but not the other (perhaps by a typo), which is the basis for many past hacks. Coding best practices suggests using a constant integer to store this value in a way that clearly explains the purpose of this value to prevent confusion.
Recommended Mitigation Steps
Create a constant:
Uint256 const BASIS_POINTS = 10000:
HardlyDifficult
commented
2 years ago
Title: Wrong naming convention for variables
Impact
Existing style for naming variables is misleading and decreases code maintainability. Only constant variable names should be in all capital letters. A lot of projects use all capital letters not only for constant but also for immutable variables.
Official Solidity language documentation recommendations should be followed for variable naming convention: https://docs.soliditylang.org/en/latest/style-guide.html?highlight=variable#local-and-state-variable-names
Recommended Mitigation Steps
Do not use the same naming convention for state variables and constants. Below is a example with wrong naming convention: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L54-L63 /// @dev WETH address of a chain; set at deploy time to the WETH address of the chain that this contract is deployed to address public immutable WETH; /// @dev Used in order signing with EIP-712 bytes32 public immutable DOMAIN_SEPARATOR; /// @dev This is the adress that is used to send auto sniped orders for execution on chain address public MATCH_EXECUTOR; /// @dev Gas cost for auto sniped orders are paid by the buyers and refunded to this contract in the form of WETH uint32 public WETH_TRANSFER_GAS_UNITS = 50000; /// @notice Exchange fee in basis points (250 bps = 2.5%) uint16 public PROTOCOL_FEE_BPS = 250;
To: Local and state variables should use mixedCase. Examples: totalSupply, remainingSupply, balancesOf, creatorAddress, isPreSale, tokenExchangeRate. Constants should be named with all capital letters with underscores separating words. Examples: MAX_BLOCKS, TOKEN_NAME, TOKEN_TICKER, CONTRACT_VERSION. Additionally I would like to recommend to use Solidity language naming convention as base and extend it with prefixes:
Title: Events aren't indexed
Impact
Events are not indexed, so their filtering is disabled, which makes it harder to programmatically use the system
Recommended Mitigation Steps
Add indexes for events: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L80-L102
Title: Remove fallback() and receive()
Impact
Empty fallback() and receive() do not serve any purpose and unnecessary increases amount of code. It is always possible to send ether from a self-destruct function so empty fallback, receive are not necessary.
Recommended Mitigation Steps
Remove fallback() and receive(): https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L119-L121 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L54-L57
Title: Create onlyExecutor modifier
Impact
Three functions has the same same duplicate code to check for access control. Best practice is to have access control in a modifier and not to copy the logic to each function.
Recommended Mitigation Steps
Remove code - require(msg.sender == MATCH_EXECUTOR, 'OME'); From: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L138 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L183 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L263
Create a new modifier and add it to above mentioned functions: modifier onlyExecutor() { require(msg.sender == MATCHEXECUTOR, 'OME'); ; }
Title: Remove MakerOrder.execParams[]
Impact
MakerOrder.execParams[] makes code hard to read and it stores only two variables: MakerOrder.execParams[0] - complication address MakerOrder.execParams[1] - currency
Both .execParams[] are checked at the main contract so keeping them as a array doesn’t give any benefit and only complicates code: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L442-L443
Recommended Mitigation Steps
Change: MakerOrder.execParams[0] MakerOrder.execParams[1] To: MakerOrder.compAddress MakerOrder.currency
Title: Constant variables should be declared as constants and not memory variables
Impact
Only constant variables should use the appropriate constant naming convention.
Recommended Mitigation Steps
Change: uint256 PRECISION = 104; // precision for division; similar to bps https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityOrderBookComplication.sol#L338 To: uint256 constant PRECISION = 104; // precision for division; similar to bps
Title: Magic numbers should be constants
Impact
There are many instances of the value 10000. Currently this integer value is used directly without a clear indication that this value relates to the decimals value, which could lead to one of these values being modified but not the other (perhaps by a typo), which is the basis for many past hacks. Coding best practices suggests using a constant integer to store this value in a way that clearly explains the purpose of this value to prevent confusion.
Recommended Mitigation Steps
Create a constant: Uint256 const BASIS_POINTS = 10000:
And replace magic number 10000 with the new constant: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L725 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L775 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L819 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L873 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1135
Replace 10**18: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L237