code-423n4 / 2023-01-opensea-findings

0 stars 0 forks source link

Lack of proper input validation in fulfillOrder function #31

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/ProjectOpenSea/seaport/blob/5de7302bc773d9821ba4759e47fc981680911ea0/contracts/interfaces/SeaportInterface.sol#L68-L71

Vulnerability details

Impact

 function fulfillOrder(
        Order calldata order,
        bytes32 fulfillerConduitKey
    ) external payable returns (bool fulfilled);

fulfillOrder function is designed to fulfill orders on the marketplace, however, the code provided does not clearly define what fields the input struct Order contains and if they are properly verified before fulfilling the order.

Proof of Concept

An attacker could create a struct that has a higher value for the orderValue field than the intended order, allowing them to gain an unfair advantage by paying less than the actual value of the order.

pragma solidity ^0.8.13;

contract Attacker {
    struct Order {
        uint256 orderValue;
    }
    function exploit(address seaport) public {
        Order memory order;
        order.orderValue = 10000; // 10 ether, while the real order value is 1 ether
        seaport.fulfillOrder(order);
    }
}

Tools Used

Manual audit.

Recommended Mitigation Steps

Clearly define the fields of the Order struct and their expected types in the contract documentation. This will provide a clear understanding of what information is required to fulfill an order and what the expected range of values for each field is.

pragma solidity ^0.8.13;

contract Seaport {
    struct Order {
        uint256 orderValue;
    }
    function fulfillOrder(Order calldata order) external payable {
        require(order.orderValue > 0, "Order value must be greater than zero");
        // fulfill the order
    }
}
0age commented 1 year ago

contested, another very scattered report that doesn't use the correct params (which are quite documented)

HickupHH3 commented 1 year ago

Similar to #30. There are relevant checks on orderValue and msg.value:

// Utilize assembly to compare the route to the callvalue.
assembly {
    // route 0 and 1 are payable, otherwise route is not payable.
    correctPayableStatus := eq(
        additionalRecipientsItemType,
        iszero(callvalue())
    )
}

// Revert if msg.value has not been supplied as part of payable
// routes or has been supplied as part of non-payable routes.
if (!correctPayableStatus) {
    _revertInvalidMsgValue(msg.value);
}

and

// Ensure that sufficient native tokens are still available.
if (item.amount > nativeTokenBalance) {
   _revertInsufficientEtherSupplied();
}
c4-judge commented 1 year ago

HickupHH3 marked the issue as unsatisfactory: Insufficient quality