Seaport's validate returned value is not validated

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L207

Vulnerability details

Impact

ClearingHouse's validateOrder function calls Seaport's validate function to list the NFT on OpenSea. Seaport's validate returns a boolean value upon success or failure. However, the value isn't validated.

This results in (not limited to);

1. Wrong state update on Astaria's protocol that the NFT under auction but in fact, it's not.
2. Accruing interest is stopped for all liens against the collateral token.
3. When the auction time passes (e.g. 72 hours), the NFT can be claimed skipping the auction step.

From here you can imagine what else could go wrong.

Proof of Concept

• Check the Seaport's Consideration contract:

  function validate(Order[] calldata orders)
      external
      override
      returns (bool validated)
  {
      // Validate the orders.
      validated = _validate(orders);
  }

  https://github.com/ProjectOpenSea/seaport/blob/main/contracts/lib/Consideration.sol#L457-L464

As you can see, the function doesn't revert on false returned value. The same goes for the internal function _validate.

• The ClearingHouse's validateOrder function doesn't check the returned value as well.

  function validateOrder(Order memory order) external {
  IAstariaRouter ASTARIA_ROUTER = IAstariaRouter(_getArgAddress(0));
  require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
  Order[] memory listings = new Order[](1);
  listings[0] = order;
  
  ERC721(order.parameters.offer[0].token).approve(
    ASTARIA_ROUTER.COLLATERAL_TOKEN().getConduit(),
    order.parameters.offer[0].identifierOrCriteria
  );
  ASTARIA_ROUTER.COLLATERAL_TOKEN().SEAPORT().validate(listings);
  }

https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L207

Tools Used

Manual analysis

Recommended Mitigation Steps

Check if the returned value is true. otherwise, revert.

An example:

 require(ASTARIA_ROUTER.COLLATERAL_TOKEN().SEAPORT().validate(listings), "Failed to list on Seaport");

[SantiagoGregory]

@androolloyd

[androolloyd]

while we should be requiring that orders are valid, there is currently no incorrect implementation of the orders definition, so its always returning true.

should we update things in the future that break order definition and that isn't caught by tests this would be an issue, is a QA or medium issue

[c4-sponsor]

androolloyd marked the issue as disagree with severity

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

Low severity in the absence of a PoC where it would return false.

[c4-judge]

Picodes marked the issue as grade-a