Infinity NFT Marketplace contest - _Adam's results

QA01 Require Statement

InfinityStaker.sol#L193 The require statement in InfinityStaker.getRageQuitAmounts is checking that user has something staked to rage quit but is currently checking totalStaked >= 0, so will pass even if user has nothing staked. Recommend changing to either:

require(totalStaked > 0, 'nothing staked to rage quit');
or
require(totalStaked != 0, 'nothing staked to rage quit'); 
(will be slightly cheaper in gas costs)

QA02 No Cap on Exhange Fees/Staking Penalties

Users may be hesitant to use a protocol that can set the fee to 100% of profits at any time, recommend adding a maxFee that the protocol can't go past. InfinityExchange.sol#L1267

QA03 Incomplete Natspec

InfinityExchange.sol#L456 - missing @param verifySellOrder InfinityExchange.sol#L512 - missing @return InfinityExchange.sol#L668 - missing @param execPrice

QA04 Return Statements and Named Return Values

It is unnecessary to have both named returns and return statements, i recommend being consistent accross contracts and removing the named returns from the following functions.

InfinityToken.sol#L113-L114 InfinityToken.sol#L117-L118 InfinityToken.sol#L121-L122 InfinityToken.sol#L125-L126 InfinityToken.sol#L129-L130 InfinityToken.sol#L133-L134

G01 Custom Errors

As your using a solidity version greater 0.8.4 for most of the contracts you can replace revert strings with custom errors. This will save in deployment costs and runtime costs. I ran a test in remix comparing a revert string vs custom errors and found that replacing a single revert string with a custom error saved 12,404 gas in deployment cost and 86 gas on each function call.

contract Test {
	uint256 a;
	function check() external {
		require(a != 0, "check failed");
	}
}   (Deployment cost: 114,703, Cost on Function call: 23,392)
vs 
contract Test {
	uint256 a;
	error checkFailed();
	function check() external {
		if (a != 0) revert checkFailed();
	}
}   (Deployment cost: 102,299, Cost on Function call: 23,306)

You have 54 revert strings in the following locations that can be replaced with custom errors: InfinityExchange.sol#L138-L139 InfinityExchange.sol#L150 InfinityExchange.sol#L155 InfinityExchange.sol#L183-L190 InfinityExchange.sol#L263-L264 InfinityExchange.sol#L279 InfinityExchange.sol#L306 InfinityExchange.sol#L310 InfinityExchange.sol#L313-L315 InfinityExchange.sol#L326 InfinityExchange.sol#L342 InfinityExchange.sol#L347 InfinityExchange.sol#L350-L351 InfinityExchange.sol#L362 InfinityExchange.sol#L380-L381 InfinityExchange.sol#L392-L395 InfinityExchange.sol#L587 InfinityExchange.sol#L621 InfinityExchange.sol#L649 InfinityExchange.sol#L684 InfinityExchange.sol#L949 InfinityExchange.sol#L1141 InfinityExchange.sol#L1231 InfinityOrderBookComplication.sol#L255 SignatureChecker.sol#L27-L36 InfinityStaker.sol#L68-L69 InfinityStaker.sol#L91-L96 InfinityStaker.sol#L117 InfinityStaker.sol#L123 InfinityStaker.sol#L193 InfinityStaker.sol#L347 InfinityToken.sol#L61-L63 TimelockConfig.sol#L39 TimelockConfig.sol#L51-L52 TimelockConfig.sol#L94 TimelockConfig.sol#L112 TimelockConfig.sol#L119 TimelockConfig.sol#L127

G02 Long Revert Strings

If you opt not to use custom errors keeping revert strings <= 32 bytes in length will save gas. I ran a test in remix and found the savings for a single short revert string vs long string to be 9,377 gas in deployment cost and 18 gas on function call.

contract Test {
	uint256 a;
	function check() external {
		require(a != 0, "short error message"); 
		(Deployment cost: 114,799, Cost on function call: 23,392)	
		vs 
		require(a != 0, "A longer Error Message over 32 bytes in     
        length"); 
		(Deployment cost: 124,176, Cost on function call: 23,410)	
	}
}

I recommend shortenning the following 3 revert strings to < 32 bytes in length: InfinityExchange.sol#L395 Currently 35 bytes long InfinityStaker.sol#L92-L95 Currently 45 bytes long InfinityStaker.sol#L96 Currently 47 bytes long

G03 Uint > 0 Checks in Require Statements

When checking whether a uint is > 0 in a requrie statement you can save a small amount of gas by replacing with != 0. This is only true if optimiser is turned on and in a require statement. I ran a test in remix with optimisation set to 10,000 and found the savings for a single occurance is 632 in deployment cost and 6 gas on each function call.

contract Test {
	uint256 a;
	function check() external {
		require(a > 0); 
		(Deployment cost: 79,763, Cost on function call: 23,305)	
		vs
		require(a != 0); 
		(Deployment cost: 79,331, Cost on function call: 23,299)	
	}
}

Instances where uint != 0 can be used: InfinityExchange.sol#L392 InfinityStaker.sol#L193 (Mentioned in QA report that this require check should be > 0 not >= 0)

G04 Immutable Variables

INFINITY_TOKEN is initialised in the constructor and is never updated anywhere. It can be changed to immutable. InfinityStaker.sol#L25

G05 && in Require Functions

If optimising for running costs over deployment costs you can seperate && in require functions into 2 parts. I ran a basic test in remix and it cost an extra 234 gas to deploy but will save ~9 gas everytime the require function is called.

contract Test {
	uint256 a = 0;
	uint256 b = 1;

	function test() external {
		require(a == 0 && b > a) 
		(Deployment cost: 123,291, Cost on function call: 29,371)
		vs
		require(a == 0);
		require(b > a);
		(Deployment cost: 123,525, Cost on function call: 29,362)
	}
}

Instances where require statements can be split into seperate statements: InfinityExchange.sol#L264 InfinityExchange.sol#L413 InfinityExchange.sol#L949

G06 Minimizing SLOAD's

Whenever using a storage variable more than once in a function you can save ~97 gas per use. (normally 100 gas each use vs 103 gas to SLOAD/MSTORE for the first use and then only 3 gas for any others)

InfinityToken.sol#L61-L66 currentEpoch is used on line 61 and line 66 and can be cached to save 1 SLOAD ~97 gas. previousEpochTimestamp is used on line 63 and line 65 and can be cached to save 1 SLOAD ~97 gas.

InfinityExchange.sol#L197 InfinityExchange.sol#L231 WETH_TRANSFER_GAS_UNITS has already been cached on line 197, recommend using wethTransferGasUnits instead of WETH_TRANSFER_GAS_UNITS on line 231. will save 1 SLOAD ~97 gas