Infinity NFT Marketplace contest - StErMi's results

InfinityStaker

style: avoid to use uppercase notation when not needed, should be used only for constant/immutable

Usually in Solidity, the uppercase notation is used only for constant/immutable variable. The code use this notation for various variables:

• INFINITY_TREASURY
• BRONZE_STAKE_THRESHOLD
• SILVER_STAKE_THRESHOLD
• GOLD_STAKE_THRESHOLD
• PLATINUM_STAKE_THRESHOLD
• THREE_MONTH_PENALTY
• SIX_MONTH_PENALTY
• TWELVE_MONTH_PENALTY

Consider changing the name of those variable in camelCase style following the Solidity documentation style guide.

updateStakeLevelThreshold should emit an event

The stake level threshold is important to determinate the stake level of a user's stake. This function should emit an event when a threshold is updated

Consider creating an event when a value is updated and emit it

emit StakeLevelThreshold(stakeLevel, threshold);

updateStakeLevelThreshold is not checking the user input

From a semantic point and from how the logic of getUserStakeLevel works, each threshold should have

1. different values
2. be in a DESC order from PLATINUM to NONE

Without any check, the owner could make all the threshold equal to zero or reverse the order (bronze_threshold = 1000, platinum_threshold = 1)

Consider checking that when updating the value of threshold value, it's greater than the previous but less than the next.

For example, when updating the StakeLevel.SILVER threshold it must be

• newThreshold >= BRONZE_STAKE_THRESHOLD
• newThreshold <= GOLD_STAKE_THRESHOLD

updatePenalties should emit an event

Penalty value is important to understand how much of the user vested amount will be taken as a penalty when he/she rageQuit.

When that value is updated, it should be tracked via an event. Consider emitting an event like: emit PenaltiesUpdated(threeMonthPenalty, sixMonthPenalty, twelveMonthPenalty);

updateInfinityTreasury should emit an event

When the treasury is updated, the function should emit an event

Consider emitting an event like InfinityTreasuryUpdated(_infinityTreasury)

updatePenalties should check the value of input parameter

While there should be a specific check to prevent to have a value equal to 0 for any of the penalties to prevent rageQuit to revert (already submitted as a separate issue) those values should be checked to preserve a hierarchy like this

THREE_MONTH_PENALTY < SIX_MONTH_PENALTY < TWELVE_MONTH_PENALTY as already specified at contract deployment time.

InfinityExchange

Declare event parameter as indexed when possible

Some events are missing the indexed keyword on addresses. Using the indexed parameters allow external monitoring tool to filter those logs and monitor events in a better way.

• CancelAllOrders can declare user as indexed
• CancelMultipleOrders can declare user as indexed
• MatchOrderFulfilled can declare seller, buyer, complication, currency as indexed (max 3)
• TakeOrderFulfilled can declare seller, buyer, complication, currency as indexed (max 3)

Consider declaring those event parameters as indexed to better monitor/filter those events.

Missing event for critical functions

A function that set/update state variable should always emit an event for both monitoring reason, but also as a best practice to allow users to track that a contract's parameter/behavior has changed.

All these functions are missing an event emission:

• rescueETH
• addCurrency
• addComplication
• removeCurrency
• removeComplication
• updateMatchExecutor

Consider adding an event emission for those functions

PROTOCOL_FEE_BPS has no min/max value and no check in setProtocolFee

Having a PROTOCOL_FEE_BPS equal to 10_000 mean that the protocol take the whole amount and the user take 0 when selling an NFT. Having a PROTOCOL_FEE_BPS equal to 0 mean that the protocol will get nothing when NFTs are sold.

There should be an explicit min/max value to be documented (for the user/creator) and checked in setProtocolFee to avoid this edge cases.

If the 0 protocol fee is a correct use case, consider to also update _transferFees to skip the calculation and skip the IERC20(currency).safeTransferFrom(buyer, address(this), protocolFee); that would waste gas transferring 0 tokens to the contract.

InfinityToken

advancedEpoch use local variable to save gas

At the moment the method is calling getEpochDuration twice wasting gas for two SLOAD with a real need because the duration value won't change in that portion of code.

Instead of doing those two SLOAD the value can be saved in a local temp variable

require(currentEpoch < getMaxEpochs(), 'no epochs left');
require(block.timestamp >= currentEpochTimestamp + getCliff(), 'cliff not passed');


- require(block.timestamp >= previousEpochTimestamp + getEpochDuration(), 'not ready to advance');
- uint256 epochsPassedSinceLastAdvance = (block.timestamp - previousEpochTimestamp) / getEpochDuration();
+ uint256 epochDuration = getEpochDuration();
+ require(block.timestamp >= previousEpochTimestamp + epochDuration, 'not ready to advance');
+ uint256 epochsPassedSinceLastAdvance = (block.timestamp - previousEpochTimestamp) / epochDuration;

