Infinity NFT Marketplace contest - Ruhum's results

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

The contract allows the user to overpay in ETH. It verifies that msg.value >= totalPrice instead of checking that it's equal: msg.value == totalPrice.

Generally, you can say that it's probably a mistake by the buyer if they pay more than necessary. Those funds are also not transferred to the seller. Instead, they are left in the Exchange contract which can be retrieved by the owner with the collected fees.

Proof of Concept

User is allowed to pay more:

• 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

The user only gets the actual sale amount (follow the call stack for this function):

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

Tools Used

none

Recommended Mitigation Steps

Change msg.value >= totalPrice to msg.value == totalPrice.

Lines of code

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

Vulnerability details

Impact

Following scenario:

Alice has staked X token for 6 months that have vested. She stakes Y tokens for another three months. If she now calls unstake(X) to take out the tokens that have vested, the Y tokens she staked for three months will be locked up.

Proof of Concept

First, here's a test showcasing the issue:

  describe('should cause trouble', () => {
    it('should lock up funds', async function () {
      await approveERC20(signer1.address, token.address, amountStaked, signer1, infinityStaker.address);
      await infinityStaker.connect(signer1).stake(amountStaked, 2);
      await network.provider.send("evm_increaseTime", [181 * DAY]);
      await network.provider.send('evm_mine', []);
      
      // The funds we staked for 6 months have vested
      expect(await infinityStaker.getUserTotalVested(signer1.address)).to.eq(amountStaked);

      // Now we want to stake funds for three months
      await approveERC20(signer1.address, token.address, amountStaked2, signer1, infinityStaker.address);
      await infinityStaker.connect(signer1).stake(amountStaked2, 1);

      // total staked is now the funds staked for three & six months
      // total vested stays the same
      expect(await infinityStaker.getUserTotalStaked(signer1.address)).to.eq(amountStaked.add(amountStaked2));
      expect(await infinityStaker.getUserTotalVested(signer1.address)).to.eq(amountStaked);

      // we unstake the funds that are already vested.
      const userBalanceBefore = await token.balanceOf(signer1.address);
      await infinityStaker.connect(signer1).unstake(amountStaked);
      const userBalanceAfter = await token.balanceOf(signer1.address);

      expect(userBalanceAfter).to.eq(userBalanceBefore.add(amountStaked));

      expect(await infinityStaker.getUserTotalStaked(signer1.address)).to.eq(ethers.BigNumber.from(0));
      expect(await infinityStaker.getUserTotalVested(signer1.address)).to.eq(ethers.BigNumber.from(0));
    });
  });

The test implements the scenario I've described above. In the end, the user got back their amountStaked tokens with the amountStaked2 tokens being locked up in the contract. The user has no tokens staked at the end.

The issue is in the _updateUserStakedAmounts() function:

    if (amount > noVesting) {
      userstakedAmounts[user][Duration.NONE].amount = 0;
      userstakedAmounts[user][Duration.NONE].timestamp = 0;
      amount = amount - noVesting;
      if (amount > vestedThreeMonths) {
        // MAIN ISSUE:
        // here `vestedThreeMonths` is 0. The current staked tokens are set to `0` and `amount` is decreased by `0`.
        // Since `vestedThreeMonths` is `0` we shouldn't decrease `userstakedAmounts` at all here.
        userstakedAmounts[user][Duration.THREE_MONTHS].amount = 0;
        userstakedAmounts[user][Duration.THREE_MONTHS].timestamp = 0;
        amount = amount - vestedThreeMonths;
        // `amount == vestedSixMonths` so we enter the else block
        if (amount > vestedSixMonths) {
          userstakedAmounts[user][Duration.SIX_MONTHS].amount = 0;
          userstakedAmounts[user][Duration.SIX_MONTHS].timestamp = 0;
          amount = amount - vestedSixMonths;
          if (amount > vestedTwelveMonths) {
            userstakedAmounts[user][Duration.TWELVE_MONTHS].amount = 0;
            userstakedAmounts[user][Duration.TWELVE_MONTHS].timestamp = 0;
          } else {
            userstakedAmounts[user][Duration.TWELVE_MONTHS].amount -= amount;
          }
        } else {
          // the staked amount is set to `0`.
          userstakedAmounts[user][Duration.SIX_MONTHS].amount -= amount;
        }
      } else {
        userstakedAmounts[user][Duration.THREE_MONTHS].amount -= amount;
      }
    } else {
      userstakedAmounts[user][Duration.NONE].amount -= amount;
    }

