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

0 stars 0 forks source link

Potential Manipulation Vulnerability in _validateOrdersAndPrepareToFulfill Function #49

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/ProjectOpenSea/seaport/blob/5de7302bc773d9821ba4759e47fc981680911ea0/contracts/lib/OrderCombiner.sol#L939-L945

Vulnerability details

Impact

The smart contract may not be properly validated, which could lead to fraudulent or malicious orders being fulfilled. This could result in loss of assets or other financial damage to users of the contract. Additionally, if the validation process is not properly implemented, it could make the contract more susceptible to hacking or other forms of attack.

Proof of Concept

with the vulnerable contract and manipulates the input passed to the _validateOrdersAndPrepareToFulfill function in order to bypass the validation process and manipulate the orders in some way.

contract MaliciousContract {
    // Address of the vulnerable contract
    address vulnerableContract;

    // Function to exploit vulnerability
    function exploitVulnerability() public {
        // Call the vulnerable contract's _validateOrdersAndPrepareToFulfill function with manipulated input
        vulnerableContract.call(bytes4(keccak256("_validateOrdersAndPrepareToFulfill(AdvancedOrder[],CriteriaResolver[],Fulfillment[],address)")), manipulatedOrders, manipulatedCriteriaResolvers, manipulatedFulfillments, manipulatedRecipient);
    }
}

Tools Used

Manual review, Vscode

Recommended Mitigation Steps

Checking that the offerer and fulfiller on each order have approved this contract (or their conduit if indicated by the order) to transfer any relevant tokens on their behalf.

Checking that each consideration recipient has implemented the onERC1155Received function in order to receive ERC1155 tokens.

Checking that the offer and consideration components for each order have no remainder after multiplying the respective amount with the supplied fraction in order for the group of partial fills to be considered valid.

Implementing additional checks and constraints as needed to ensure the validity and integrity of the orders being processed.

function _validateOrdersAndPrepareToFulfill(
    AdvancedOrder[] memory advancedOrders,
    bytes32[] memory orderHashes,
    bool invalidOrdersShouldRevert,
    uint256 totalOrders,
    address recipient
) internal returns (bytes32[] memory /* orderHashes */) {

    // Initialize variables for order hashes, item amounts, and order statuses.
    uint256[] memory itemAmounts = new uint256[](totalOrders);
    OrderStatus[] memory orderStatuses = new OrderStatus[](totalOrders);

    // Iterate over each order.
    for (uint256 i = 0; i < totalOrders; ) {
        // Retrieve the order.
        AdvancedOrder memory order = advancedOrders[i];

        // Check if offerer approved the contract
        if(!order.offerer.approve(address(this), order.offer.amount)) {
          if(invalidOrdersShouldRevert) _revertOffererNotApproved();
          orderStatuses[i] = OrderStatus.OFFERER_NOT_APPROVED;
        }

        // Check if fulfiller approved the contract
        if(!order.fulfiller.approve(address(this), order.consideration.amount)) {
          if(invalidOrdersShouldRevert) _revertFulfillerNotApproved();
          orderStatuses[i] = OrderStatus.FULFILLER_NOT_APPROVED;
        }

        // Check if consideration recipient implemented onERC1155Received
        if(!order.consideration.recipient.onERC1155Received.selector == keccak256(abi.encodePacked("onERC1155Received(address,address,uint256,bytes)"))) {
          if(invalidOrdersShouldRevert) _revertConsiderationRecipientNotImplemented();
          orderStatuses[i] = OrderStatus.CONSIDERATION_RECIPIENT_NOT_IMPLEMENTED;
        }

        // Check that the offer and consideration components do not have a remainder after multiplying with the fraction
        if((order.offer.amount % order.fraction != 0) || (order.consideration.amount % order.fraction != 0)) {
          if(invalidOrdersShouldRevert) _revertAmountsNotMultipleOfFraction();
          orderStatuses[i] = OrderStatus.AMOUNTS_NOT_MULT
0age commented 1 year ago

contested; there is no concrete finding presented here

HickupHH3 commented 1 year ago

"may not be properly validated".

Not sure what wasn't properly validated.

c4-judge commented 1 year ago

HickupHH3 marked the issue as unsatisfactory: Insufficient proof