Inappropriate Fee Charge

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-looksrare/tree/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/SeaportProxy.sol#L199-L217

Vulnerability details

Impact

The fulfillAdvancedOrder may return false on an order not successfully fulfilled. The try/catch only skip current order when it reverts on fail, while still charge fee on user when it returns false.

Proof of Concept

https://github.com/code-423n4/2022-11-looksrare/tree/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/SeaportProxy.sol#L199-L217

try
    marketplace.fulfillAdvancedOrder{value: currency == address(0) ? price : 0}(
        advancedOrder,
        new CriteriaResolver[](0),
        bytes32(0),
        recipient
    )
  {
    if (feeRecipient != address(0)) {
        uint256 orderFee = (price * feeBp) / 10000;
        if (currency == lastOrderCurrency) {
            fee += orderFee;
        } else {
            if (fee > 0) _transferFee(fee, lastOrderCurrency, feeRecipient);
  
            lastOrderCurrency = currency;
            fee = orderFee;
        }
    }
  } catch {}

when marketplace.fulfillAdvancedOrder return false (but not reverted), the code in the try clause will execute, and charge the fee on the user. However, it is inappropriate to charge a fee on a not fulfilled order.

Tools Used

Recommendation

also check return value on marketplace.fulfillAdvancedOrder

try
    marketplace.fulfillAdvancedOrder{value: currency == address(0) ? price : 0}(
        advancedOrder,
        new CriteriaResolver[](0),
        bytes32(0),
        recipient
    ) returns (bool fulfilled)
  {
    if (fulfilled && feeRecipient != address(0)) {
        uint256 orderFee = (price * feeBp) / 10000;
        if (currency == lastOrderCurrency) {
            fee += orderFee;
        } else {
            if (fee > 0) _transferFee(fee, lastOrderCurrency, feeRecipient);
  
            lastOrderCurrency = currency;
            fee = orderFee;
        }
    }
  } catch {}

[c4-judge]

Picodes marked the issue as duplicate of #93

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b