SRF-02M: Inexistent Validation of Remaining Supply

[JayWelsh]

SRF-02M: Inexistent Validation of Remaining Supply

Type	Severity	Location
Logical Fault	Medium	SaleRunnerFacet.sol:L105, L134

Description:

The ticket mechanism of the sale runner does not validate the specified purchase amount can actually be fulfilled by the consignment.

Example:

function buy(uint256 _consignmentId, uint256 _amount)
external
override
payable
onlyAudienceMember(_consignmentId)
{
    // Get Market Handler Storage slot
    MarketHandlerLib.MarketHandlerStorage storage mhs = MarketHandlerLib.marketHandlerStorage();

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

    // Make sure the sale exists
    Sale storage sale = mhs.sales[_consignmentId];
    require(sale.start != 0, "Sale does not exist");

    // Make sure we can accept the buy order
    require(block.timestamp >= sale.start, "Sale hasn't started");
    require(!AddressUpgradeable.isContract(msg.sender), "Contracts may not buy");
    require(_amount <= sale.perTxCap, "Per transaction limit for this sale exceeded");
    require(msg.value == sale.price * _amount, "Payment does not cover order price");

    // If this was the first successful purchase...
    if (sale.state == State.Pending) {

        // First buy updates sale state to Running
        sale.state = State.Running;

        // Notify listeners of state change
        emit SaleStarted(_consignmentId);

    }

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

        // Issue an escrow ticket to the buyer
        address escrowTicketer = getMarketController().getEscrowTicketer(_consignmentId);
        IEscrowTicketer(escrowTicketer).issueTicket(_consignmentId, _amount, payable(msg.sender));

    } else {

        // Release the purchased amount of the consigned token supply to buyer
        getMarketController().releaseConsignment(_consignmentId, _amount, msg.sender);

    }

    uint256 pendingPayoutValue = consignment.pendingPayout + msg.value;
    getMarketController().setConsignmentPendingPayout(consignment.id, pendingPayoutValue);

    // Announce the purchase
    emit Purchase(consignment.id, msg.sender, _amount, msg.value);

    // Track the sale info against the token itself
    emit TokenHistoryTracker(consignment.tokenAddress, consignment.tokenId, msg.sender, msg.value, _amount, consignment.id);
}

Recommendation:

We advise this trait of the system to be evaluated and proper validation to be imposed as otherwise unfulfill-able tickets may be created.

[JayWelsh]

In addition to this, I am thinking to add a check into the releaseConsignment function which makes sure that _amount <= remainingSupply in the case of a multiToken (ERC1155), to ensure that a consignment is only ever able to release the supply associated with it (in order to prevent someone trying to release tokens with the same ID which are part of a different consignment).

[JayWelsh]

Proposing to fix this in a slightly different way to the suggested fix (to ensure that the checks are done within the releaseConsignment and issueTicket functions, instead of doing checks adjacently to calls made to those functions.

i.e. https://github.com/seen-haus/seen-contracts/pull/42/commits/138d2ea0b2963956c380bd2af8f0b9d2b2b01fa6

[JayWelsh]

Initial change increased contract size beyond max

[JayWelsh]

Max contract size violation resolved by https://github.com/seen-haus/seen-contracts/pull/54