fulfillAdvancedOrder return value ignored

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/14865ba6a33ab7e8631e6ef2bd5e98207ca57f2a/contracts/proxies/SeaportProxy.sol#L199

Vulnerability details

Impact

According to Seaport's ConsiderationInterface (see here), the function fulfillAdvancedOrder returns "A boolean indicating whether the order has been succesfully fulfilled". However, SeaportProxy._executeNonAtomicOrders ignores the return value and simply assumes that the transfer was succesful, even when it was not. Therefore, it can happen that the user has to pay fees for an unsuccesful order, which normally should not happen.

Note that the current implementation always either returns true or reverts. However, Seaport is being developed further. For instance, Seaport 1.1 is currently in development. Because of that, it is strongly recommended to follow the described interface, which will be stable across versions, whereas implementation details can change.

Proof Of Concept

Let's say that the Seaport implementation changes in the future. Of course, they will still follow the described interface such that all integrations continue to work. However, the interface already says that the boolean return value indicates the success. Because this value is not checked by LooksRare, the following can happen: Bob performs a non-atomic trade on Seaport via Looksrare for 100 ETH with a 10% fee. However, the trade does not succeed. Because this is not detected by Looksrare, Bob still has to pay the 10 ETH fee, which is lost.

Recommended Mitigation Steps

Check the return value.

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes marked the issue as primary issue

[0xhiroshi]

Since the return value with the current Seaport impl is always true, so it doesn't solve any actual issues. When Seaport has an upgraded impl we are going to have an upgraded proxy for Seaport

[c4-sponsor]

0xhiroshi marked the issue as disagree with severity

[Picodes]

As the current implementation is designed for the current Seaport implementation and the address of Seaport would change, nothing can automatically happen. To me this is similar to a lack __gaps in upgradeable contracts, so falls within QA.