A sale factory owner has the power to rug a fee receiver

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDAFactory.sol#L46-L48 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPriceFactory.sol#L42-L44 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEditionFactory.sol#L42-L44

Vulnerability details

Impact

In every sale factory contracts (LPDAFactory, FixedPriceFactory, and OpenEditionFactory), the owner of the factory contract has the power to change the feeReceiver at any time. This means that before that a sale has finalized (for FixedPrice and OpenEdition sales), the owner could frontrun the last buy() or do it before calling the cancel() function by itself in the case of the FixedPrice sale, or front-run/call by itself the finalize() after changing the feeReceiver. This issue is less likely to be very impactful for the LPDA sale, as the fees are linearized over time for each sale, but the very first sale in the case of a Dutch auction would be the higest price one. So a malicious factory owner would probably rather want to frontrun the first sale.

Proof of Concept

Usually, a feeReceiver is the collection artist. Multiple attack vectors have been stated before, but let's take the OpenEdition contract to illustrate, and for simplicity. We will denote Bob as the (malicious) sale factory owner, and Alice as the artist, and feeReceiver.

1. Bob deploys a OpenEditionFactory contract
2. Bob sets Alice as the feeReceiver with setFeeReceiver as agreed between them
3. The sale takes place, and a lot of NFTs have been sold
4. The sale is now finished, and somebody calls the finalize() function
5. Before that the function gets mined, Bob front-run the transaction by setting his own account as the feeReceiver
6. Bob has stolen the sales fees

Tools Used

Manual inspection

Recommended Mitigation Steps

Set the feeReceiver as a "sale" parameter upon the sale deployment, similarly to the saleReceiver for the best transparency for both parties.

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEdition.sol#L13-L23

    struct Sale {
        // slot 1
        uint72 price;
        uint24 currentId;
        address edition;
        // slot 2
        uint96 startTime;
        address payable saleReceiver;
        // slot 3
        uint96 endTime;
        + address payable feeReceiver;
    }

[c4-sponsor]

stevennevins marked the issue as sponsor disputed

[stevennevins]

Users won't be deploying their own sale factories, just their own sale contracts. Escher will have controller over the feeReceiver on the factory that is enforced on the sale contracts. So there is no ruggability of end users here.

[berndartmueller]

Downgrading to QA (Low) as it is a centralization risk.

[c4-judge]

berndartmueller changed the severity to QA (Quality Assurance)

[c4-judge]

berndartmueller marked the issue as grade-c