Infinity NFT Marketplace contest - MiloTruck's results

QA Report

Non-Critical Issues

Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommended Mitigation Steps
Block timestamps should not be used for entropy or generating random numbers — i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

Instances where block.timestamp is used:

contracts/token/InfinityToken.sol:
  51:        previousEpochTimestamp = block.timestamp;
  52:        currentEpochTimestamp = block.timestamp;
  62:        require(block.timestamp >= currentEpochTimestamp + getCliff(), 'cliff not passed');
  63:        require(block.timestamp >= previousEpochTimestamp + getEpochDuration(), 'not ready to advance');
  65:        uint256 epochsPassedSinceLastAdvance = (block.timestamp - previousEpochTimestamp) / getEpochDuration();
  73:        previousEpochTimestamp = block.timestamp;

contracts/core/InfinityExchange.sol:
 311:        bool isTimeValid = makerOrders[i].constraints[3] <= block.timestamp &&
 312:        makerOrders[i].constraints[4] >= block.timestamp;
1160:        uint256 elapsedTime = block.timestamp - order.constraints[3];

contracts/core/InfinityOrderBookComplication.sol:
  38:        bool _isTimeValid = makerOrder2.constraints[3] <= block.timestamp &&
  39:        makerOrder2.constraints[4] >= block.timestamp &&
  40:        makerOrder1.constraints[3] <= block.timestamp &&
  41:        makerOrder1.constraints[4] >= block.timestamp;
  91:        manyMakerOrders[i].constraints[3] <= block.timestamp &&
  92:        manyMakerOrders[i].constraints[4] >= block.timestamp;
 102:        makerOrder.constraints[3] <= block.timestamp &&
 103:        makerOrder.constraints[4] >= block.timestamp;
 160:        return (makerOrder.constraints[3] <= block.timestamp &&
 161:        makerOrder.constraints[4] >= block.timestamp &&
 175:        sell.constraints[3] <= block.timestamp &&
 176:        sell.constraints[4] >= block.timestamp &&
 177:        buy.constraints[3] <= block.timestamp &&
 178:        buy.constraints[4] >= block.timestamp;
 337:        uint256 elapsedTime = block.timestamp - order.constraints[3];

contracts/staking/InfinityStaker.sol:
  72:        userstakedAmounts[msg.sender][duration].timestamp = block.timestamp;
 102:        userstakedAmounts[msg.sender][newDuration].timestamp = block.timestamp;
 268:        uint256 secondsSinceStake = block.timestamp - timestamp;

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields:

contracts/core/InfinityExchange.sol:
  85:        event MatchOrderFulfilled(
  86:          bytes32 sellOrderHash,
  87:          bytes32 buyOrderHash,
  88:          address seller,
  89:          address buyer,
  90:          address complication, // address of the complication that defines the execution
  91:          address currency, // token address of the transacting currency
  92:          uint256 amount // amount spent on the order
  93:        );

  95:        event TakeOrderFulfilled(
  96:          bytes32 orderHash,
  97:          address seller,
  98:          address buyer,
  99:          address complication, // address of the complication that defines the execution
 100:          address currency, // token address of the transacting currency
 101:          uint256 amount // amount spent on the order
 102:        );

contracts/staking/InfinityStaker.sol:
  44:        event Staked(address indexed user, uint256 amount, Duration duration);
  45:        event DurationChanged(address indexed user, uint256 amount, Duration oldDuration, Duration newDuration);
  47:        event RageQuit(address indexed user, uint256 totalToUser, uint256 penalty);

Gas Report

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

for (uint256 i = 0; i < length; ++i) {

can be changed to:

for (uint256 i; i < length; ++i) {

Consider declaring the following lines without explicitly setting the index to 0:

contracts/core/InfinityExchange.sol:
 148:        for (uint256 i = 0; i < numMakerOrders; ) {
 200:        for (uint256 i = 0; i < ordersLength; ) {
 219:        for (uint256 i = 0; i < ordersLength; ) {
 272:        for (uint256 i = 0; i < numSells; ) {
 308:        for (uint256 i = 0; i < numMakerOrders; ) {
 349:        for (uint256 i = 0; i < ordersLength; ) {
 393:        for (uint256 i = 0; i < numNonces; ) {
1048:        for (uint256 i = 0; i < numNfts; ) {
1086:        for (uint256 i = 0; i < numTokens; ) {
1109:        for (uint256 i = 0; i < numNfts; ) {
1190:        for (uint256 i = 0; i < numNfts; ) {
1206:        for (uint256 i = 0; i < numTokens; ) {

contracts/core/InfinityOrderBookComplication.sol:
  76:        for (uint256 i = 0; i < ordersLength; ) {
  82:        for (uint256 j = 0; j < nftsLength; ) {
 199:        for (uint256 i = 0; i < nftsLength; ) {
 216:        for (uint256 i = 0; i < nftsLength; ) {
 246:        for (uint256 i = 0; i < order2NftsLength; ) {
 247:        for (uint256 j = 0; j < order1NftsLength; ) {
 290:        for (uint256 k = 0; k < item2TokensLength; ) {
 291:        for (uint256 l = 0; l < item1TokensLength; ) {
 320:        for (uint256 i = 0; i < ordersLength; ) {

Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

Consider changing > 0 to != 0 in these lines:

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

Arithmetics: Use Shift Right/Left instead of Division/Multiplication if possible

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

For example, the following code:

uint256 b = a / 2;
uint256 c = a / 4;
uint256 d = a * 8;

can be changed to:

uint256 b = a >> 1;
uint256 c = a >> 2;
uint256 d = a << 3;

Consider making this change to the following lines:

contracts/staking/InfinityStaker.sol:
 235:        (userstakedAmounts[user][Duration.THREE_MONTHS].amount * 2) +
 237:        (userstakedAmounts[user][Duration.TWELVE_MONTHS].amount * 4)) / (10**18);

Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas.

Consider changing the visibility of the following from public to internal or private:

contracts/token/InfinityToken.sol:
  25:        bytes32 public constant EPOCH_INFLATION = keccak256('Inflation');
  26:        bytes32 public constant EPOCH_DURATION = keccak256('EpochDuration');
  27:        bytes32 public constant EPOCH_CLIFF = keccak256('Cliff');
  28:        bytes32 public constant MAX_EPOCHS = keccak256('MaxEpochs');

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

Consider changing following functions from public to external:

contracts/token/InfinityToken.sol:
 117:        function getTimelock() public view returns (uint256 timelock) {
 125:        function getCliff() public view returns (uint256 cliff) {
 133:        function getEpochDuration() public view returns (uint256 epochDuration) {

contracts/core/InfinityOrderBookComplication.sol:
 192:        function areNumItemsValid(
 193:          OrderTypes.MakerOrder calldata sell,
 194:          OrderTypes.MakerOrder calldata buy,
 195:          OrderTypes.OrderItem[] calldata constructedNfts
 196:        ) public pure returns (bool) {

 209:        function areTakerNumItemsValid(OrderTypes.MakerOrder calldata makerOrder, OrderTypes.OrderItem[] calldata takerItems)
 210:          public
 211:          pure
 212:          returns (bool)
 213:        {

 232:        function doItemsIntersect(OrderTypes.OrderItem[] calldata order1Nfts, OrderTypes.OrderItem[] calldata order2Nfts)
 233:          public
 234:          pure
 235:          returns (bool)
 236:        {

Errors: Reduce the length of error messages (long revert strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

In these instances, consider shortening the revert strings to fit within 32 bytes, or using custom errors:

contracts/core/InfinityExchange.sol:
 395:        require(!isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]], 'nonce already executed or cancelled');

contracts/staking/InfinityStaker.sol:
  92:        require(
  93:            userstakedAmounts[msg.sender][oldDuration].amount >= amount,
  94:            'insufficient staked amount to change duration'
  95:        );

  96:        require(newDuration > oldDuration, 'new duration must be greater than old duration');

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors should be used instead of revert strings due to:

• Cheaper deployment cost
• Lower runtime cost upon revert

Taken from 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 can be defined using of the error statement, both inside or outside of contracts.

Instances where custom errors can be used instead:

contracts/token/InfinityToken.sol:
  61:        require(currentEpoch < getMaxEpochs(), 'no epochs left');
  62:        require(block.timestamp >= currentEpochTimestamp + getCliff(), 'cliff not passed');
  63:        require(block.timestamp >= previousEpochTimestamp + getEpochDuration(), 'not ready to advance');

contracts/core/InfinityExchange.sol:
 138:        require(msg.sender == MATCH_EXECUTOR, 'OME');
 139:        require(numMakerOrders == makerOrders2.length, 'mismatched lengths');
 150:        require(_complications.contains(makerOrders1[i].execParams[0]), 'invalid complication');
 155:        require(canExec, 'cannot execute');
 183:        require(msg.sender == MATCH_EXECUTOR, 'OME');
 184:        require(_complications.contains(makerOrder.execParams[0]), 'invalid complication');
 190:        require(isOrderValid(makerOrder, makerOrderHash), 'invalid maker order');
 263:        require(msg.sender == MATCH_EXECUTOR, 'OME');
 264:        require(numSells == buys.length && numSells == constructs.length, 'mismatched lengths');
 279:        require(executionValid, 'cannot execute');
 306:        require(currency != address(0), 'offers only in ERC20');
 310:        require(isOrderValid(makerOrders[i], makerOrderHash), 'invalid maker order');
 313:        require(isTimeValid, 'invalid time');
 314:        require(currency == makerOrders[i].execParams[1], 'cannot mix currencies');
 315:        require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides');
 326:        require(msg.value >= totalPrice, 'invalid total price');
 342:        require(ordersLength == takerNfts.length, 'mismatched lengths');
 347:        require(currency != address(0), 'offers only in ERC20');
 350:        require(currency == makerOrders[i].execParams[1], 'cannot mix currencies');
 351:        require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides');
 362:        require(msg.value >= totalPrice, 'invalid total price');
 380:        require(minNonce > userMinOrderNonce[msg.sender], 'nonce too low');
 381:        require(minNonce < userMinOrderNonce[msg.sender] + 1000000, 'too many');
 392:        require(numNonces > 0, 'cannot be empty');
 394:        require(orderNonces[i] >= userMinOrderNonce[msg.sender], 'nonce too low');
 395:        require(!isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]], 'nonce already executed or cancelled');
 587:        require(verifyMatchOneToOneOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified');
 621:        require(verifyMatchOneToManyOrders(buyOrderHash, false, sell, buy), 'order not verified');
 649:        require(verifyMatchOneToManyOrders(sellOrderHash, true, sell, buy), 'order not verified');
 684:        require(verifyMatchOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified');
 949:        require(makerOrderValid && executionValid, 'order not verified');
1141:        require(sent, 'failed to send ether to seller');
1231:        require(sent, 'failed');

contracts/core/InfinityOrderBookComplication.sol:
 255:        require(tokenIdsIntersect, 'tokenIds dont intersect');

contracts/staking/InfinityStaker.sol:
  68:        require(amount != 0, 'stake amount cant be 0');
  69:        require(IERC20(INFINITY_TOKEN).balanceOf(msg.sender) >= amount, 'insufficient balance to stake');
  91:        require(amount != 0, 'amount cant be 0');
  96:        require(newDuration > oldDuration, 'new duration must be greater than old duration');
 117:        require(amount != 0, 'stake amount cant be 0');
 123:        require(totalVested >= amount, 'insufficient balance to unstake');
 193:        require(totalStaked >= 0, 'nothing staked to rage quit');
 347:        require(sent, 'Failed to send Ether');

Use modifiers for duplicated access role checks

Instead of using a require statement to check that msg.sender belongs to a certain role (e.g. msg.sender is owner), consider using modifiers. This would help to save gas and improve code clarity.

For example, to check that msg.sender is owner, a modifier can be written as such:

modifier isOwner() {
  require(msg.sender == owner, "error");
  _;
}

Functions can then use isOwner to validate msg.sender, for example:

function setOwner(address _owner) external {
  require(msg.sender == owner, "error");
  // ...
}

can be rewritten to:

function setOwner(address _owner) external isOwner {
  // ...
}

Instances where modifiers can be used include:

contracts/core/InfinityExchange.sol:
 138:        require(msg.sender == MATCH_EXECUTOR, 'OME');
 183:        require(msg.sender == MATCH_EXECUTOR, 'OME');
 263:        require(msg.sender == MATCH_EXECUTOR, 'OME');

Unnecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

• uint: 0
• bool: false
• address: address(0)

Thus, explicitly initializing a variable with its default value costs unnecesary gas. For example, the following code:

bool b = false;
address c = address(0);
uint256 a = 0;

can be changed to:

uint256 a;
bool b;
address c;

Consider declaring the following lines without explicitly setting a value:

contracts/core/InfinityOrderBookComplication.sol:
  42:        bool _isPriceValid = false;
 108:        bool _isPriceValid = false;
 197:        uint256 numConstructedItems = 0;
 214:        uint256 numTakerItems = 0;
 244:        uint256 numCollsMatched = 0;
 289:        uint256 numTokenIdsPerCollMatched = 0;
 318:        uint256 sum = 0;

Unnecessary definition of variables

Some variables are defined even though they are only used once in their respective functions. Not defining these variables can help to reduce gas cost and contract size.

Instances include:

contracts/staking/InfinityStaker.sol:
 261:        uint256 amount = userstakedAmounts[user][duration].amount;

Storage variables should be declared immutable when possible

If a storage variable is assigned only in the constructor, it should be declared as immutable. This would help to reduce gas costs as calls to immutable variables are much cheaper than regular state variables, as seen from the Solidity Docs:

Compared to regular state variables, the gas costs of constant and immutable variables are much lower. Immutable variables are evaluated once at construction time and their value is copied to all the places in the code where they are accessed.

Consider declaring these variables as immutable:

contracts/token/InfinityToken.sol:
  31:        uint256 public currentEpochTimestamp;

contracts/staking/InfinityStaker.sol:
  25:        address public INFINITY_TOKEN;

Variables declared as constant are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232:

Consequences: each usage of a “constant” costs ~100 gas more on each access (it is still a little better than storing the result in storage, but not much). since these are not real constants, they can’t be referenced from a real constant environment (e.g. from assembly, or from another library)

contracts/token/InfinityToken.sol:
  25:        bytes32 public constant EPOCH_INFLATION = keccak256('Inflation');
  26:        bytes32 public constant EPOCH_DURATION = keccak256('EpochDuration');
  27:        bytes32 public constant EPOCH_CLIFF = keccak256('Cliff');
  28:        bytes32 public constant MAX_EPOCHS = keccak256('MaxEpochs');

Change these expressions from constant to immutable and implement the calculation in the constructor. Alternatively, hardcode these values in the constants and add a comment to say how the value was calculated.