Infinity NFT Marketplace contest - rfa's results

Lines of code

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

Vulnerability details

Impact

Owner can't rescuing exchange fees paid of the contract

Proof of Concept

The function using msg.value as the value that intended to be transferred to destination. By doing this way, if owner want to rescue 10 ETH from the contract, he need to send 10 eth to make msg.value == 10. Therefore, The function is useless and the ETH will stuck in the contract.

Tools Used

Manual review

Recommended Mitigation Steps

Replace msg.value with address(this).balance:

    (bool sent, ) = destination.call{value: address(this).balance}('');

or make new parameter to send certain amount of ETH:

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

then remove the payable

LOW

Title: Useless receive() function

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

The InfinityExchange.sol contract already has fallback() function which can receive data and ether (receive can only receive eth). We can just remove receive() function

Title: Using constructor() in InfinityStaker.sol

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

The vars are not constant and can set anytime through function. Therefore, setting the initial value via constructor can increase readability of the code (since the hardcoded value won't have the same value as the value in the future). And also it can save gas

Title: Using delete statement

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

Using delete to set value == 0 can save 4 gas per execution