Infinity NFT Marketplace contest - Wayne's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1230 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L346

Vulnerability details

Impact

Detailed description of the impact of this finding.

rescueETH() is a function used to rescue the Ether funds present in the InfinityStaker & InfinityExchange contracts, however, it does not work as expected and sends msg.value (Ether sent by the owner) to the destination.

This will result in a lack of timely access to Ether funds in the event of an emergency, further leading to a hack.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

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

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L346

function rescueETH(address destination) external payable onlyOwner {
    (bool sent, ) = destination.call{value: msg.value}('');
    require(sent, 'failed');
}

Replace the code of the function with

function rescueETH(address destination) external payable onlyOwner {
    (bool sent, ) = destination.call{value: address(this).balance}('');
    require(sent, 'failed');
}

done.

Tools Used

Recommended Mitigation Steps

function rescueETH(address destination) external payable onlyOwner {
    (bool sent, ) = destination.call{value: msg.value}('');
    require(sent, 'failed');
}

Replace the code of the function with

function rescueETH(address destination) external payable onlyOwner {
    (bool sent, ) = destination.call{value: address(this).balance}('');
    require(sent, 'failed');
}

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

cancelAllOrders() is used to cancel all pending orders, however, the value of userMinOrderNonce[caller] is in the range of next + 1000000 which means that userMinOrderNonce can be set arbitrarily, resulting in the cancelMultipleOrders() function not being called successfully.

function cancelAllOrders(uint256 minNonce) external {
    require(minNonce > userMinOrderNonce[msg.sender], 'nonce too low');
    require(minNonce < userMinOrderNonce[msg.sender] + 1000000, 'too many');
    userMinOrderNonce[msg.sender] = minNonce;
    emit CancelAllOrders(msg.sender, minNonce);
  }

function cancelMultipleOrders(uint256[] calldata orderNonces) external {
    uint256 numNonces = orderNonces.length;
    require(numNonces > 0, 'cannot be empty');
    for (uint256 i = 0; i < numNonces; ) {
      require(orderNonces[i] >= userMinOrderNonce[msg.sender], 'nonce too low');
      require(!isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]], 'nonce already executed or cancelled');
      isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]] = true;
      unchecked {
        ++i;
      }
    }
    emit CancelMultipleOrders(msg.sender, orderNonces);
  }

e,g.

Step 1.

minNonce(999999) > userMinOrderNonce[msg.sender]=0

minNonce(999999) < 0 + 1000000

userMinOrderNonce[msg.sender] = 999999

Step 2.

minNonce(1999998) > userMinOrderNonce[msg.sender]=999999

minNonce(1999998) < 999999 + 1000000

userMinOrderNonce[msg.sender] = 1999999

The principle is the current value + 1000000 - 1.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Set a maximum limit, i.e. the user's current nonce + 1000000 must not exceed the limit

Title:

INFINITY_TOKEN global variable mutability to immutable

Content:

I noticed that other contracts use immutable to optimize gas consumption, and the only global variable in the InfinityStaker.sol contract that will change during the deployment phase and not change later is not set to immutable, so I suggest changing the mutability of INFINITY_TOKEN to immutable.