MHB-03M: Non-Standard Application of Escrow Fee

[JayWelsh]

MHB-03M: Non-Standard Application of Escrow Fee

Type	Severity	Location
Logical Fault	Minor	MarketHandlerBase.sol:L255, L273, L277, L287

Description:

The deductEscrowAgentFee function does not validate that the total applied fee does not exceed the total available amount, causing "correct" configuration of the contract to lead to a failure.

Example:

function deductEscrowAgentFee(Consignment memory _consignment, uint256 _grossSale, uint256 _netAfterRoyalties)
internal
returns (uint256 net)
{
    // Only pay royalties on secondary market sales
    uint256 escrowAgentFeeAmount = 0;
    if (_consignment.market == Market.Secondary) {
        // Get the MarketController
        IMarketController marketController = getMarketController();
        address consignor = marketController.getConsignor(_consignment.id);
        if(consignor != _consignment.seller) {
            uint16 escrowAgentBasisPoints = marketController.getEscrowAgentFeeBasisPoints(consignor);
            if(escrowAgentBasisPoints > 0) {
                // Determine if consignment is physical
                address nft = marketController.getNft();
                if (nft == _consignment.tokenAddress && ISeenHausNFT(nft).isPhysical(_consignment.tokenId)) {
                    // Consignor is not seller, consigner has a positive escrowAgentBasisPoints value, consignment is of a physical item
                    // Therefore, pay consignor the escrow agent fees
                    escrowAgentFeeAmount = getPercentageOf(_grossSale, escrowAgentBasisPoints);

                    // If escrow agent fee is expected...
                    if (escrowAgentFeeAmount > 0) {
                        payable(consignor).transfer(escrowAgentFeeAmount);
                        // Notify listeners of payment
                        emit EscrowAgentFeeDisbursed(_consignment.id, consignor, escrowAgentFeeAmount);
                    }
                }
            }
        }
    }

    // Return the net amount after royalty deduction
    net = _netAfterRoyalties - escrowAgentFeeAmount;
}

Recommendation:

We advise the escrow fee to instead be applied on the after-royalty amount to ensure that the statements will always execute properly. Alternatively, we advise the code to gracefully handle the royalty and escrow agent fee exceeding the total amount by introducing a new maximum escrow agent fee to the system.

[JayWelsh]

We will need to apply all fee deductions to the total sale/auction value (i.e. we can't apply %s to the amounts after each % has been deducted as that means that parties involved may receive amounts that do not match up to their allocated % share of the total).

[JayWelsh]

Attempted resolution: https://github.com/seen-haus/seen-contracts/pull/57

1. Imposes a new max escrowAgentFee of 50% (this value can only get set by the ADMIN role and would generally be much lower than 50%, usually this value would likely be a single-digit percentage value, but the upper limit is now 50%).
2. The disburseFunds process now runs a check in the deductFee & deductEscrowAgentFee steps to ensure that neither fee payout would exceed the remaining amount of funds left to distribute as part of the disburseFunds process.
3. Some require statements in SaleRunnerFacet.sol needed to be batched to prevent the contract size exceeding max size.

[JayWelsh]

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