Infinity NFT Marketplace contest - BowTiedWardens's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1266

Vulnerability details

Submission

Issue: The protocol fee can be set by the owner without input validation.

Consequences: Arbitrarily high fees up to 100% of the sale price can be inflicted on users.

Mitigation: Add input validation restricting _protocolFeeBps <= 1000 (10%), or other suitable upper bound.

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1260 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L231 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L739 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L787 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L893

Vulnerability details

Submission

Issue: The WETH_TRANSFER_GAS_UNITS parameter can be set arbitrarily by the owner.

Consequence: An unexpected amount of WETH can be taken from a buyer, upper bounded by type(uint32).max * tx.gasprice or the exchange's approval of WETH for that user.

Proof of concept:

• updateWethTransferGas is accidentally or maliciously called with a high value
• Any valid order match is executed
• In case of malicious protocol action, tx.gasprice could be set very high to extract further value
• The gas cost reimbursement function will transfer an exorbitant amount of WETH from the buyer

Mitigation: Implement timelocking of the updateWethTransferGas function at the owner address. Internally and externally document this behavior.

Overview

Table of Contents

• Low Risk Issues
  • 1. Missing address(0) checks
  • 2. Prevent accidentally burning tokens
  • 3. Add a timelock to critical functions
  • 4. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
  • 5. Events not indexed
  • 6. Use a constant instead of duplicating the same string
  • 7. Use a 2-step ownership transfer pattern
• Non-Critical Issues
  • 1. Typos
  • 2. Use scientific notation for readability reasons
  • 3. Use string.concat() or bytes.concat()
  • 4. Adding a return statement when the function defines a named return variable, is redundant

Low Risk Issues

1. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

• core/InfinityExchange.sol:

55:  address public immutable WETH;
...
104:   constructor(address _WETH, address _matchExecutor) {
...
+ 115:     require(_WETH != address(0));
115:     WETH = _WETH;
...
117:   }

2. Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

File: InfinityExchange.sol
1220:   function rescueTokens(
1221:     address destination,
1222:     address currency,
1223:     uint256 amount
1224:   ) external onlyOwner {
+ 1225:     require(destination != address(0));
1225:     IERC20(currency).safeTransfer(destination, amount);
1226:   }

3. Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Consider adding a timelock to:

core/InfinityExchange.sol:1266:  function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {

4. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Similar issue in the past: here

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

core/InfinityExchange.sol:1177:          keccak256(abi.encodePacked(order.constraints)),
core/InfinityExchange.sol:1179:          keccak256(abi.encodePacked(order.execParams)),
core/InfinityExchange.sol:1197:    bytes32 nftsHash = keccak256(abi.encodePacked(hashes));
core/InfinityExchange.sol:1213:    bytes32 tokensHash = keccak256(abi.encodePacked(hashes));
libs/SignatureChecker.sol:61:    bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, orderHash));
token/TimelockConfig.sol:77:    return keccak256(abi.encodePacked(name));

5. Events not indexed

