Infinity NFT Marketplace contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

Depending on the amounts and timeline of user stakes, the user can lose locked value upon unstaking. The unstake() function firstly checks the vested balance of the user for each duration and then calls _updateUserStakedAmounts(). This function steps through each duration linearly, setting the duration stake amount to 0 if the requested amount is greater than the vested amount. This will cause loss of funds in certain circumstances. A detailed example is outlined below.

Proof of Concept

Imagine the scenario where a user stakes an amount 100 for each duration, but at specific times. The user stakes for Duration.TWELVE_MONTH 12 months ago, so it is fully vested. The user then stakes in Duration.NONE, Duration.THREE_MONTHS, and Duration.SIX_MONTH right this instant. The user's stake balance and vested balance will looks as follows:

Duration | Stake | Vested NONE | 100 | 100 3 MO | 100 | 0 6 MO | 100 | 0 12 MO | 100 | 100

The user attempts to unstake 200. This will call _updateUserStakedAmounts() as such:

Definition w/ params: _updateUserStakedAmounts(msg.sender, amount, noVesting, vestedThreeMonths, vestedsixMonths, vestedTwelveMonths);

Actual call: _updateUserStakedAmounts(msg.sender, 200, 100, 0, 0, 100);

When these values are passed into _updateUserStakedAmounts(), the function will set the user's THREE_MONTH and SIX_MONTH stake balances to 0. The function is a multiple-nested if statement, but the logic in question is this portion:

if (amount > noVesting) {
      userstakedAmounts[user][Duration.NONE].amount = 0;
      userstakedAmounts[user][Duration.NONE].timestamp = 0;
      amount = amount - noVesting;
      if (amount > vestedThreeMonths) {
        userstakedAmounts[user][Duration.THREE_MONTHS].amount = 0;
        userstakedAmounts[user][Duration.THREE_MONTHS].timestamp = 0;
        amount = amount - vestedThreeMonths;
        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;

• amount > noVesting -> (200 > 100), so userstakedAmounts[user][Duration.NONE].amount is correctly set to 0.
• amount is reduced to 100.
• amount > vestedThreeMonths -> (100 > 0), so userstakedAmounts[user][Duration.THREE_MONTHS].amount is INCORRECTLY set to 0.
• amount > vestedSixMonths -> (100 > 0), so userstakedAmounts[user][Duration.SIX_MONTHS].amount is INCORRECTLY set to 0.
• Finally, the final else statement executes and correctly decrements userstakedAmounts[user][Duration.TWELVE_MONTHS].amount -= amount;

The user has lost the value of their 3 and 6 months stakes.

Tools Used

Manual review.

Recommended Mitigation Steps

Instead of stepping through these linearly based on duration, step through based on total vested amount of each duration. In the above example, the function should execute in the following order:

NONE -> TWELVE_MONTHS -> THREE_MONTHS -> SIX_MONTHS

Lines of code

https://github.com/infinitydotxyz/exchange-contracts-v2/blob/c51b7e8af6f95cc0a3b5489369cbc7cee060434b/contracts/core/InfinityExchange.sol#L230

Vulnerability details

Impact

Due to transfer control on the ERC1155 transfer, a buyer is able to reenter the contract and take the sell order again, effectively doubling the amount that the seller ordered to sell. Specific conditions need to be met for this to be true.

• Seller must by selling 2 or more ERC1155s
• Original buy must be initiated by matchOneToManyOrders() function called by the MATCH_EXECUTOR
• Seller must own more ERC1155 than placed in the sell order for this exploit to make sense

The issue stems from the fact that the original sell order is only marked as true after the many buy orders have completed. The attacker is able to reenter and call takeOrders() in the middle of the for loop of the original matched buy orders.

Proof of Concept

• A seller lists a quantity greater than 1 of an ERC1155 token. For this example, the seller wants to sell 5 total qty of ERC1155.
• The buyer (attacker) creates 5 separate buy orders for the ERC1155. Each buy order requests 1 qty.
• The MATCH_EXECUTOR attempts to match the sell order and multiple buy orders by calling matchOneToManyOrders().
• The matchOneToManyOrders() function checks that the orders are valid (not completed or cancelled) and initiates the for loop to iterate through the buy orders.
• The first buy order terminates by sending the ERC1155 to the attacker via safeBatchTransferFrom() which internally calls _doSafeBatchTransferAcceptanceCheck() which makes an external call to the receiver, transferring control of execution.

NFTs transferred: 1 NFTs remaining: 4

• The attacker can now call takeOrders() and take the entire sell order. Since the sell order has not been marked as completed yet, the call is valid. The attacker receives all 5 qty from the original sell order.

IMPORTANT: the match... functions do not contain the nonReentrant modifier. Only the takeOrders() function does. Therefore, this exploit does not get mitigated by reentrancy guard.

NFTs transferred: 6 NFTs remaining: 4

• The other 4 orders from the matchOneToManyOrders() complete, transferring 4 more NFTs to the attacker.

NFTs transferred: 10 NFTs remaining: 0

The attacker has now bought double the NFTs than the owner listed.

Tools Used

Manual review.

Recommended Mitigation Steps

Multiple options:

1. Add reentrancy guard to the match functions.
2. Move the isUserOrderNonceExecutedOrCancelled[makerOrder.signer][makerOrder.constraints[5]] = true; line above the for loop so that the order is marked as completed prior to control transfer.
3. This is expensive for gas, but you can check the order validity before EVERY buy order in the for loop to make sure the sell order is still uncompleted. This isn't an all encompassing solution but would help mitigate a little bit.

I recommend option 2. Option 1 will add a bit of gas to the match calls.

[N-01] Only constants should be styled as all caps

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L59-L63 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L27 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L33-L42

[G-01] Can be declared constant

No setter function for INFINITY_TOKEN other than constructor.

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