InfinityStaker

INFINITY_TOKEN can be set as immutable

The current state variable INFINITY_TOKEN is not immutable even if it's only initialized during the constructor and never changed during the code execution.

Declare it as immmutable to save gas.

_updateUserStakedAmounts could save gas by skipping the update of a duration when needed

At the moment if the amount > vestedAmountOfDuration the code is doing 2 SSTORE each time the function is called. This function could be called multiple time during the lifetime of a vest period and the "lower" vesting period are always called before the longer one.

The code could be changed like this

if (amount > noVesting) {

-  userstakedAmounts[user][Duration.NONE].amount = 0;
-  userstakedAmounts[user][Duration.NONE].timestamp = 0;
-  amount = amount - noVesting;
+  if( userstakedAmounts[user][Duration.NONE].amount > 0 ) {
+    userstakedAmounts[user][Duration.NONE].amount = 0;
+    userstakedAmounts[user][Duration.NONE].timestamp = 0;
+    amount = amount - noVesting;
  }
  if (amount > vestedThreeMonths) {
-    userstakedAmounts[user][Duration.THREE_MONTHS].amount = 0;
-    userstakedAmounts[user][Duration.THREE_MONTHS].timestamp = 0;
-    amount = amount - vestedThreeMonths;
+    if( userstakedAmounts[user][Duration.THREE_MONTHS].amount > 0 ) {
+      userstakedAmounts[user][Duration.THREE_MONTHS].amount = 0;
+      userstakedAmounts[user][Duration.THREE_MONTHS].timestamp = 0;
+      amount = amount - vestedThreeMonths;
    }
    if (amount > vestedSixMonths) {
-      userstakedAmounts[user][Duration.SIX_MONTHS].amount = 0;
-      userstakedAmounts[user][Duration.SIX_MONTHS].timestamp = 0;
-      amount = amount - vestedSixMonths;
+      if( userstakedAmounts[user][Duration.SIX_MONTHS].amount > 0 ) {
+        userstakedAmounts[user][Duration.SIX_MONTHS].amount = 0;
+        userstakedAmounts[user][Duration.SIX_MONTHS].timestamp = 0;
+        amount = amount - vestedSixMonths;
      }
      if (amount > vestedTwelveMonths) {
-        userstakedAmounts[user][Duration.TWELVE_MONTHS].amount = 0;
-        userstakedAmounts[user][Duration.TWELVE_MONTHS].timestamp = 0;
+        if( userstakedAmounts[user][Duration.TWELVE_MONTHS].amount > 0 ) { 
+          userstakedAmounts[user][Duration.TWELVE_MONTHS].amount = 0;
+          userstakedAmounts[user][Duration.TWELVE_MONTHS].timestamp = 0;
+        }
      } else {
        userstakedAmounts[user][Duration.TWELVE_MONTHS].amount -= amount;
      }
    } else {
      userstakedAmounts[user][Duration.SIX_MONTHS].amount -= amount;
    }
  } else {
    userstakedAmounts[user][Duration.THREE_MONTHS].amount -= amount;
  }
} else {
  userstakedAmounts[user][Duration.NONE].amount -= amount;
}

The amount and timestamp will be changed only when needed (duration.amount > 0). While it's true that we are introducing an additional SLOAD + 2 SSTORE in the worst case scenario, in the best case scenario we are only using an SLOAD

getRageQuitAmounts can save gas by moving the totalStaked check before loading vested amount

The code of the getRageQuitAmounts could be changed like this

function getRageQuitAmounts(address user) public view override returns (uint256, uint256) {
  uint256 noLock = userstakedAmounts[user][Duration.NONE].amount;
  uint256 threeMonthLock = userstakedAmounts[user][Duration.THREE_MONTHS].amount;
  uint256 sixMonthLock = userstakedAmounts[user][Duration.SIX_MONTHS].amount;
  uint256 twelveMonthLock = userstakedAmounts[user][Duration.TWELVE_MONTHS].amount;
+  uint256 totalStaked = noLock + threeMonthLock + sixMonthLock + twelveMonthLock;
+  require(totalStaked >= 0, 'nothing staked to rage quit');

  uint256 threeMonthVested = getVestedAmount(user, Duration.THREE_MONTHS);
  uint256 sixMonthVested = getVestedAmount(user, Duration.SIX_MONTHS);
  uint256 twelveMonthVested = getVestedAmount(user, Duration.TWELVE_MONTHS);

  uint256 totalVested = noLock + threeMonthVested + sixMonthVested + twelveMonthVested;
-  uint256 totalStaked = noLock + threeMonthLock + sixMonthLock + twelveMonthLock;
-  require(totalStaked >= 0, 'nothing staked to rage quit');

  // [...] other code
}