core/InfinityExchange.sol:80:  event CancelAllOrders(address user, uint256 newMinNonce);
core/InfinityExchange.sol:81:  event CancelMultipleOrders(address user, uint256[] orderNonces);
core/InfinityExchange.sol:85:  event MatchOrderFulfilled(
core/InfinityExchange.sol:95:  event TakeOrderFulfilled(
token/TimelockConfig.sol:34:  event ChangeRequested(bytes32 configId, uint256 value);
token/TimelockConfig.sol:35:  event ChangeConfirmed(bytes32 configId, uint256 value);
token/TimelockConfig.sol:36:  event ChangeCanceled(bytes32 configId, uint256 value);

6. Use a constant instead of duplicating the same string

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

core/InfinityExchange.sol:155:      require(canExec, 'cannot execute');
core/InfinityExchange.sol:187:      'cannot execute'
core/InfinityExchange.sol:279:      require(executionValid, 'cannot execute');

core/InfinityExchange.sol:150:      require(_complications.contains(makerOrders1[i].execParams[0]), 'invalid complication');
core/InfinityExchange.sol:184:    require(_complications.contains(makerOrder.execParams[0]), 'invalid complication');

core/InfinityExchange.sol:190:    require(isOrderValid(makerOrder, makerOrderHash), 'invalid maker order');
core/InfinityExchange.sol:310:      require(isOrderValid(makerOrders[i], makerOrderHash), 'invalid maker order');

core/InfinityExchange.sol:139:    require(numMakerOrders == makerOrders2.length, 'mismatched lengths');
core/InfinityExchange.sol:264:    require(numSells == buys.length && numSells == constructs.length, 'mismatched lengths');
core/InfinityExchange.sol:342:    require(ordersLength == takerNfts.length, 'mismatched lengths');

core/InfinityExchange.sol:380:    require(minNonce > userMinOrderNonce[msg.sender], 'nonce too low');
core/InfinityExchange.sol:394:      require(orderNonces[i] >= userMinOrderNonce[msg.sender], 'nonce too low');

core/InfinityExchange.sol:587:    require(verifyMatchOneToOneOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified');
core/InfinityExchange.sol:621:    require(verifyMatchOneToManyOrders(buyOrderHash, false, sell, buy), 'order not verified');
core/InfinityExchange.sol:649:    require(verifyMatchOneToManyOrders(sellOrderHash, true, sell, buy), 'order not verified');
core/InfinityExchange.sol:684:    require(verifyMatchOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified');
core/InfinityExchange.sol:949:    require(makerOrderValid && executionValid, 'order not verified');

staking/InfinityStaker.sol:68:    require(amount != 0, 'stake amount cant be 0');
staking/InfinityStaker.sol:117:    require(amount != 0, 'stake amount cant be 0');

7. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

core/InfinityExchange.sol:5:import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol';
core/InfinityExchange.sol:50:contract InfinityExchange is ReentrancyGuard, Ownable {

Non-Critical Issues

1. Typos

• adress

core/InfinityExchange.sol:58:  /// @dev This is the adress that is used to send auto sniped orders for execution on chain

• Storate

core/InfinityExchange.sol:77:  /// @dev Storate variable that keeps track of valid currencies (tokens)

• ordes

libs/OrderTypes.sol:39:    ///@dev additional parameters like traits for trait orders, private sale buyer for OTC ordes etc

• updateWethTranferGas vs updateWethTransferGas

core/InfinityExchange.sol:1260:  function updateWethTranferGas(uint32 _wethTransferGasUnits) external onlyOwner {

• mesage

libs/SignatureChecker.sol:14:   * @param hashed the hash containing the signed mesage

• paramer

libs/SignatureChecker.sol:48:   * @param domainSeparator paramer to prevent signature being executed in other chains and environments

2. Use scientific notation for readability reasons

100000 should be changed to 1e5 1000000 should be changed to 1e6 20000 should be changed to 2e5 50000 should be changed to 5e5

core/InfinityExchange.sol:61:  uint32 public WETH_TRANSFER_GAS_UNITS = 50000;
core/InfinityExchange.sol:201:        // 20000 for the SSTORE op that updates maker nonce status from zero to a non zero status
core/InfinityExchange.sol:202:        uint256 startGasPerOrder = gasleft() + ((startGas + 20000 - gasleft()) / ordersLength);
core/InfinityExchange.sol:381:    require(minNonce < userMinOrderNonce[msg.sender] + 1000000, 'too many');
core/InfinityExchange.sol:725:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
core/InfinityExchange.sol:775:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
core/InfinityExchange.sol:819:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
core/InfinityExchange.sol:873:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
core/InfinityExchange.sol:1135:    uint256 protocolFee = (PROTOCOL_FEE_BPS * amount) / 10000;
staking/InfinityStaker.sol:35:  uint16 public GOLD_STAKE_THRESHOLD = 10000;
staking/InfinityStaker.sol:36:  uint16 public PLATINUM_STAKE_THRESHOLD = 20000;

3. Use string.concat() or bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>)) Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>))

Here, the version is 0.8.14:

core/InfinityExchange.sol:1177:          keccak256(abi.encodePacked(order.constraints)),
core/InfinityExchange.sol:1179:          keccak256(abi.encodePacked(order.execParams)),
core/InfinityExchange.sol:1197:    bytes32 nftsHash = keccak256(abi.encodePacked(hashes));
core/InfinityExchange.sol:1213:    bytes32 tokensHash = keccak256(abi.encodePacked(hashes));
libs/SignatureChecker.sol:61:    bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, orderHash));
libs/SignatureChecker.sol:65:      return IERC1271(signer).isValidSignature(digest, abi.encodePacked(r, s, v)) == 0x1626ba7e;
token/TimelockConfig.sol:77:    return keccak256(abi.encodePacked(name));

4. Adding a return statement when the function defines a named return variable, is redundant

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Affected code:

