ParaSpace contest - SmartSek's results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L302-L303

Vulnerability details

Description

delete doesnt delete mapping inside a struct. setPrice() wouldnt be able to change its price and any user or contract who queries the variable directly will receive stale/untruthful information.

Proof of Concept

Run forge test below.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "ds-test/test.sol";
import "forge-std/Script.sol";

contract TestDeleteMappingInStruct is DSTest, Script {
    struct Token {
        bool enabled;
        mapping(address => uint256) balances;
    }

    /// @notice Mapping of token address to Token struct
    mapping(address => Token) public tokens;

    function testMapping() public {
        address tkn = address(0xB0FFED);
        address badBabe = address(0xBADBABE);
        uint256 balanceBB = 1_000_000;

        tokens[tkn].balances[badBabe] = balanceBB;

        delete tokens[tkn];
        // should fail
        vm.expectRevert();
        assert(tokens[tkn].balances[badBabe] == 0);
    }

    function testMappingSuccess() public {
        address tkn = address(0xB0FFED);
        address badBabe = address(0xBADBABE);
        uint256 balanceBB = 1_000_000;

        tokens[tkn].balances[badBabe] = balanceBB;

        delete tokens[tkn].balances[badBabe];
        // should succeed
        assert(tokens[tkn].balances[badBabe] == 0);
    }
}


Variable affected:

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L86-L88

    /// @dev Original raw value to aggregate with
    // the NFT contract address -> FeederRegistrar which contains price from each feeder
    mapping(address => FeederRegistrar) public assetFeederMap;

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L32-L42

struct FeederRegistrar {
    // if asset registered or not
    bool registered;
    // index in asset list
    uint8 index;
    // if asset paused,reject the price
    bool paused;
    // feeder -> PriceInformation
    mapping(address => PriceInformation) feederPrice;
}

Causation: https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L302-L303

        delete assetFeederMap[_asset];

Recommended Mitigation Steps

Implement the following line:

        delete assetFeederMap[_asset].feederPrice;

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol#L189-L202

Vulnerability details

Impact

Users or contracts making calls directly to the smart contract will have their executeBatchBuyWithCredit() calls failing because the contract assumes parameters are in a certain order, even though there are no checks or logic enforcing such behaviour. Any fault or hang up in the frontend would also lead to reverts.

Proof of Concept

This check should be done at the smart contract level because there is nothing preventing users to interact in this manner directly through the smart contract.

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol#L189-L202

            // Once we encounter a listing using WETH, then we convert all our ethLeft to WETH
            // this also means that the parameters order is very important
            //
            // frontend/sdk needs to guarantee that WETH orders will always be put after ALL
            // ETH orders, all ETH orders after WETH orders will fail
            //
            // eg. The following example image that the `taker` owns only ETH and wants to
            // batch buy bunch of NFTs which are listed using WETH and ETH
            //
            // batchBuyWithCredit([ETH, WETH, ETH]) => ko
            //                            | -> convert all ethLeft to WETH, 3rd purchase will fail
            // batchBuyWithCredit([ETH, ETH, ETH]) => ok
            // batchBuyWithCredit([ETH, ETH, WETH]) => ok
            //

Recommended Mitigation Steps

Implement a check to ensure parameters are sent in the right order. A much more gas expensive alternative would be sorting them in the right order.

Please find below an instance of a similar issue where frontend input sanitization has been deemed to be insufficient: https://github.com/code-423n4/2022-06-canto-findings/issues/244#issuecomment-1205692351