SEF-01M: Double Payout Vulnerability

[JayWelsh]

SEF-01M: Double Payout Vulnerability

Type	Severity	Location
Logical Fault	Major	SaleEnderFacet.sol:L97-L98, L166-L169

Description:

The sale system suffers from a double payout vulnerability as the disburseFunds function is re-entrant. In detail, a sale can claimPendingPayout with the final unit available and during the disburseFunds hook in that function purchase the last unit and then call closeSale before the setConsignmentPendingPayout has been zeroed out. This will lead to the attacker acquiring double the payout at the expense of the system. The same attack vector albeit simplified is applicable for the cancellation of a sale.

Example:

function closeSale(uint256 _consignmentId)
external
override
{
    // Get Market Handler Storage slot
    MarketHandlerLib.MarketHandlerStorage storage mhs = MarketHandlerLib.marketHandlerStorage();

    // Get consignment
    Consignment memory consignment = getMarketController().getConsignment(_consignmentId);

    // Make sure the sale exists and can be closed normally
    Sale storage sale = mhs.sales[_consignmentId];
    require(sale.start != 0, "Sale does not exist");
    require(sale.state != State.Ended, "Sale has already been settled");
    require(sale.state == State.Running, "Sale hasn't started");

    // Determine if consignment is physical
    address nft = getMarketController().getNft();
    if (nft == consignment.tokenAddress && ISeenHausNFT(nft).isPhysical(consignment.tokenId)) {

        // Check how many total claims are possible against issued tickets
        address escrowTicketer = getMarketController().getEscrowTicketer(_consignmentId);
        uint256 totalTicketClaimsIssued = IEscrowTicketer(escrowTicketer).getTicketClaimableCount(_consignmentId);

        // Ensure that sale is sold out before allowing closure
        require((consignment.supply - totalTicketClaimsIssued) == 0, "Sale cannot be closed with remaining inventory");

    } else {

        // Ensure that sale is sold out before allowing closure
        require((consignment.supply - consignment.releasedSupply) == 0, "Sale cannot be closed with remaining inventory");   

    }

    // Mark sale as settled
    sale.state = State.Ended;
    sale.outcome = Outcome.Closed;

    // Distribute the funds (handles royalties, staking, multisig, and seller)
    disburseFunds(_consignmentId, consignment.pendingPayout);
    getMarketController().setConsignmentPendingPayout(consignment.id, 0);

    // Notify listeners about state change
    emit SaleEnded(_consignmentId, sale.outcome);

}

Recommendation:

We advise the Checks-Effects-Interactions (CEI) pattern to be conformed to by first nullifying the pending payout and then distributing it to ensure that no such re-entrancy can unfold.

[cliffhall]

Response

• claimPendingPayout isn't a method anywhere in the codebase. Not sure what it's referencing.
• A reentrancy to this method (triggered when disburseFunds is called) would revert before doing any damage because sale.state was called to State.Ended prior to calling disburseFunds in the first pass:

Prior to triggering reentrancy:

    // Mark sale as settled
    sale.state = State.Ended;
    sale.outcome = Outcome.Closed;

    // Distribute the funds (handles royalties, staking, multisig, and seller)
    disburseFunds(_consignmentId, consignment.pendingPayout);

Upon re-entering:

    require(sale.state != State.Ended, "Sale has already been settled");

Remediation Suggestion

• Although re-entrancy here is possible, the appropriate effect for stopping a re-entrant call doing anything malicious has been actually made. Therefore it is not an actual threat.
• Regardless, best practices should be observed strictly where possible, so moving all effects before interactions should be done here, and doesn't have any functional impact.

    // Distribute the funds (handles royalties, staking, multisig, and seller)
    getMarketController().setConsignmentPendingPayout(consignment.id, 0);
    disburseFunds(_consignmentId, consignment.pendingPayout);

[JayWelsh]

Addressed by https://github.com/seen-haus/seen-contracts/pull/55

Also makes CEI adjustment to cancelAuction & cancelAuction of AuctionEnderFacet.sol