Infinity NFT Marketplace contest - 0x29A's results

The world's most advanced NFT marketplace.

General Information

Platform: Code4rena

Start Date: 14/06/2022

Pot Size: $50,000 USDC

Total HM: 19

Participants: 99

Period: 5 days

Judge: HardlyDifficult

Total Solo HM: 4

Id: 136

League: ETH

Infinity NFT Marketplace

Findings Distribution

Researcher Performance

Rank: 18/99

Findings: 6

Award: $637.55

🌟 Selected for report: 0

🚀 Solo Findings: 0

Awards

11.084 USDC - $11.08

Labels

bug
duplicate
3 (High Risk)
sponsor confirmed

External Links

Lines of code

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

Vulnerability details

Impact

The rescueETH function does not work as expected and the ETH balance of the contract, received from takeMultipleOneOrders and takeOrders (rare but also from fallback and receive), gets stuck on contract

Proof of Concept

With the takeMultipleOneOrders and takeOrders (also by mistake the fallback and receive) functions the contract receive ETH to pay the exchange fees, the owner withdraw these fees with the rescueETH but this function send the msg.value, what was sent in the transaction, to the destination and this it's not the balance of the contract

The fallback and receive functions worst this scenario because available the contract to receive ETH

Tools Used

Manual revision

I recommend:

  • Remove the fallback, receive the contract should not receive ETH
  • Modify the rescueETH function:
event RescueETH(address destination, uint256 amount); // @audit optional, you can add the msg.sender but always will be the owner

/// @dev Used for rescuing exchange fees paid to the contract in ETH
function rescueETH(address payable destination) external onlyOwner {
    require(destination != address(0), "The destination can't be the address 0"); // @audit optional
    uint256 balance = address(this).balance;
    (bool sent, ) = destination.call{value: balance}('');
    require(sent, 'Failed to send Ether');
    emit RescueETH(destination, balance); // @audit optional
}    

Note: A contract can use selfdestruct to send ETH to the contract, but why would someone do this?

#0 - nneverlander

2022-06-20T14:39:37Z

#2 - HardlyDifficult

2022-07-09T16:53:16Z

Findings Information

🌟 Selected for report: k

Also found by: 0x29A, 0xf15ers, 0xsanson, PwnedNoMore, antonttc, hyh, zzzitron

Labels

bug
duplicate
3 (High Risk)
sponsor confirmed

Awards

276.9248 USDC - $276.92

External Links

Lines of code

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

Vulnerability details

Impact

If a user sells a NFT that doesn't respect the EIP165 (example Cryptokitties contract), after the sell occurs seller will receive the funds, but buyer will not recive the NFT.

External functions affects:

Proof of Concept

  1. Alice put his CryptoKitti to sell, with a low price
  2. Kate see this offer and he take it
  3. The exchange attempts to transfer CryptoKitti to Kate, but the Cryptokitties contract don't support the interface 0x80ac58cd (or interface 0xd9b67a26), _transferNFTs is not rolled back and the transaction continues(see L1069 goes to the else path)
  4. Finally the exchange successfully transfer the ERC20/ETH to Alice
  5. Kate would not get the kittie and Alice will get the funds

Tools used

Manual revision

The best aproach is add a new parameter in the TokenInfo struct(library OrderTypes) to indentifie the type of token

In OrderTypes library:

...
+ enum CollectionType {
+   ERC721,
+   ERC1155
+ }

/**
 * @title OrderTypes
 * @author nneverlander. Twitter @nneverlander
 * @notice This library contains the order types used by the main exchange and complications
 */
