Infinity NFT Marketplace contest - TomJ's results

[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;

[G-01] Unnecessary Default Value Initialization

When variable is not initialized, it will have its default values. Example: 0 for uint, false for bool and address(0) for address

I suggest removing default value initialization for following variables.

./InfinityOrderBookComplication.sol:42:    bool _isPriceValid = false;
./InfinityOrderBookComplication.sol:76:    for (uint256 i = 0; i < ordersLength; ) {
./InfinityOrderBookComplication.sol:82:      for (uint256 j = 0; j < nftsLength; ) {
./InfinityOrderBookComplication.sol:108:    bool _isPriceValid = false;
./InfinityOrderBookComplication.sol:197:    uint256 numConstructedItems = 0;
./InfinityOrderBookComplication.sol:199:    for (uint256 i = 0; i < nftsLength; ) {
./InfinityOrderBookComplication.sol:214:    uint256 numTakerItems = 0;
./InfinityOrderBookComplication.sol:216:    for (uint256 i = 0; i < nftsLength; ) {
./InfinityOrderBookComplication.sol:244:    uint256 numCollsMatched = 0;
./InfinityOrderBookComplication.sol:246:    for (uint256 i = 0; i < order2NftsLength; ) {
./InfinityOrderBookComplication.sol:247:      for (uint256 j = 0; j < order1NftsLength; ) {
./InfinityOrderBookComplication.sol:289:    uint256 numTokenIdsPerCollMatched = 0;
./InfinityOrderBookComplication.sol:290:    for (uint256 k = 0; k < item2TokensLength; ) {
./InfinityOrderBookComplication.sol:291:      for (uint256 l = 0; l < item1TokensLength; ) {
./InfinityOrderBookComplication.sol:318:    uint256 sum = 0;
./InfinityOrderBookComplication.sol:320:    for (uint256 i = 0; i < ordersLength; ) {
./InfinityExchange.sol:148:    for (uint256 i = 0; i < numMakerOrders; ) {
./InfinityExchange.sol:200:      for (uint256 i = 0; i < ordersLength; ) {
./InfinityExchange.sol:219:      for (uint256 i = 0; i < ordersLength; ) {
./InfinityExchange.sol:272:    for (uint256 i = 0; i < numSells; ) {
./InfinityExchange.sol:308:    for (uint256 i = 0; i < numMakerOrders; ) {
./InfinityExchange.sol:349:    for (uint256 i = 0; i < ordersLength; ) {
./InfinityExchange.sol:393:    for (uint256 i = 0; i < numNonces; ) {
./InfinityExchange.sol:1048:    for (uint256 i = 0; i < numNfts; ) {
./InfinityExchange.sol:1086:    for (uint256 i = 0; i < numTokens; ) {
./InfinityExchange.sol:1109:    for (uint256 i = 0; i < numNfts; ) {
./InfinityExchange.sol:1190:    for (uint256 i = 0; i < numNfts; ) {
./InfinityExchange.sol:1206:    for (uint256 i = 0; i < numTokens; ) {

For example these can change to:

• bool _isPriceValid;
• for (uint8 i; i < _pooledTokens.length; i++) {

[G-02] Defined Variables Used Only Once

Certain variables is defined even though they are used only once. Remove these unnecessary variables to save gas. For cases where it will reduce the readability, one can use comments to help describe what the code is doing.

Issue found at

1. InfinityExchange.sol

• Remove isTimeValid variable of takeMultipleOneOrders function

311:      bool isTimeValid = makerOrders[i].constraints[3] <= block.timestamp &&
313:      require(isTimeValid, 'invalid time');

• Remove currenciesMatch variable of verifyMatchOneToOneOrders function

442:    bool currenciesMatch = sell.execParams[1] == buy.execParams[1] ||
443:      (sell.execParams[1] == address(0) && buy.execParams[1] == WETH);
447:      currenciesMatch &&

• Remove currenciesMatch variable of verifyMatchOneToManyOrders function

466:    bool currenciesMatch = sell.execParams[1] == buy.execParams[1] ||
467:      (sell.execParams[1] == address(0) && buy.execParams[1] == WETH);
477:      currenciesMatch &&

• Remove currenciesMatch variable of verifyMatchOrders function

497:    bool currenciesMatch = sell.execParams[1] == buy.execParams[1] ||
498:      (sell.execParams[1] == address(0) && buy.execParams[1] == WETH);
502:      currenciesMatch &&

For mitigation, simply don't define variable that is used only once. Below is mitigation example of above 1.

      require(makerOrders[i].constraints[3] <= block.timestamp && makerOrders[i].constraints[4] >= block.timestamp, 'invalid time');

[G-03] != 0 costs less gass then > 0

!= 0 costs less gas when optimizer is enabled and is used for unsigned integers in "require" statement. I suggest changing > 0 to != 0

Issue found at:

./InfinityExchange.sol:392:    require(numNonces > 0, 'cannot be empty');

[G-04] Reduce the long revert strings of error messages

By keeping the revert strings within 32 bytes will save you gas since each slot is 32 bytes.

Following are revert strings that are more than 32 bytes. I recommend to keep these error messages within 32 bytes.

./InfinityStaker.sol:92-95:    require(userstakedAmounts[msg.sender][oldDuration].amount >= amount, 'insufficient staked amount to change duration');
./InfinityStaker.sol:96:    require(newDuration > oldDuration, 'new duration must be greater than old duration');
./InfinityExchange.sol:395:      require(!isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]], 'nonce already executed or cancelled');

[G-05] Use Custom Errors to Save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings. I recommend using custom errors.

./InfinityToken.sol:61:    require(currentEpoch < getMaxEpochs(), 'no epochs left');
./InfinityToken.sol:62:    require(block.timestamp >= currentEpochTimestamp + getCliff(), 'cliff not passed');
./InfinityToken.sol:63:    require(block.timestamp >= previousEpochTimestamp + getEpochDuration(), 'not ready to advance');
./InfinityStaker.sol:68:    require(amount != 0, 'stake amount cant be 0');
./InfinityStaker.sol:69:    require(IERC20(INFINITY_TOKEN).balanceOf(msg.sender) >= amount, 'insufficient balance to stake');
./InfinityStaker.sol:91:    require(amount != 0, 'amount cant be 0');
./InfinityStaker.sol:92-95:    require(userstakedAmounts[msg.sender][oldDuration].amount >= amount, 'insufficient staked amount to change duration');
./InfinityStaker.sol:96:    require(newDuration > oldDuration, 'new duration must be greater than old duration');
./InfinityStaker.sol:117:    require(amount != 0, 'stake amount cant be 0');
./InfinityStaker.sol:123:    require(totalVested >= amount, 'insufficient balance to unstake');
./InfinityStaker.sol:193:    require(totalStaked >= 0, 'nothing staked to rage quit');
./InfinityStaker.sol:347:    require(sent, 'Failed to send Ether');
./InfinityOrderBookComplication.sol:255:          require(tokenIdsIntersect, 'tokenIds dont intersect');
./InfinityExchange.sol:138:    require(msg.sender == MATCH_EXECUTOR, 'OME');
./InfinityExchange.sol:139:    require(numMakerOrders == makerOrders2.length, 'mismatched lengths');
./InfinityExchange.sol:150:      require(_complications.contains(makerOrders1[i].execParams[0]), 'invalid complication');
./InfinityExchange.sol:155:      require(canExec, 'cannot execute');
./InfinityExchange.sol:183:    require(msg.sender == MATCH_EXECUTOR, 'OME');
./InfinityExchange.sol:184:    require(_complications.contains(makerOrder.execParams[0]), 'invalid complication');
./InfinityExchange.sol:185-188:    require(IComplication(makerOrder.execParams[0]).canExecMatchOneToMany(makerOrder, manyMakerOrders), 'cannot execute');
./InfinityExchange.sol:190:    require(isOrderValid(makerOrder, makerOrderHash), 'invalid maker order');
./InfinityExchange.sol:263:    require(msg.sender == MATCH_EXECUTOR, 'OME');
./InfinityExchange.sol:264:    require(numSells == buys.length && numSells == constructs.length, 'mismatched lengths');
./InfinityExchange.sol:279:      require(executionValid, 'cannot execute');
./InfinityExchange.sol:306:      require(currency != address(0), 'offers only in ERC20');
./InfinityExchange.sol:310:      require(isOrderValid(makerOrders[i], makerOrderHash), 'invalid maker order');
./InfinityExchange.sol:313:      require(isTimeValid, 'invalid time');
./InfinityExchange.sol:314:      require(currency == makerOrders[i].execParams[1], 'cannot mix currencies');
./InfinityExchange.sol:315:      require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides');
./InfinityExchange.sol:326:      require(msg.value >= totalPrice, 'invalid total price');
./InfinityExchange.sol:342:    require(ordersLength == takerNfts.length, 'mismatched lengths');
./InfinityExchange.sol:347:      require(currency != address(0), 'offers only in ERC20');
./InfinityExchange.sol:350:      require(currency == makerOrders[i].execParams[1], 'cannot mix currencies');
./InfinityExchange.sol:351:      require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides');
./InfinityExchange.sol:362:      require(msg.value >= totalPrice, 'invalid total price');
./InfinityExchange.sol:380:    require(minNonce > userMinOrderNonce[msg.sender], 'nonce too low');
./InfinityExchange.sol:381:    require(minNonce < userMinOrderNonce[msg.sender] + 1000000, 'too many');
./InfinityExchange.sol:392:    require(numNonces > 0, 'cannot be empty');
./InfinityExchange.sol:394:      require(orderNonces[i] >= userMinOrderNonce[msg.sender], 'nonce too low');
./InfinityExchange.sol:395:      require(!isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]], 'nonce already executed or cancelled');
./InfinityExchange.sol:587:    require(verifyMatchOneToOneOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified');
./InfinityExchange.sol:621:    require(verifyMatchOneToManyOrders(buyOrderHash, false, sell, buy), 'order not verified');
./InfinityExchange.sol:649:    require(verifyMatchOneToManyOrders(sellOrderHash, true, sell, buy), 'order not verified');
./InfinityExchange.sol:684:    require(verifyMatchOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified');
./InfinityExchange.sol:949:    require(makerOrderValid && executionValid, 'order not verified');
./InfinityExchange.sol:1141:      require(sent, 'failed to send ether to seller');
./InfinityExchange.sol:1231:    require(sent, 'failed');

[G-06] Both named returns and return statement are used

Removing unused named returns variable in below code can save gas and improve code readability.

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:  }

[G-07] Use require instead of &&

When there are multiple conditions in require statement, break down the require statement into multiple require statements instead of using && can save gas.

Issue found at

./InfinityExchange.sol:264:    require(numSells == buys.length && numSells == constructs.length, 'mismatched lengths');
./InfinityExchange.sol:949:    require(makerOrderValid && executionValid, 'order not verified');

For example these can be changed to

require(numSells == buys.length);
require(numSells == constructs.length, 'mismatched lengths');