Tools Used

none

Recommended Mitigation Steps

Don't set userstakedAmounts.amount to 0 if none of its tokens are removed (vestedAmount == 0)

Lines of code

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

Vulnerability details

Impact

The way the gas refunds are computed in the InfinityExchange contract, the first orders pay less than the latter ones. This causes a loss of funds for the buyers whose orders came last in the batch.

Proof of Concept

The issue is that the startGasPerOrder variable is computed within the for-loop. That causes the first iterations to be lower than later ones.

Here's an example for the following line: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L202 To make the math easy we use the following values:

startGas = 1,000,000
gasPerOrder = 100,000 (so fulfilling an order costs us 100,000 gas)
ordersLength = 10

For the 2nd order we then get:

startGasPerOrder = 900,000 + ((1,000,000 + 20,000 - 900,000) / 10)
startGasPerOrder = 912,000

For the 9th order we get:

startGasPerOrder = 200,000 + ((1,000,000 + 20,000 - 200,000) / 10)
startGasPerOrder = 282,000

The startGasPerOrder variable is passed through a couple of functions without any modification until it reaches a line like this: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L231

uint256 gasCost = (startGasPerOrder - gasleft() + wethTransferGasUnits) * tx.gasprice;

There, the actual gas costs for the user are computed.

In our case, that would be:

# 2nd order
# gasleft() is 800,00 because we said that executing the order costs ~100,000 gas. At the beginning of the order, it was 900,000 so now it's 800,000. This makes the computation a little more straightforward although it's not 100% correct.
gasCost = (912,000 - 800,000 + 50,000) * 1
gasCost = 162,000

# 9th order
gasCost = (282,000 - 100,000 + 50,000) * 1
gasCost = 232,000

So the 2nd order's buyer pays 162,000 while the 9th order's buyer pays 232,000.

As I said the math was dumbed down a bit to make it easier. The actual difference might not be as big as shown here. But, there is a difference.

Tools Used

none

Recommended Mitigation Steps

The startGasPerOrder variable should be initialized outside the for-loop.

Lines of code

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

Vulnerability details

Impact

Currently, there's no limit on the InfinityExchange contract's protocol fee. Setting it to a value higher than 100% will make the contract unusable because of an underflow.

There should be a reasonable limit in the setProtocolFee() function, e.g. 5%.

Proof of Concept

setProtocolFee() function with no constraints: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1267

If the fee is higher than 100%, > 10000, the following line will trigger an underflow: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1135-L1136

Tools Used

none

Recommended Mitigation Steps

Add a constraint to the setProtocolFee() function.

Report

• Low
  • L-01: InfinityStaker's staking levels should have constraints for logical progression
  • L-02: InfinityStaker.getRageQuitAmounts() has a require statement that's a tautology
  • L-03: there should be limits for the ragequit penalty in InfinityStaker
• Non-Critical
  • N-01: emit an event when making changes to a contract's configuration

Low

L-01: InfinityStaker's staking levels should have constraints for logical progression

The staking levels follow a logical order, Bronze -> Silver -> Gold -> Platinum. The updateStakeLevelThreshold() function responsible for specifying each level's threshold should have constraints to now allow the levels to overlap. Currently, it's possible for the caller to set the threshold for silver lower than that of bronze.

Relevant code:

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

L-02: InfinityStaker.getRageQuitAmounts() has a require statement that's a tautology

Inside the function there's the following require statement:

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

totalStaked is a uint meaning it will always be >= 0. I guess you meant to use > 0

Relevant code:

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

L-03: there should be limits for the ragequit penalty in InfinityStaker

The owner can freely choose the penalty value. There should be a reasonable limit on it. The user might potentially call ragequit() without first checking the actual amount of tokens they would receive. They might have a rude awakening.

Relevant code:

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

Non-Critical

N-01: emit an event when making changes to a contract's configuration

There are multiple functions that handle part of a contract's configuration. Those functions should emit events.

Relevant code:

• https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L351-L377
• https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1235-L1257