Infinity NFT Marketplace contest - GreyArt's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1228-L1232 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L348-L352

Vulnerability details

Description

The rescueETH() function uses msg.value instead of the contract’s balance as the amount to be sent back to the owner, thereby defeating its purpose since it merely sends back whatever ETH was sent with the transaction. No rescue performed at all :(

Impact

Exchange fees (in the case of the InfinityExchange contract) and funds in InfinityStaker aren’t rescued. I guess you can say that they are locked up till infinity and beyond.

Recommended Mitigation Steps

/// @dev used for rescuing exchange fees paid to the contract in ETH
function rescueETH(address destination) external onlyOwner {
  (bool sent, ) = destination.call{value: address(this).balance}('');
  require(sent, 'failed');
}

Lines of code

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

Vulnerability details

Description

Users aren’t refunded any excess ETH funds if they sent more than the total price of the orders. It becomes unfairly taken by the exchange as fees.

Recommended Mitigation Steps

Either ensure strict equality

require(msg.value == totalPrice, 'invalid total price');

or refund users the excess funds.

Lines of code

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

Vulnerability details

Impact

Funds for later but shorter schedules will be permanently lost when unstaking for vested earlier but longer schedules.

Description

_updateUserStakedAmounts() fails to account for the case when shorter but later schedules are in the midst of vesting, but the longer ones have been vested. Because the shorter ones will have 0 amount, their state is being overridden to 0 as well, causing funds to be permanently lost.

// Take the 3 and 6 months vestings for instance
// 3 months unvested => vestedThreeMonths = 0
// 6 months vested => vestedThreeMonths > 0
// amount = sum of all vested > 0
// both conditions are true, so amounts and timestamps in both durations
// get overridden
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;

See the commented POC for details.

Proof of Concept

it.only('Will cause funds of shorter but later vesting durations to be set to zero', async function () {
  // step 0: setup
  await approveERC20(signer1.address, token.address, amountStaked2, signer1, infinityStaker.address);

  // step 1: stake 6 months
  await infinityStaker.connect(signer1).stake(amountStaked2.div(2), 2);
  
  // step 2: move forward by 4 months, then stake 3 months
  // so that 6 months duration will vest first
  // but 3 months duration still in vesting progress
  await network.provider.send('evm_increaseTime', [120 * DAY]);
  await network.provider.send('evm_mine', []);
  await infinityStaker.connect(signer1).stake(amountStaked2.div(2), 1);

  // step 3: move forward by 2+ months, unstake 6 months duration
  await network.provider.send('evm_increaseTime', [61 * DAY]);
  await network.provider.send('evm_mine', []);
  await infinityStaker.unstake(amountStaked2.div(2));
  
  // verify staked amounts become zero for all durations
  // user lost amount for 3 month duration
  let userStakedAmounts = await infinityStaker.userstakedAmounts(signer1.address, 0);
  expect(userStakedAmounts.amount).to.equal(toBN(0));
  userStakedAmounts = await infinityStaker.userstakedAmounts(signer1.address, 1);
  expect(userStakedAmounts.amount).to.equal(toBN(0));
  userStakedAmounts = await infinityStaker.userstakedAmounts(signer1.address, 2);
  expect(userStakedAmounts.amount).to.equal(toBN(0));
  userStakedAmounts = await infinityStaker.userstakedAmounts(signer1.address, 3);
  expect(userStakedAmounts.amount).to.equal(toBN(0));
});

Tools Used

Hardhat

Recommended Mitigation Steps

The if conditions should also check that the relevant vested amounts are greater than zero.

if (amount > noVesting && noVesting > 0) {
	...
	if (amount > vestedThreeMonths && vestedThreeMonths > 0) {
		...
	  if (amount > vestedSixMonths && vestedSixMonths > 0) {
			...
			if (amount > vestedTwelveMonths && vestedTwelveMonths > 0) {
				...

Lines of code

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

Vulnerability details

Description

It is possible for the owner to set the protocol fee to be above the theoretical maximum fee allowable of BPS = 10_000.

Should this be done, order executions will be bricked because the protocol fee calculated will exceed the amount sent.

Recommended Mitigation Steps

Enforce a maximum fee limit.

require(_protocolFeeBps < 10_000, "cannot exceed BPS");

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L54 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L94 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L138-L140 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L163

Vulnerability details

Description

The function doItemsIntersect() only checks if order2Nfts is a subset of order1Nfts but it does not ensure that there are no duplicates in the array. For brevity, let’s assume that makerOrder is [1,2,3,4,5] and manyMakerOrders is [[1], [1], [1], [1], [1]] in the canExecMatchOneToMany() function.

Running through the code, numItems will have a value of 5 and itemsIntersect = itemsIntersect && doItemsIntersect(makerOrder.nfts, manyMakerOrders[i].nfts); will always be true since [1] is always be an intersect of [1,2,3,4,5]. Assuming _isTimeValid and _isPriceValid are both true then canExecMatchOneToMany will return true but this is not true since you cannot match [1,2,3,4,5] with [[1], [1], [1], [1], [1]].

Note that isOrderValid will not be able to catch this either because it only checks if the nonce is still valid which is true as the nonce is only updated later in either _execMatchOneMakerSellToManyMakerBuys or _execMatchOneMakerBuyToManyMakerSells.

Recommended Mitigation Steps

The simplest way to solve this is to sort the orders and loop through to ensure there are no duplicate orders). Moreover, sorting the orders offchain and hashing them can also allow you to save a lot of gas when checking if orders match.

Lines of code

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

Vulnerability details

Description

There doesn't seem to be a use case for the existence of the receive() and fallback() functions. Removing them is recommended as it will prevent accidental ETH transfers to the contract, which will then require the owner to call rescueETH(), creating needless customer support queries.

Recommended Mitigation Steps

Remove the receive() and fallback() functions.