Infinity NFT Marketplace contest - 0x1f8b's results

Low

Centralized risks

Owner can change the thresholds wherever he wants.

• InfinityStaker.sol#L351

Also he can change the penalties, and it could facilitate a front-running issues with bad actors.

• InfinityStaker.sol#L364-L372

PROTOCOL_FEE_BPS could be higher than factor (10_000) and it could facilitate a front-running or denial of services with bad actors.

• InfinityExchange.sol#L1267

Unfair staking increase

If the user makes any changes or changeDuration, the staking timestamp is reset and loses all the time already spent in the previous deposit.

Affected source code:

• InfinityStaker.sol#L72
• InfinityStaker.sol#L102

No compatible with fee tokens

The current lock logic does not contemplate ERC20 tokens with fee during the transferFrom, therefore, the amount received by BribeVault will be less than the expected.

Some tokens may implement a fee during transfers, this is the case of USDT, even though the project has currently set it to 0. So, the transferFrom function would return true despite receiving less than expected.

It's recommended to use balance difference in:

• InfinityStaker.sol#L74
• InfinityExchange.sol#L729
• InfinityExchange.sol#L844

Ownable / Pausable

The contract InfinityStaker is Ownable and Pausable, so the owner could resign while the contract is paused, causing a Denial of Service. Owner resignation while paused should be avoided.

Affected source code:

• InfinityStaker.sol#L15

Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

requestChange(ADMIN,X):

• TimelockConfig.sol#L121

Ownable:

• InfinityExchange.sol#L50
• InfinityStaker.sol#L15
• InfinityOrderBookComplication.sol#L14
• MockERC721.sol#L7

Lack of checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

address(0):

• TimelockConfig.sol#L44
• InfinityStaker.sol#L375
• InfinityExchange.sol#L116

Integer values:

• InfinityToken.sol#L39-L44
• TimelockConfig.sol#L45

Non critical

Update packages

The packages used are out of date, it is good practice to use the latest version of these packages:

"@openzeppelin/contracts": "4.5.0"

Use encode instead of encodePacked for hashig

Use of abi.encodePacked in ConnextMessage is safe, but unnecessary and not recommended. abi.encodePacked can result in hash collisions when used with two dynamic arguments (string/bytes).

There is also discussion of removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Affected source code:

• TimelockConfig.sol#L77

There's no need to set default values for variables.

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• InfinityOrderBookComplication.sol#L76-L82
• InfinityOrderBookComplication.sol#L197-L199
• core/InfinityOrderBookComplication.sol#L216
• InfinityOrderBookComplication.sol#L244-L247
• InfinityOrderBookComplication.sol#L289-L291
• InfinityOrderBookComplication.sol#L318-L320
• InfinityExchange.sol#L148
• InfinityExchange.sol#L200
• InfinityExchange.sol#L219
• InfinityExchange.sol#L272
• InfinityExchange.sol#L308
• InfinityExchange.sol#L349
• InfinityExchange.sol#L393
• InfinityExchange.sol#L1048
• InfinityExchange.sol#L1086
• InfinityExchange.sol#L1109
• InfinityExchange.sol#L1190
• InfinityExchange.sol#L1206

Reduce math operations

Use scientific notation (e.g. 10E18) rather than exponentiation (e.g. 10**18)

1_000_000 * (10**decimals()) => 1_000_000 ether (check before)

• MockERC20.sol#L7

10**18:

• InfinityStaker.sol#L237

Use constant for uint256 PRECISION = 10**4;:

• InfinityOrderBookComplication.sol#L338
• InfinityExchange.sol#L1161

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Affected contracts:

• SignatureChecker
• TimelockConfig
• InfinityToken.sol
• InfinityStaker.sol
• InfinityExchange.sol

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• MockERC721.sol#L11

Wrong Visibility

The following methods are public, it's better to use private:

• InfinityToken.sol#L25-L28
• TimelockConfig.sol#L9-L10

Logic change

PendingChange struct is not required, it's cheaper to avoid it changing TimelockConfig.sol#L106-L114 to:

