Infinity NFT Marketplace contest - shenwilly's results

Lines of code

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

Vulnerability details

An order's type and it's rules are defined in it's Complication. Not checking it would allow anyone to take any orders regardless of their Complication's rule, causing unexpected execution for order makers.

takeMultipleOneOrders assumes that all makerOrders are simple orderbook orders and the Complication check is missing here.

Proof of Concept

• Alice signs a makerOrder with PrivateSaleComplication, allowing only Bob to take the private sale order.
• A malicious trader calls takeMultipleOneOrders to take Alice's order, despite the Complication only allowing Bob to take it.

Recommended Mitigation Steps

Add canExecTakeOneOrder function in IComplication.sol and implement it in InfinityOrderBookComplication (and future Complications) to support takeMultipleOneOrders operation, then modify takeMultipleOneOrders to use the check:

function takeMultipleOneOrders() {
    ...
    for (uint256 i = 0; i < numMakerOrders; ) {
        bytes32 makerOrderHash = _hash(makerOrders[i]);
        bool makerOrderValid = isOrderValid(makerOrders[i], makerOrderHash);
        bool executionValid = IComplication(makerOrders[i].execParams[0]).canExecTakeOneOrder(makerOrders[i]);
        
        require(makerOrderValid && executionValid, 'order not verified');
        
        require(currency == makerOrders[i].execParams[1], 'cannot mix currencies');
        require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides');
        uint256 execPrice = _getCurrentPrice(makerOrders[i]);
        totalPrice += execPrice; // @audit-issue missing complication check
        _execTakeOneOrder(makerOrderHash, makerOrders[i], isMakerSeller, execPrice);
        unchecked {
            ++i;
        }
    }
    ...
}

Lines of code

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

Vulnerability details

A malicious or compromised executor can execute matching orders at a very high tx.gasprice, causing buyers to pay more gasCost than necessary.

While this griefing attack is costly (unless governance is also malicious and can refund the gasCost), it's better to let users the ability to decide beforehand how much gas price they are willing to pay for auto snipes.

Recommended Mitigation Steps

Consider adding maxGasPrice variable in MakerOrder and check them before executing matching orders. For example, if we are using execParams[2] to store the gas price limit:

if (buy.execParams[2] > 0 ) {
    require(buy.execParams[2] <= tx.gasprice, "gas price too expensive");
}

Lines of code

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

Vulnerability details

A malicious or compromised governance can set protocol fee to an unreasonable amount and steal funds from sellers.

When executing orders, governance can frontrun the transactions by setting protocol fee to 10000, effectively taking all funds supposed to be received by seller.

Proof of Concept

• Protocol fee is set to 2,5%.
• Alice signed a sell order with 100 ETH price.
• Bob is interested and calls takeOrders to buy Alice's order.
• Governance frontrun Bob's transaction by setting protocol fee to 100%.
• Alice's NFT is sent to Bob, but Alice didn't receive any money.

Recommended Mitigation Steps

Set a sanity check in setProtocolFee so governance can't set it to unreasonable value. Consider using timelock for setting governance settings.

function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {
    require(_protocolFeeBps <= 1500, "fee must not be higher than 15%");
    PROTOCOL_FEE_BPS = _protocolFeeBps;
    emit NewProtocolFee(_protocolFeeBps);
}

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#L739-L747

Vulnerability details

A malicious or compromised governance can set the transfer gas cost to an unreasonable amount and steal approved WETH from buyers.

There are two ways for governance to exploit this:

• When an order is being executed, governance can frontrun the transactions by setting WETH_TRANSFER_GAS_UNITS to a very high amount.
• Set WETH_TRANSFER_GAS_UNITS to a very high amount, and execute trades against active buy orders. As long as the value of WETH to steal is higher than the cost to prepare the NFTs to sell, it is profitable to do so.

Proof of Concept

• WETH_TRANSFER_GAS_UNITS is set to 50000.
• Alice has 100 WETH and 100 USDC. She approved infinite allowance to InfinityExchange.
• Alice signs a buy order to buy a FakePunk NFT with 100 USDC price.
• Malicious governance sets WETH_TRANSFER_GAS_UNITS to a very high amount such that the gasCost calculation equals 100 WETH.
• Governance then bought a FakePunk in open market, and fills Alice's order.
• Alice received the NFT but paid 100 WETH as gas cost.

Recommended Mitigation Steps

Set a sanity check in updateWethTranferGas so governance can't set it to unreasonable value. Consider using timelock for setting governance settings.

function updateWethTranferGas(uint32 _wethTransferGasUnits) external onlyOwner {
    require(_wethTransferGasUnits <= 100000, "gas unit must not be higher than 100000");
    WETH_TRANSFER_GAS_UNITS = _wethTransferGasUnits;
    emit NewWethTransferGasUnits(_wethTransferGasUnits);
}

Low Risk Vulnerabilities

1. Unreachable code leading to wrong stake timestamp

In InfinityStaker._updateUserStakedAmounts:

if (amount > vestedTwelveMonths) {
    userstakedAmounts[user][Duration.TWELVE_MONTHS].amount = 0;
    userstakedAmounts[user][Duration.TWELVE_MONTHS].timestamp = 0; // @audit if amount == vestedTwelveMonths timestamp not 0, unreachable
} else {
    userstakedAmounts[user][Duration.TWELVE_MONTHS].amount -= amount;
}

The amount > vestedTwelveMonths check will never be satisfied as amount is guaranteed to be no more than total vested at L123. When amount equals vestedTwelveMonths, userstakedAmounts[user][Duration.TWELVE_MONTHS] will be reduced to zero but the timestamp is not reset.

Recommended Mitigation Steps

Use inclusive condition instead:

if (amount >= vestedTwelveMonths) {
    ...
}

Relevant Code

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

Non-Critical Vulnerabilities

1. Function name typo

updateWethTranferGas should be updateWethTransferGas.

Relevant Codes

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

1. Avoid magic numbers

It's best practice to use a constant instead of magic number for readability, especially for future maintainers.

Add a constant variable BPS = 10000.

Relevant Codes

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L725 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L775 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L819 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L873 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1135