code-423n4 / 2022-06-infinity-findings

4 stars 0 forks source link

QA Report #265

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

[L-01] Immutable addresses should 0-Check

I recommend adding check of 0-address for immutable addresses. Not doing so might lead to non-functional contract when it is updated to 0-address accidentally.

Issue found at

InfinityExchange.sol
 55:  address public immutable WETH;
104:  constructor(address _WETH, address _matchExecutor) {
115:    WETH = _WETH;

[L-02] abi.encodePacked should not be used with dynamic types

This is because using abi.encodePacked with dynamic types will cause a hash collisions. link: https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode I recommend using abi.encode instead.

./InfinityExchange.sol:1197:    bytes32 nftsHash = keccak256(abi.encodePacked(hashes));
./InfinityExchange.sol:1213:    bytes32 tokensHash = keccak256(abi.encodePacked(hashes));

[N-01] Event is missing indexed fields

Each event should have 3 indexed fields if there are 3 or more fields.

Issue found at

./InfinityToken.sol:35:  event EpochAdvanced(uint256 currentEpoch, uint256 supplyMinted);
./InfinityStaker.sol:44:  event Staked(address indexed user, uint256 amount, Duration duration);
./InfinityStaker.sol:45:  event DurationChanged(address indexed user, uint256 amount, Duration oldDuration, Duration newDuration);
./InfinityStaker.sol:46:  event UnStaked(address indexed user, uint256 amount);
./InfinityStaker.sol:47:  event RageQuit(address indexed user, uint256 totalToUser, uint256 penalty);
./InfinityExchange.sol:80:  event CancelAllOrders(address user, uint256 newMinNonce);
./InfinityExchange.sol:81:  event CancelMultipleOrders(address user, uint256[] orderNonces);
./InfinityExchange.sol:82:  event NewWethTransferGasUnits(uint32 wethTransferGasUnits);
./InfinityExchange.sol:83:  event NewProtocolFee(uint16 protocolFee);
./InfinityExchange.sol:85-93:  event MatchOrderFulfilled(bytes32 sellOrderHash, bytes32 buyOrderHash, address seller, address buyer, address complication, // address of the complication that defines the execution address currency, // token address of the transacting currency uint256 amount // amount spent on the order);
./InfinityExchange.sol:95:  event TakeOrderFulfilled(bytes32 orderHash, address seller, address buyer, address complication, // address of the complication that defines the execution address currency, // token address of the transacting currency uint256 amount // amount spent on the order);

[N-02] Unnecessary use of named returns

Several function adds return statement even thought named returns variable are used. Remove unnecessary named returns variable to improve code readability. Also keeping the use of named returns or return statement consistent through out the whole project if possible is recommended.

Issue found at InfinityToken.sol

Remove returns variable "admin"
113:  function getAdmin() public view returns (address admin) {
114:    return address(uint160(TimelockConfig.getConfig(TimelockConfig.ADMIN).value));
115:  }
Remove returns variable "timelock"
117:  function getTimelock() public view returns (uint256 timelock) {
118:    return TimelockConfig.getConfig(TimelockConfig.TIMELOCK).value;
119:  }
Remove returns variable "inflation"
121:  function getInflation() public view returns (uint256 inflation) {
122:    return TimelockConfig.getConfig(EPOCH_INFLATION).value;
123:  }
Remove returns variable "cliff"
125:  function getCliff() public view returns (uint256 cliff) {
126:    return TimelockConfig.getConfig(EPOCH_CLIFF).value;
127:  }
Remove returns variable "totalEpochs"
129:  function getMaxEpochs() public view returns (uint256 totalEpochs) {
130:    return TimelockConfig.getConfig(MAX_EPOCHS).value;
131:  }
Remove returns variable "epochDuration"
133:  function getEpochDuration() public view returns (uint256 epochDuration) {
134:    return TimelockConfig.getConfig(EPOCH_DURATION).value;
135:  }

[N-03] Large Multiples of Ten should use Scientific Notation

Large multiples of ten is hard to read so use scientific notation instead for readability.

Issue found at

./InfinityStaker.sol:35:  uint16 public GOLD_STAKE_THRESHOLD = 10000;
./InfinityExchange.sol:381:    require(minNonce < userMinOrderNonce[msg.sender] + 1000000, 'too many');
./InfinityExchange.sol:725:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
./InfinityExchange.sol:775:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
./InfinityExchange.sol:819:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
./InfinityExchange.sol:873:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
./InfinityExchange.sol:1135:    uint256 protocolFee = (PROTOCOL_FEE_BPS * amount) / 10000;