Omniscia Seen Haus Audit

SaleEnderFacet Manual Review Findings

SaleEnderFacet Manual Review Findings

SEF-01M: Double Payout Vulnerability

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:

contracts/market/handlers/facets/SaleEnderFacet.sol
58function closeSale(uint256 _consignmentId)
59external
60override
61{
62    // Get Market Handler Storage slot
63    MarketHandlerLib.MarketHandlerStorage storage mhs = MarketHandlerLib.marketHandlerStorage();
64

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

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

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

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

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

85    } else {
86

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

90    }
91

92    // Mark sale as settled
93    sale.state = State.Ended;
94    sale.outcome = Outcome.Closed;
95

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

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

103}

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.

Alleviation:

The CEI pattern is now properly applied by nullifying the pending payout first and then disbursing it.

Prev

SaleBuilderFacet.sol (SBF-M)

Next

SaleRunnerFacet.sol (SRF-M)

On this page