fix(MarketUtilsV2): pay out full remaining amount on indivisible ETH splits#24
Open
charlescrain wants to merge 1 commit into
Open
fix(MarketUtilsV2): pay out full remaining amount on indivisible ETH splits#24charlescrain wants to merge 1 commit into
charlescrain wants to merge 1 commit into
Conversation
…splits payoutWithMarketplaceFee floored each split recipient's share but passed the un-floored remainingAmount as the total to performPayouts. When the floored amounts don't sum back to remainingAmount (the common case for non-round amounts, e.g. 1%-increment auction bids), Payments.payout's require(msg.value == sum(amounts)) reverts on the ETH path. For RareBatchAuctionHouse.settleAuction this is unrecoverable: the winning bid and NFT are escrowed and settlement is the only release path, so the funds and token are locked permanently. RareBatchListingMarketplace. buyWithMerkleProof hits the same revert (recoverable by re-registering). v1 MarketUtils.payout passed the summed amounts; v2 regressed this. Fix: assign the rounding remainder to the last recipient so the per- recipient amounts sum to remainingAmount exactly, distributing the full proceeds with no dust stranded in the contract. Also adds upgrade scripts to swap the implementation behind each UUPS proxy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
MarketUtilsV2.payoutWithMarketplaceFeecomputes each split recipient's cut by flooring:but passes the un-floored
remainingAmountas the total toperformPayouts. On the ETH path,Payments.payoutenforcesrequire(msg.value == sum(amounts)). Since integer division drops the remainder, the floored amounts almost always sum to less thanremainingAmount, and the call reverts withpayout::not enough sent.This triggers in the common case, not an edge case: after marketplace fee, primary commission, and royalties are subtracted,
remainingAmountis an arbitrary wei value, and any multi-recipient split (or a single recipient whose ratio doesn't divide cleanly) leaves a rounding remainder. Auction bids use a 1% minimum increment (minimumBidIncreasePercentage = 1), so winning bids are essentially never round.Impact
RareBatchAuctionHouse.settleAuction— unrecoverable. The winning bid's ETH and the NFT are escrowed and settlement is the only release path, so funds and token are locked permanently.RareBatchListingMarketplace.buyWithMerkleProof— same revert; recoverable by re-registering with a single clean split.The bug is ETH-only — the ERC20 branch of
performPayoutstransfers per-recipient and ignores the total, which is why the existing (ERC20-based) settle tests never caught it.This is a regression: v1
MarketUtils.payoutpassed the summed amounts; v2 passedremainingAmount.Fix
Assign the rounding remainder to the last recipient so the per-recipient amounts sum to
remainingAmountexactly — distributing the seller's full proceeds with no dust stranded in the contract.Tests (TDD)
test_payout_multipleSplits_indivisibleAmount(odd-wei amount, 50/50 ETH split). Confirmed it failed withpayout::not enough sentbefore the fix and passes after.MarketUtilsV2tests pass; all 113 auction + marketplace tests pass.Deployment
Both contracts are UUPS proxies, so this ships as an implementation swap with no state migration. Added owner-broadcast upgrade scripts:
script/auctionhouse/rare-batch-auctionhouse-upgrade/(env:PRIVATE_KEY,RARE_BATCH_AUCTIONHOUSE)script/marketplace/rare-batch-listing-marketplace-upgrade/(env:PRIVATE_KEY,RARE_BATCH_LISTING_MARKETPLACE)Each deploys a fresh implementation and calls
upgradeTo(newImpl)on the proxy.🤖 Generated with Claude Code