contracts/token/InfinityToken.sol:
  113:   function getAdmin() public view returns (address admin) {
  114:     return address(uint160(TimelockConfig.getConfig(TimelockConfig.ADMIN).value));

  117:   function getTimelock() public view returns (uint256 timelock) {
  118:     return TimelockConfig.getConfig(TimelockConfig.TIMELOCK).value;

  121:   function getInflation() public view returns (uint256 inflation) {
  122:     return TimelockConfig.getConfig(EPOCH_INFLATION).value;

  125:   function getCliff() public view returns (uint256 cliff) {
  126:     return TimelockConfig.getConfig(EPOCH_CLIFF).value;

  129:   function getMaxEpochs() public view returns (uint256 totalEpochs) {
  130:     return TimelockConfig.getConfig(MAX_EPOCHS).value;

  133:   function getEpochDuration() public view returns (uint256 epochDuration) {
  134:     return TimelockConfig.getConfig(EPOCH_DURATION).value;

contracts/token/TimelockConfig.sol:
   76:   function calculateConfigId(string memory name) external pure returns (bytes32 configId) {
   77:     return keccak256(abi.encodePacked(name));

   80:   function isConfig(bytes32 configId) external view returns (bool status) {
   81:     return _configSet.contains(configId);

   84:   function getConfigCount() external view returns (uint256 count) {
   85:     return _configSet.length();

   88:   function getConfigByIndex(uint256 index) external view returns (Config memory config) {
   90:     return Config(configId, _config[configId]);
   
   93:   function getConfig(bytes32 configId) public view returns (Config memory config) {
   95:     return Config(configId, _config[configId]);

   98:   function isPending(bytes32 configId) public view returns (bool status) {
   99:     return _pendingSet.contains(configId);

  102:   function getPendingCount() external view returns (uint256 count) {
  103:     return _pendingSet.length();

  106:   function getPendingByIndex(uint256 index) external view returns (PendingChange memory pendingRequest) {
  108:     return PendingChange(configId, _pending[configId].value, _pending[configId].timestamp);

  111:   function getPending(bytes32 configId) external view returns (PendingChange memory pendingRequest) {
  113:     return PendingChange(configId, _pending[configId].value, _pending[configId].timestamp);

Overview

Table of Contents:

• 1. Unchecking arithmetics operations that can't underflow/overflow
• 2. >= is cheaper than > (and <= cheaper than <)
• 3. Splitting require() statements that use && saves gas
• 4. Using private rather than public for constants saves gas
• 5. It costs more gas to initialize variables with their default value than letting the default value be applied
• 6. Some variables should be immutable
• 7. Use Custom Errors instead of Revert Strings to save Gas
• 8. Functions guaranteed to revert when called by normal users can be marked payable
• 9. Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

1. Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Consider wrapping with an unchecked block here:

core/InfinityExchange.sol:726:    uint256 remainingAmount = execPrice - protocolFee;
core/InfinityExchange.sol:776:    uint256 remainingAmount = execPrice - protocolFee;
core/InfinityExchange.sol:820:    uint256 remainingAmount = execPrice - protocolFee;
core/InfinityExchange.sol:874:    uint256 remainingAmount = execPrice - protocolFee;
core/InfinityExchange.sol:1136:    uint256 remainingAmount = amount - protocolFee;
core/InfinityExchange.sol:1156:    uint256 priceDiff = startPrice > endPrice ? startPrice - endPrice : endPrice - startPrice;
core/InfinityExchange.sol:1164:    return startPrice > endPrice ? startPrice - priceDiff : startPrice + priceDiff;
staking/InfinityStaker.sol:301:      amount = amount - noVesting;
staking/InfinityStaker.sol:305:        amount = amount - vestedThreeMonths;
staking/InfinityStaker.sol:309:          amount = amount - vestedSixMonths;
token/InfinityToken.sol:65:    uint256 epochsPassedSinceLastAdvance = (block.timestamp - previousEpochTimestamp) / getEpochDuration();

2. >= is cheaper than > (and <= cheaper than <)

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas). This also holds true between <= and <.

Consider replacing strict inequalities with non-strict ones to save some gas here:

core/InfinityExchange.sol:1156:    uint256 priceDiff = startPrice > endPrice ? startPrice - endPrice : endPrice - startPrice;
core/InfinityExchange.sol:1162:    uint256 portionBps = elapsedTime > duration ? PRECISION : ((elapsedTime * PRECISION) / duration);
core/InfinityExchange.sol:1164:    return startPrice > endPrice ? startPrice - priceDiff : startPrice + priceDiff;
core/InfinityOrderBookComplication.sol:333:    uint256 priceDiff = startPrice > endPrice ? startPrice - endPrice : endPrice - startPrice;
core/InfinityOrderBookComplication.sol:339:    uint256 portionBps = elapsedTime > duration ? PRECISION : ((elapsedTime * PRECISION) / duration);
core/InfinityOrderBookComplication.sol:341:    return startPrice > endPrice ? startPrice - priceDiff : startPrice + priceDiff;

3. Splitting require() statements that use && saves gas

If you're using the Optimizer at 200, instead of using the && operator in a single require statement to check multiple conditions, Consider using multiple require statements with 1 condition per require statement:

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

Please, note that this might not hold true at a higher number of runs for the Optimizer (10k). However, it indeed is true at 200.

4. Using private rather than public for constants saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

token/InfinityToken.sol:25:  bytes32 public constant EPOCH_INFLATION = keccak256('Inflation');
token/InfinityToken.sol:26:  bytes32 public constant EPOCH_DURATION = keccak256('EpochDuration');
token/InfinityToken.sol:27:  bytes32 public constant EPOCH_CLIFF = keccak256('Cliff');
token/InfinityToken.sol:28:  bytes32 public constant MAX_EPOCHS = keccak256('MaxEpochs');
token/TimelockConfig.sol:9:  bytes32 public constant ADMIN = keccak256('Admin');
token/TimelockConfig.sol:10:  bytes32 public constant TIMELOCK = keccak256('Timelock'); 

5. It costs more gas to initialize variables with their default value than letting the default value be applied

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

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

Consider removing explicit initializations for default values.

6. Some variables should be immutable

These variables are only set in the constructor and are never edited after that:

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

Consider marking them as immutable, as it would avoid the expensive storage-writing operation (around 20 000 gas)

7. Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

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: https://blog.soliditylang.org/2021/04/21/custom-errors/:

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).

Consider replacing all revert strings with custom errors in the solution.

core/InfinityExchange.sol:138:    require(msg.sender == MATCH_EXECUTOR, 'OME');
core/InfinityExchange.sol:139:    require(numMakerOrders == makerOrders2.length, 'mismatched lengths');
core/InfinityExchange.sol:150:      require(_complications.contains(makerOrders1[i].execParams[0]), 'invalid complication');
core/InfinityExchange.sol:155:      require(canExec, 'cannot execute');
core/InfinityExchange.sol:183:    require(msg.sender == MATCH_EXECUTOR, 'OME');
core/InfinityExchange.sol:184:    require(_complications.contains(makerOrder.execParams[0]), 'invalid complication');
core/InfinityExchange.sol:185:    require(
core/InfinityExchange.sol:190:    require(isOrderValid(makerOrder, makerOrderHash), 'invalid maker order');
core/InfinityExchange.sol:263:    require(msg.sender == MATCH_EXECUTOR, 'OME');
core/InfinityExchange.sol:264:    require(numSells == buys.length && numSells == constructs.length, 'mismatched lengths');
core/InfinityExchange.sol:279:      require(executionValid, 'cannot execute');
core/InfinityExchange.sol:306:      require(currency != address(0), 'offers only in ERC20');
core/InfinityExchange.sol:310:      require(isOrderValid(makerOrders[i], makerOrderHash), 'invalid maker order');
core/InfinityExchange.sol:313:      require(isTimeValid, 'invalid time');
core/InfinityExchange.sol:314:      require(currency == makerOrders[i].execParams[1], 'cannot mix currencies');
core/InfinityExchange.sol:315:      require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides');
core/InfinityExchange.sol:326:      require(msg.value >= totalPrice, 'invalid total price');
core/InfinityExchange.sol:342:    require(ordersLength == takerNfts.length, 'mismatched lengths');
core/InfinityExchange.sol:347:      require(currency != address(0), 'offers only in ERC20');
core/InfinityExchange.sol:350:      require(currency == makerOrders[i].execParams[1], 'cannot mix currencies');
core/InfinityExchange.sol:351:      require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides');
core/InfinityExchange.sol:362:      require(msg.value >= totalPrice, 'invalid total price');
core/InfinityExchange.sol:380:    require(minNonce > userMinOrderNonce[msg.sender], 'nonce too low');
core/InfinityExchange.sol:381:    require(minNonce < userMinOrderNonce[msg.sender] + 1000000, 'too many');
core/InfinityExchange.sol:392:    require(numNonces > 0, 'cannot be empty');
core/InfinityExchange.sol:394:      require(orderNonces[i] >= userMinOrderNonce[msg.sender], 'nonce too low');
core/InfinityExchange.sol:395:      require(!isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]], 'nonce already executed or cancelled');
core/InfinityExchange.sol:587:    require(verifyMatchOneToOneOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified');
core/InfinityExchange.sol:621:    require(verifyMatchOneToManyOrders(buyOrderHash, false, sell, buy), 'order not verified');
core/InfinityExchange.sol:649:    require(verifyMatchOneToManyOrders(sellOrderHash, true, sell, buy), 'order not verified');
core/InfinityExchange.sol:684:    require(verifyMatchOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified');
core/InfinityExchange.sol:949:    require(makerOrderValid && executionValid, 'order not verified');
core/InfinityExchange.sol:1141:      require(sent, 'failed to send ether to seller');
core/InfinityExchange.sol:1231:    require(sent, 'failed');
core/InfinityOrderBookComplication.sol:255:          require(tokenIdsIntersect, 'tokenIds dont intersect');
libs/SignatureChecker.sol:27:    require(
libs/SignatureChecker.sol:32:    require(v == 27 || v == 28, 'Signature: Invalid v parameter');
libs/SignatureChecker.sol:36:    require(signer != address(0), 'Signature: Invalid signer');
staking/InfinityStaker.sol:68:    require(amount != 0, 'stake amount cant be 0');
staking/InfinityStaker.sol:69:    require(IERC20(INFINITY_TOKEN).balanceOf(msg.sender) >= amount, 'insufficient balance to stake');
staking/InfinityStaker.sol:91:    require(amount != 0, 'amount cant be 0');
staking/InfinityStaker.sol:92:    require(
staking/InfinityStaker.sol:96:    require(newDuration > oldDuration, 'new duration must be greater than old duration');
staking/InfinityStaker.sol:117:    require(amount != 0, 'stake amount cant be 0');
staking/InfinityStaker.sol:123:    require(totalVested >= amount, 'insufficient balance to unstake');
staking/InfinityStaker.sol:193:    require(totalStaked >= 0, 'nothing staked to rage quit');
staking/InfinityStaker.sol:347:    require(sent, 'Failed to send Ether');
token/InfinityToken.sol:61:    require(currentEpoch < getMaxEpochs(), 'no epochs left');
token/InfinityToken.sol:62:    require(block.timestamp >= currentEpochTimestamp + getCliff(), 'cliff not passed');
token/InfinityToken.sol:63:    require(block.timestamp >= previousEpochTimestamp + getEpochDuration(), 'not ready to advance');
token/TimelockConfig.sol:39:    require(msg.sender == address(uint160(_config[ADMIN])), 'only admin');
token/TimelockConfig.sol:51:    require(isPending(configId), 'No pending configId found');
token/TimelockConfig.sol:52:    require(block.timestamp >= _pending[configId].timestamp + _config[TIMELOCK], 'too early');
token/TimelockConfig.sol:94:    require(_configSet.contains(configId), 'not config');
token/TimelockConfig.sol:112:    require(_pendingSet.contains(configId), 'not pending');
token/TimelockConfig.sol:119:    require(_pendingSet.add(configId), 'request already exists');
token/TimelockConfig.sol:127:    require(_pendingSet.remove(configId), 'no pending request');

8. Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

core/InfinityExchange.sol:1235:  function addCurrency(address _currency) external onlyOwner {
core/InfinityExchange.sol:1240:  function addComplication(address _complication) external onlyOwner {
core/InfinityExchange.sol:1245:  function removeCurrency(address _currency) external onlyOwner {
core/InfinityExchange.sol:1250:  function removeComplication(address _complication) external onlyOwner {
core/InfinityExchange.sol:1255:  function updateMatchExecutor(address _matchExecutor) external onlyOwner {
core/InfinityExchange.sol:1260:  function updateWethTranferGas(uint32 _wethTransferGasUnits) external onlyOwner {
core/InfinityExchange.sol:1266:  function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {
staking/InfinityStaker.sol:351:  function updateStakeLevelThreshold(StakeLevel stakeLevel, uint16 threshold) external onlyOwner {
staking/InfinityStaker.sol:375:  function updateInfinityTreasury(address _infinityTreasury) external onlyOwner {
token/TimelockConfig.sol:118:  function requestChange(bytes32 configId, uint256 value) external onlyAdmin {
token/TimelockConfig.sol:126:  function cancelChange(bytes32 configId) external onlyAdmin {

9. Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

core/InfinityExchange.sol:1161:    uint256 PRECISION = 10**4; // precision for division; similar to bps
core/InfinityOrderBookComplication.sol:338:    uint256 PRECISION = 10**4; // precision for division; similar to bps
staking/InfinityStaker.sol:237:        (userstakedAmounts[user][Duration.TWELVE_MONTHS].amount * 4)) / (10**18);