library OrderTypes {
  /// @dev the tokenId and numTokens (=1 for ERC721 and >=1 for ERC1155)
  struct TokenInfo {
    uint256 tokenId;
    uint256 numTokens;
+   CollectionType nftType;
  }
...

In InfinityExchange.sol:

...
- import {OrderTypes} from '../libs/OrderTypes.sol';
+ import {CollectionType, OrderTypes} from '../libs/OrderTypes.sol';
...
  /**
   * @notice Transfer NFTs
   * @param from address of the sender
   * @param to address of the recipient
   * @param item item to transfer
   */
  function _transferNFTs(
    address from,
    address to,
    OrderTypes.OrderItem calldata item
  ) internal {
-   if (IERC165(item.collection).supportsInterface(0x80ac58cd)) {
+   if (item.nftType == CollectionType.ERC721) {
      _transferERC721s(from, to, item);
-   } else if (IERC165(item.collection).supportsInterface(0xd9b67a26)) {
+   } else {
      _transferERC1155s(from, to, item);
    }
  }
...

#2 - HardlyDifficult

2022-07-09T19:44:24Z

Findings Information

Labels

bug
duplicate
3 (High Risk)

Awards

84.0967 USDC - $84.10

External Links

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L326 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L362

Vulnerability details

Impact

Ether send by the user cang gets locks when taking order/s

Proof of Concept

If Bob use function takeMultipleOneOrders or takeOrders to buy and sends more ETH that it supposes to remaing ETH will be lost, also if the seller is selling for other token and sends ETH by mistake ETH will be lost.

Tools Used

Manual revision

There are two things that you could do, make a exact match require for the ether send or give user remaing ETH back.

diff --git a/contracts/core/InfinityExchange.sol b/contracts/core/InfinityExchange.sol
index 3639554..59c5f0f 100644
--- a/contracts/core/InfinityExchange.sol
+++ b/contracts/core/InfinityExchange.sol
@@ -323,7 +323,9 @@ contract InfinityExchange is ReentrancyGuard, Ownable {
     // check to ensure that for ETH orders, enough ETH is sent
     // for non ETH orders, IERC20 safeTransferFrom will throw error if insufficient amount is sent
     if (isMakerSeller && currency == address(0)) {
-      require(msg.value >= totalPrice, 'invalid total price');
+      require(msg.value == totalPrice, 'invalid total price');
+    } else {
+      require(msg.value == 0, 'invalid total price');
     }
   }
 
@@ -359,7 +361,9 @@ contract InfinityExchange is ReentrancyGuard, Ownable {
     // check to ensure that for ETH orders, enough ETH is sent
     // for non ETH orders, IERC20 safeTransferFrom will throw error if insufficient amount is sent
     if (isMakerSeller && currency == address(0)) {
-      require(msg.value >= totalPrice, 'invalid total price');
+      require(msg.value == totalPrice, 'invalid total price');
+    } else {
+      require(msg.value == 0, 'invalid total price');
     }
   }

Another alternative could be sending back to the user the reimaing ETH

diff --git a/contracts/core/InfinityExchange.sol b/contracts/core/InfinityExchange.sol
index 3639554..94d5a30 100644
--- a/contracts/core/InfinityExchange.sol
+++ b/contracts/core/InfinityExchange.sol
@@ -324,6 +324,15 @@ contract InfinityExchange is ReentrancyGuard, Ownable {
     // for non ETH orders, IERC20 safeTransferFrom will throw error if insufficient amount is sent
     if (isMakerSeller && currency == address(0)) {
       require(msg.value >= totalPrice, 'invalid total price');
+      if (msg.value > totalPrice) {
+        // transfer remaining amount to buyer
+        (bool sent, ) = msg.sender.call{value: msg.value - totalPrice}('');
+        require(sent, 'failed to send ether to seller');
+      }
+    } else if (msg.value > 0 ) {
+      // transfer remaining amount to buyer
+      (bool sent, ) = msg.sender.call{value: msg.value}('');
+      require(sent, 'failed to send ether to seller');
     }
   }
 
@@ -360,6 +369,15 @@ contract InfinityExchange is ReentrancyGuard, Ownable {
     // for non ETH orders, IERC20 safeTransferFrom will throw error if insufficient amount is sent
     if (isMakerSeller && currency == address(0)) {
       require(msg.value >= totalPrice, 'invalid total price');
+      if (msg.value > totalPrice) {
+        // transfer remaining amount to buyer
+        (bool sent, ) = msg.sender.call{value: msg.value - totalPrice}('');
+        require(sent, 'failed to send ether to seller');
+      }
+    } else if (msg.value > 0 ) {
+      // transfer remaining amount to buyer
+      (bool sent, ) = msg.sender.call{value: msg.value}('');
+      require(sent, 'failed to send ether to seller');
     }
   }

#0 - KenzoAgada

2022-06-21T12:19:45Z

Duplicate of #244

#1 - HardlyDifficult

2022-07-10T12:41:01Z

Findings Information

🌟 Selected for report: shenwilly

Also found by: 0x29A, BowTiedWardens, VAD37, berndartmueller, peritoflores

Labels

bug
duplicate
2 (Med Risk)
disagree with severity
sponsor acknowledged

Awards

136.753 USDC - $136.75

External Links

Lines of code

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

Vulnerability details

Impact

The protocol can steal WETH founds with the refunds gas cost mechanism in the functions matchOneToOneOrders, matchOneToManyOrders and matchOrders This functions can call only by the MATCH_EXECUTOR but we don't know what is this contract/address according the sponsor in discord this code is not public

Proof of Concept

  1. Alice approve WETH to use the exchange, generally this approve it's for max amount to save gas for the future approbes
  2. Alice maker an order to sell/buy
  3. When the MATCH_EXECUTOR send matchOneToManyOrders, with the Alice signature, it send the transaction with a high amount of gas
  4. In the L231 the gasCost will be high
  5. In the L237 or L240 the protocol take the gasCost from Alice to the exchange
  6. The remaining gas of the transaction will return to the sender of the transaction

The best approach its remove the refunds gas cost mechanism, and use other mechanism for charge fees in a easy way, like a fixed amount of X token Other option it's add another mechanism that gives to the signer of the order the max amount of fee in WETH who is willing to pay In OrderTypes library:

...
  struct MakerOrder {
    ///@dev is order sell or buy
    bool isSellOrder;
    ///@dev signer of the order (maker address)
    address signer;
+   ///@dev Maximum WETH to pay gas cost mechanism
+   uint256 maxWETHGasCost;
    ///@dev Constraints array contains the order constraints. Total constraints: 6. In order:
    // numItems - min (for buy orders) / max (for sell orders) number of items in the order
    // start price in wei
    // end price in wei
    // start time in block.timestamp
    // end time in block.timestamp
    // nonce of the order
    uint256[] constraints;
    ///@dev nfts array contains order items where each item is a collection and its tokenIds
    OrderItem[] nfts;
    ///@dev address of complication for trade execution (e.g. InfinityOrderBookComplication), address of the currency (e.g., WETH)
    address[] execParams;
    ///@dev additional parameters like traits for trait orders, private sale buyer for OTC ordes etc
    bytes extraParams;
    ///@dev the order signature uint8 v: parameter (27 or 28), bytes32 r, bytes32 s
    bytes sig;
  }
...

In InfinityExchange.sol add this lines:

In L231:
  uint256 gasCost = (startGas - gasleft() + WETH_TRANSFER_GAS_UNITS) * tx.gasprice;
+ require(gasCost <= makerOrder.maxWETHGasCost, "The gasCost it's to high");
In L739:
  uint256 gasCost = (startGasPerOrder - gasleft() + wethTransferGasUnits) * tx.gasprice;
+ require(gasCost <= buy.maxWETHGasCost, "The gasCost it's to high");
In L787: 
  uint256 gasCost = (startGasPerOrder - gasleft() + wethTransferGasUnits) * tx.gasprice;
+ require(gasCost <= buy.maxWETHGasCost, "The gasCost it's to high");
In L893:
  uint256 gasCost = (startGasPerOrder - gasleft() + wethTransferGasUnits) * tx.gasprice;
+ require(gasCost <= buy.maxWETHGasCost, "The gasCost it's to high");

#0 - KenzoAgada

2022-06-21T12:07:23Z

The calculation in L231 is:

uint256 gasCost = (startGas - gasleft() + WETH_TRANSFER_GAS_UNITS) * tx.gasprice;

As far as I see, this will only transfer from Alice the actual gas cost of the transaction. Not an arbitrary amount. So the issue seems invalid.

#1 - nneverlander

2022-06-22T16:34:54Z

Duplicate

#2 - HardlyDifficult

2022-07-11T22:49:35Z

QA

[Q-01] Use a wider uncheck

In some code blocks you use a lot of uncheckeds scopes, a cleanner pattern would be to use a wider unchecked scope.

InfinityOrderBookComplication.sol

diff --git a/contracts/core/InfinityOrderBookComplication.sol b/contracts/core/InfinityOrderBookComplication.sol
index 17b2b45..dbf4997 100644
--- a/contracts/core/InfinityOrderBookComplication.sol
+++ b/contracts/core/InfinityOrderBookComplication.sol
@@ -242,25 +242,21 @@ contract InfinityOrderBookComplication is IComplication, Ownable {
     }
 
     uint256 numCollsMatched = 0;
-    // check if taker has all items in maker
-    for (uint256 i = 0; i < order2NftsLength; ) {
-      for (uint256 j = 0; j < order1NftsLength; ) {
-        if (order1Nfts[j].collection == order2Nfts[i].collection) {
-          // increment numCollsMatched
-          unchecked {
+    unchecked {
+      // check if taker has all items in maker
+      for (uint256 i = 0; i < order2NftsLength; ) {
+        for (uint256 j = 0; j < order1NftsLength; ) {
+          if (order1Nfts[j].collection == order2Nfts[i].collection) {
+            // increment numCollsMatched
             ++numCollsMatched;
+            // check if tokenIds intersect
+            bool tokenIdsIntersect = doTokenIdsIntersect(order1Nfts[j], order2Nfts[i]);
+            require(tokenIdsIntersect, 'tokenIds dont intersect');
+            // short circuit
+            break;
           }
-          // check if tokenIds intersect
-          bool tokenIdsIntersect = doTokenIdsIntersect(order1Nfts[j], order2Nfts[i]);
-          require(tokenIdsIntersect, 'tokenIds dont intersect');
-          // short circuit
-          break;
-        }
-        unchecked {
           ++j;
         }
-      }
-      unchecked {
         ++i;
       }
     }
@@ -287,23 +283,19 @@ contract InfinityOrderBookComplication is IComplication, Ownable {
       return true;
     }
     uint256 numTokenIdsPerCollMatched = 0;
-    for (uint256 k = 0; k < item2TokensLength; ) {
-      for (uint256 l = 0; l < item1TokensLength; ) {
-        if (
-          item1.tokens[l].tokenId == item2.tokens[k].tokenId && item1.tokens[l].numTokens == item2.tokens[k].numTokens
-        ) {
-          // increment numTokenIdsPerCollMatched
-          unchecked {
+    unchecked {
+      for (uint256 k = 0; k < item2TokensLength; ) {
+        for (uint256 l = 0; l < item1TokensLength; ) {
+          if (
+            item1.tokens[l].tokenId == item2.tokens[k].tokenId && item1.tokens[l].numTokens == item2.tokens[k].numTokens
+          ) {
+            // increment numTokenIdsPerCollMatched
             ++numTokenIdsPerCollMatched;
+            // short circuit
+            break;
           }
-          // short circuit
-          break;
-        }
-        unchecked {
           ++l;
         }
-      }
-      unchecked {
         ++k;
       }
     }

[Q-02] Use a common pattern

On InfinityExchange.sol#L1128 the signature of the function is function _transferFees(address seller, address buyer, uint256 amount, address currency) I recommend you to change it to a more common pattern; function _transferFees(address seller, address buyer, address currency, uint256 amount)

[Q-03] Struct definition is never use

On OrderTypes.sol#L46-L48 the struct TakerOrder is never used

[Q-04] Natspect typo, weth should be uppercase

on InfinityExchange.sol#L859 the comment is;

* @param weth weth address

And should be

* @param weth WETH address

Non-critical

[N-01] UNUSED IMPORT

IStaker.sol

In IStaker.sol, the import {OrderTypes} from '../libs/OrderTypes.sol'; is unused

Out of scope but InfinityToken.sol inherits from IStaker.sol

InfinityOrderBookComplication.sol

Import Ownable on InfinityOrderBookComplication.sol

InfinityOrderBookComplication contract is importing Ownable but no one is using it. My recommendation is to remove it.

diff --git a/contracts/core/InfinityOrderBookComplication.sol b/contracts/core/InfinityOrderBookComplication.sol
index 17b2b45..6e4cf77 100644
--- a/contracts/core/InfinityOrderBookComplication.sol
+++ b/contracts/core/InfinityOrderBookComplication.sol
@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.14;
 
-import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol';
 
 import {OrderTypes} from '../libs/OrderTypes.sol';
 import {IComplication} from '../interfaces/IComplication.sol';
@@ -11,7 +10,7 @@ import {IComplication} from '../interfaces/IComplication.sol';
  * @author nneverlander. Twitter @nneverlander
  * @notice Complication to execute orderbook orders
  */
-contract InfinityOrderBookComplication is IComplication, Ownable {
+contract InfinityOrderBookComplication is IComplication {
   // ======================================================= EXTERNAL FUNCTIONS ==================================================
 
   /**

[N-02] DECLARE AS IERC20

In InfinityStaker.sol:L25, declare INFINITY_TOKEN as IERC20 would avoid it cast to IERC20 when use it and improve code clarity:

- address public INFINITY_TOKEN;
+ IERC20 public INFINITY_TOKEN;

Cast in the constructor, or declare _tokenAddress as IERC20

 constructor(address _tokenAddress, address _infinityTreasury) {
-   INFINITY_TOKEN = _tokenAddress;
+   INFINITY_TOKEN = IERC20(_tokenAddress);
    INFINITY_TREASURY = _infinityTreasury;
  } 

Remove cast on:

L69: 
- require(IERC20(INFINITY_TOKEN).balanceOf(msg.sender) >= amount, 'insufficient balance to stake');
+ require(INFINITY_TOKEN.balanceOf(msg.sender) >= amount, 'insufficient balance to stake');
L74:
- IERC20(INFINITY_TOKEN).safeTransferFrom(msg.sender, address(this), amount);
+ INFINITY_TOKEN.safeTransferFrom(msg.sender, address(this), amount);
L128:
- IERC20(INFINITY_TOKEN).safeTransfer(msg.sender, amount);
+ INFINITY_TOKEN.safeTransfer(msg.sender, amount);
L141:
- IERC20(INFINITY_TOKEN).safeTransfer(msg.sender, totalToUser);
+ INFINITY_TOKEN.safeTransfer(msg.sender, totalToUser);
L142:
- IERC20(INFINITY_TOKEN).safeTransfer(INFINITY_TREASURY, penalty);
+ INFINITY_TOKEN.safeTransfer(INFINITY_TREASURY, penalty);

[N-03] NOT DECLARE VISIBILITY

In TimelockConfig.sol, L28, L29, L31, L32: By default, the visibility of variables in solidity is internal, but it is good practice to declare it.

Out of scope but InfinityToken.sol inherits from TimelockConfig.sol

[N-04] TYPO

In InfinityExchange.sol:

  • L858: * @param weth weth address to * @param weth WETH address
  • L1260: The name of function updateWethTranferGas to updateWethTransferGas

Low Risk

[L-01] EVENTS ARE NOT INDEXED

The emitted events are not indexed, making off-chain scripts such as front-ends of dApps difficult to filter the events efficiently.

Recommended Mitigation Steps: Add the indexed keyword in filter parameters of the events

Instances include:

InfinityExchange.sol:L80-L102:  
- event CancelAllOrders(address user, uint256 newMinNonce);
+ event CancelAllOrders(address indexed user, uint256 newMinNonce);
- event CancelMultipleOrders(address user, uint256[] orderNonces);
+ event CancelMultipleOrders(address indexed user, uint256[] orderNonces);
  event NewWethTransferGasUnits(uint32 wethTransferGasUnits);
  event NewProtocolFee(uint16 protocolFee);

  event MatchOrderFulfilled(
    bytes32 sellOrderHash,
    bytes32 buyOrderHash,
-   address seller,
+   address indexed seller,
-   address buyer,
+   address indexed buyer,
    address complication, // address of the complication that defines the execution
-   address currency, // token address of the transacting currency
+   address indexed currency, // token address of the transacting currency
    uint256 amount // amount spent on the order
  );

  event TakeOrderFulfilled(
    bytes32 orderHash,
-   address seller,
+   address indexed seller,
-   address buyer,
+   address indexed buyer,
    address complication, // address of the complication that defines the execution
-   address currency, // token address of the transacting currency
+   address indexed currency, // token address of the transacting currency
    uint256 amount // amount spent on the order
  );
InfinityToken.sol:L35:  
- event EpochAdvanced(uint256 currentEpoch, uint256 supplyMinted);
+ event EpochAdvanced(uint256 indexed currentEpoch, uint256 supplyMinted);
TimelockConfig.sol:L34-L36:  
- event ChangeRequested(bytes32 configId, uint256 value);
+ event ChangeRequested(bytes32 indexed configId, uint256 value);
- event ChangeConfirmed(bytes32 configId, uint256 value);
+ event ChangeConfirmed(bytes32 indexed configId, uint256 value);
- event ChangeCanceled(bytes32 configId, uint256 value);
+ event ChangeCanceled(bytes32 indexed configId, uint256 value);

Out of scope but InfinityToken.sol inherits from TimelockConfig.sol

[L-02] require NEVER REVERT

In InfinityStaker.sol, L193: This require never reverts the transaction, any number of uint256 data type it's greater or equal to 0, by definition

Remove the equal in therequire comparation

- require(totalStaked >= 0, 'nothing staked to rage quit');
+ require(totalStaked > 0, 'nothing staked to rage quit');

This also save gas, if the user dont have staked balance

[L-02] Unnecesary math operations

Unnecesary multiplication by one and unnecesary exponetiation.

InfinityStaker.sol

diff --git a/contracts/staking/InfinityStaker.sol b/contracts/staking/InfinityStaker.sol
index 9b36cb5..872a254 100644
--- a/contracts/staking/InfinityStaker.sol
+++ b/contracts/staking/InfinityStaker.sol
@@ -231,10 +231,10 @@ contract InfinityStaker is IStaker, Ownable, Pausable, ReentrancyGuard {
    */
   function getUserStakePower(address user) public view override returns (uint256) {
     return
-      ((userstakedAmounts[user][Duration.NONE].amount * 1) +
+      ((userstakedAmounts[user][Duration.NONE].amount) +
         (userstakedAmounts[user][Duration.THREE_MONTHS].amount * 2) +
         (userstakedAmounts[user][Duration.SIX_MONTHS].amount * 3) +
-        (userstakedAmounts[user][Duration.TWELVE_MONTHS].amount * 4)) / (10**18);
+        (userstakedAmounts[user][Duration.TWELVE_MONTHS].amount * 4)) / 1e18;
   }
 
   /**

InfinityExchange.sol

diff --git a/contracts/core/InfinityExchange.sol b/contracts/core/InfinityExchange.sol
index 3639554..d58b965 100644
--- a/contracts/core/InfinityExchange.sol
+++ b/contracts/core/InfinityExchange.sol
@@ -1158,7 +1158,7 @@ contract InfinityExchange is ReentrancyGuard, Ownable {
       return startPrice;
     }
     uint256 elapsedTime = block.timestamp - order.constraints[3];
-    uint256 PRECISION = 10**4; // precision for division; similar to bps
+    uint256 PRECISION = 1e4; // precision for division; similar to bps
     uint256 portionBps = elapsedTime > duration ? PRECISION : ((elapsedTime * PRECISION) / duration);
     priceDiff = (priceDiff * portionBps) / PRECISION;
     return startPrice > endPrice ? startPrice - priceDiff : startPrice + priceDiff;

InfinityOrderBookComplication.sol

diff --git a/contracts/core/InfinityOrderBookComplication.sol b/contracts/core/InfinityOrderBookComplication.sol
index 17b2b45..f1975bf 100644
--- a/contracts/core/InfinityOrderBookComplication.sol
+++ b/contracts/core/InfinityOrderBookComplication.sol
@@ -335,7 +335,7 @@ contract InfinityOrderBookComplication is IComplication, Ownable {
       return startPrice;
     }
     uint256 elapsedTime = block.timestamp - order.constraints[3];
-    uint256 PRECISION = 10**4; // precision for division; similar to bps
+    uint256 PRECISION = 1e4; // precision for division; similar to bps
     uint256 portionBps = elapsedTime > duration ? PRECISION : ((elapsedTime * PRECISION) / duration);
     priceDiff = (priceDiff * portionBps) / PRECISION;
     return startPrice > endPrice ? startPrice - priceDiff : startPrice + priceDiff;

[L-03] No event emmision on critical function

InfinityStaker.sol

InfinityStaker.sol#L351 function updateStakeLevelThreshold should emit event InfinityStaker.sol#L364 function updatePenalties should emit event InfinityStaker.sol#L375 function updateInfinityTreasury should emit event

InfinityExchange.sol

InfinityExchange.sol

Critical admin functions rescueTokens, rescueETH, addCurrency, addComplication, removeCurrency, removeComplication and updateMatchExecutor should emit events

[L-04] Solidity version

You are currently using solidity 0.8.14, best recommendation is to switch to 0.8.15 two major bugs introduced on previus versions were fixes, plus its more gas efficient.

#0 - nneverlander

2022-06-22T16:55:50Z

Thanks

#1 - HardlyDifficult

2022-07-11T21:27:33Z

Gas Report

[G-01] USE immutable

In InfinityStaker.sol:L25, marking INFINITY_TOKEN as immutable (as it never change outside the constructor) would avoid it taking space in the storage:

- address public INFINITY_TOKEN;
+ address public immutable INFINITY_TOKEN;

[G-02] Unnecesary checks on if

Since the conditions are order from lower to high you dont have to double check the lower bounds.

diff --git a/contracts/staking/InfinityStaker.sol b/contracts/staking/InfinityStaker.sol
index 9b36cb5..1533ddb 100644
--- a/contracts/staking/InfinityStaker.sol
+++ b/contracts/staking/InfinityStaker.sol
@@ -212,11 +212,11 @@ contract InfinityStaker is IStaker, Ownable, Pausable, ReentrancyGuard {
 
     if (totalPower <= BRONZE_STAKE_THRESHOLD) {
       return StakeLevel.NONE;
-    } else if (totalPower > BRONZE_STAKE_THRESHOLD && totalPower <= SILVER_STAKE_THRESHOLD) {
+    } else if (totalPower <= SILVER_STAKE_THRESHOLD) {
       return StakeLevel.BRONZE;
-    } else if (totalPower > SILVER_STAKE_THRESHOLD && totalPower <= GOLD_STAKE_THRESHOLD) {
+    } else if (totalPower <= GOLD_STAKE_THRESHOLD) {
       return StakeLevel.SILVER;
-    } else if (totalPower > GOLD_STAKE_THRESHOLD && totalPower <= PLATINUM_STAKE_THRESHOLD) {
+    } else if (totalPower <= PLATINUM_STAKE_THRESHOLD) {
       return StakeLevel.GOLD;
     } else {
       return StakeLevel.PLATINUM;

[G-03] Avoid caching array length for calldata variables

You could save 5 gas if you avoid caching array length of calldata variables on:

InfinityExchange.sol

InfinityExchange.sol#L137 InfinityExchange.sol#L191 InfinityExchange.sol#L262 InfinityExchange.sol#L301 InfinityExchange.sol#L341 InfinityExchange.sol#L391 InfinityExchange.sol#L1047 InfinityExchange.sol#L1085 InfinityExchange.sol#L1106 InfinityExchange.sol#L1188 InfinityExchange.sol#L1204

InfinityOrderBookComplication.sol

InfinityOrderBookComplication.sol#L75 InfinityOrderBookComplication.sol#L81 InfinityOrderBookComplication.sol#L198 InfinityOrderBookComplication.sol#L215 InfinityOrderBookComplication.sol#L237-L238 InfinityOrderBookComplication.sol#L283-L284 InfinityOrderBookComplication.sol#L319

[G-04] Unnecesary require

on InfinityStaker.sol#L69 there is no need to check if contract has enough balance, because safeTransferFrom will fail if contract doesnt have enough balance.

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter