Infinity NFT Marketplace contest - Lambda's results

Lines of code

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

Vulnerability details

Impact

InfinityExchange.sol and InfinityStaker.sol contain a function rescueETH (InfinityExchange.sol#L1229 and InfinityStaker.sol#L345) to rescue any ETH that is accidentally sent to the contract. However, these functions do not have an amount parameter and just send msg.value to the destination, i.e. the amount that was just sent to the function. Rescuing accidentally sent ETH is therefore not possible with these functions.

Recommended Mitigation Steps

Add an amount parameter to rescueETH.

Judge has assessed an item in Issue #129 High risk. The relevant finding follows:

InfinityExchange.sol#L326 and InfinityExchange.sol#L362: When a user pays too much ETH, the additional cost is not reimbursed (in contrast to ERC20 transfers, where this is not possible). Consider reimbursing the user (like other NFT marketplaces, e.g. Rarible) when he pays too much ETH.

• InfinityExchange.sol#L525: address(0) has to be added as a valid currency, otherwise isOrderValid will fail. Consider hardcoding address(0) as a valid currency, because it always is for sell orders according to the rest of the contract
• InfinityExchange.sol#L412: isNonceValid will fail, even when the nonce would be valid, because userMinOrderNonce[user] is 0 initially. Therefore, the check nonce > userMinOrderNonce[user] fails. Consider handling this case of an uninitialized userMinOrderNonce[user] or documenting this requirement explicitly.
• InfinityExchange.sol#L484: The documentation states that verifyMatchOrders "checks if the given complication can execute this order", which is not done. Consider updating the description to reflect the real behavior.
• InfinityStaker.sol#L310: amount > vestedTwelveMonths can and should never happen because of InfinityStaker.sol#L123, where it is checked that the sum of the vested tokens is larger than or equal to amount. The code can therefore be removed or a revert (when the function is used from other places in the future) could be added.
• InfinityStaker.sol#L72: The timestamp is reset for all staked tokens with the given duration when additional tokens are staked. This can disincentivize staking for some users, which is not desirable for the market place. For example, when a user has staked some tokens already for 10 months (with a duration of 12 months), he probably will not stake additional tokens, because the timer for all tokens is reset. A better solution would be to have timestamps for every deposit.
• InfinityExchange.sol#L326 and InfinityExchange.sol#L362: When a user pays too much ETH, the additional cost is not reimbursed (in contrast to ERC20 transfers, where this is not possible). Consider reimbursing the user (like other NFT marketplaces, e.g. Rarible) when he pays too much ETH.

• SignatureChecker.sol#L32: This check is actually not needed and performed by ecrecover. See https://twitter.com/alexberegszaszi/status/1534461421454606336 for an extensive discussion.
• InfinityStaker.sol: In #L301, #L305, and #L309, the subtractions can be unchecked because there is already a check for underflow before the statements.