Infinity NFT Marketplace contest - peritoflores's results

Lines of code

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

Vulnerability details

Impact

Admins are unable to recover any ETH locked in the contract

PoC

The function rescueETH#InfinityStaker.sol is incorrectly implemented. It is using msg.value instead of the balance of the contract

  /// @dev Admin function to rescue any ETH accidentally sent to the contract
  function rescueETH(address destination) external payable onlyOwner {
      (bool sent, ) = destination.call{value: msg.value}(''); @audit not working
      require(sent, 'Failed to send Ether');
  }

Recommended

Replace msg.value for address(this).balance to recover ETH.

Lines of code

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

Vulnerability details

Impact

Admins can attack users by frontrunning them

Poc

In the function setProtocolFee#InfinityExchange.sol

function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {

    PROTOCOL_FEE_BPS = _protocolFeeBps;@audit medium 

    emit NewProtocolFee(_protocolFeeBps);

}

Admins can front-run user by setting PROTOCOL_FEE_BPS to 10000 which is 100%. At the time of transfer protocolFee =amount and remaimingAmount=0

Recommended

Add a maximum reasonable limit for the fee (for example 5 or 10%)

Similar issues

https://github.com/code-423n4/2021-05-nftx-findings/issues/51

https://github.com/code-423n4/2021-06-gro-findings/issues/78

Lines of code

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

Vulnerability details

Impact

Users can lose their fund by attack from the admins

Poc

In the function updateWethTranferGas#InfinityExchange.sol there is no limit for setting WETH_TRANSFER_GAS_UNITS . As this is a kind of fee paid by from the buyers to the contract

By definition of the variable

/// @dev Gas cost for auto sniped orders are paid by the buyers and refunded to this contract in the form of WETH
uint32 public WETH_TRANSFER_GAS_UNITS = 50000;

The function is

/// @dev updates the gas units required for WETH transfers

function updateWethTranferGas(uint32 _wethTransferGasUnits) external onlyOwner {

WETH_TRANSFER_GAS_UNITS = _wethTransferGasUnits;

emit NewWethTransferGasUnits(_wethTransferGasUnits);@audit medium

}

And at the _execMatchOneToOneOrders#InfinityExchange.sol

uint256 gasCost = (startGasPerOrder - gasleft() + wethTransferGasUnits) * tx.gasprice;
// if the execution currency is weth, we can send the protocol fee and gas cost in one transfer to save gas
// else we need to send the protocol fee separately in the execution currency
if (buy.execParams[1] == weth) {
 IERC20(weth).safeTransferFrom(buy.signer, address(this), protocolFee + gasCost);
} else {
IERC20(buy.execParams[1]).safeTransferFrom(buy.signer, address(this), protocolFee);
IERC20(weth).safeTransferFrom(buy.signer, address(this), gasCost);
}

gasCost will be extremely high and buyer will paid this to the contract IERC20(weth).safeTransferFrom(buy.signer, address(this), protocolFee + gasCost);

Recommended

Add a maximum reasonable WETH_TRANSFER_GAS_UNITS.

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L364-L372

Vulnerability details

PoC

In the function updatePenalties#InfinityExchange.sol

function updatePenalties(
uint16 threeMonthPenalty,
uint16 sixMonthPenalty,
uint16 twelveMonthPenalty
) external onlyOwner {
THREE_MONTH_PENALTY = threeMonthPenalty;
SIX_MONTH_PENALTY = sixMonthPenalty;
TWELVE_MONTH_PENALTY = twelveMonthPenalty; 

}

There are not limits and also as this is a critical function lack of event emission is crucial as user need to be aware of penalties at the time of call rageQuit

Recommended

Add a maximum reasonable for penalties and emit an event

Gas Report for Infinity by PeritoFlores

Tautology in require (uint are always equal or higher than zero)

This is tautology

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L193

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

I believe that you mean != 0, however this is not affecting the logic so I believe you could just remove it saving gas

Use delete instead of assigning zero to storage variable

The keyword delete refunds you with 15,000 gas. Consider using it for example in this function

function _clearUserStakedAmounts(address user) internal {
  // clear amounts
  userstakedAmounts[user][Duration.NONE].amount = 0;@audit gas "use delete instead of = assign zero in storage"
  userstakedAmounts[user][Duration.THREE_MONTHS].amount = 0;
  userstakedAmounts[user][Duration.SIX_MONTHS].amount = 0;
  userstakedAmounts[user][Duration.TWELVE_MONTHS].amount = 0;

  // clear timestamps
  userstakedAmounts[user][Duration.NONE].timestamp = 0;
  userstakedAmounts[user][Duration.THREE_MONTHS].timestamp = 0;
  userstakedAmounts[user][Duration.SIX_MONTHS].timestamp = 0;
  userstakedAmounts[user][Duration.TWELVE_MONTHS].timestamp = 0;
 }

Use constants when you can to save gas

The following variables could be constants .

 uint256 PRECISION = 10**4;

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

 bytes32 ORDER_HASH = 0x7bcfb5a29031e6b8d34ca1a14dd0a1f5cb11b20f755bb2a31ee3c4b143477e4a;

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

 bytes32 ORDER_ITEM_HASH = 0xf73f37e9f570369ceaab59cef16249ae1c0ad1afd592d656afac0be6f63b87e0;

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

bytes32 TOKEN_INFO_HASH = 0x88f0bd19d14f8b5d22c0605a15d9fffc285ebc8c86fb21139456d305982906f1;
 

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

Use immutable variables when you can to save gas

Infinity token is a predefined address that you would not change

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L25

Consider not using SafeERC20 for INFINITY TOKEN

This is maybe a contradictory idea but when you know the implementation of your token you can save gas if you use transfer/transferFrom instead of safeTransfer/safeTransferFrom.

You can consider removing the library in lines like that.

IERC20(INFINITY_TOKEN).safeTransferFrom(msg.sender, address(this), amount);@audit gas check if neccessary

Maybe you comment explaining why you use transfer. I believe that this would save lot of gas.

Function using nonReentrant modifier without making any external call

The function changeDuration#InfinityStaker.sol is using nonReentrant modifier but only modifies internal variables without making any external call.

Avoid unnecessary initializations

Booleans initialized to false

bool _isPriceValid = false;

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

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

Uint initialized to zero

In for loops avoid initializing i or j to zero

Also in the following lines the initialization is unnecessary

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

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

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

getMaxEpochs() called twice in advanceEpoch#InifityToken.sol

In the function advanceEpoch#InifityToken.sol you are calling twice getMaxEpochs() which is an external contract call. Consider catching in a variable instead.