function getPendingByIndex(uint256 index) external view returns (bytes32 configId, PendingChangeData memory pendingRequest) {
    configId = _pendingSet.at(index);
    pendingRequest = _pending[configId]; // Parse the return is the same, but the return it's cheaper
}

function getPending(bytes32 configId) external view returns (PendingChangeData memory pendingRequest) {
    require(_pendingSet.contains(configId), 'not pending');
    return _pending[configId]; // Not need to return configId, because it's the provided argument
}

Move the totalVested line:

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

Change the logic from InfinityStaker.sol#L213-L223 to:

if (totalPower < BRONZE_STAKE_THRESHOLD) return StakeLevel.NONE;    // The current code use <= but seems wrong.
if (totalPower < SILVER_STAKE_THRESHOLD) return StakeLevel.BRONZE;  // The current code use <= but seems wrong.
if (totalPower < GOLD_STAKE_THRESHOLD) return StakeLevel.SILVER;    // The current code use <= but seems wrong.
if (totalPower < PLATINUM_STAKE_THRESHOLD) return StakeLevel.GOLD;  // The current code use <= but seems wrong.
return StakeLevel.PLATINUM;

Speed up false returns changing InfinityOrderBookComplication.sol#L26-L57:

  function canExecMatchOneToOne(OrderTypes.MakerOrder calldata makerOrder1, OrderTypes.MakerOrder calldata makerOrder2)
    external
    view
    override
    returns (bool, uint256)
  {
    bool numItemsValid = makerOrder2.constraints[0] == makerOrder1.constraints[0] &&
      makerOrder2.constraints[0] == 1 &&
      makerOrder2.nfts.length == 1 &&
      makerOrder2.nfts[0].tokens.length == 1 &&
      makerOrder1.nfts.length == 1 &&
      makerOrder1.nfts[0].tokens.length == 1;
+   if (!numItemsValid) return false;
    bool _isTimeValid = makerOrder2.constraints[3] <= block.timestamp &&
      makerOrder2.constraints[4] >= block.timestamp &&
      makerOrder1.constraints[3] <= block.timestamp &&
      makerOrder1.constraints[4] >= block.timestamp;
+   if (!_isTimeValid) return false;
-   bool _isPriceValid = false;
    uint256 makerOrder1Price = _getCurrentPrice(makerOrder1);
    uint256 makerOrder2Price = _getCurrentPrice(makerOrder2);
    uint256 execPrice;
    if (makerOrder1.isSellOrder) {
-     _isPriceValid = makerOrder2Price >= makerOrder1Price;
+     if (makerOrder2Price < makerOrder1Price) return false;
      execPrice = makerOrder1Price;
    } else {
-     _isPriceValid = makerOrder1Price >= makerOrder2Price;
+     if (makerOrder1Price < makerOrder2Price) return false;
      execPrice = makerOrder2Price;
    }
    return (
-     numItemsValid && _isTimeValid && doItemsIntersect(makerOrder1.nfts, makerOrder2.nfts) && _isPriceValid,
+     doItemsIntersect(makerOrder1.nfts, makerOrder2.nfts),
      execPrice
    );
  }

Gas saving using immutable.

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

Affected source code:

• InfinityStaker.sol#L50

Delete optimization

Use delete instead of set to default value (false or 0)

Affected source code:

• InfinityStaker.sol#L105
• InfinityStaker.sol#L299-L312
• InfinityStaker.sol#L330-L339

Use storage pointers or memory

Use storage keyword for save gas in order to cache a storage pointer.

The recurring use of the same register reading from storage is a task that can be easily optimized by using storage or memory, a clear example is access to userstakedAmounts[user] but it can be seen throughout all contracts.

Affected source code:

userstakedAmounts[msg.sender]:

• InfinityStaker.sol#L100

userstakedAmounts[user]:

• InfinityStaker.sol#L156
• InfinityStaker.sol#L182-L185
• InfinityStaker.sol#L234-L237
• InfinityStaker.sol#L247-L250
• InfinityStaker.sol#L299-L323