Coinbase: Sonar SettlementSale v2

Description

cancelBid() processes all wallet/token pairs for an entity atomically in a single transaction. For each non-zero pair it calls _reduceCommitment(), which performs a safeTransfer to the wallet. If any single transfer reverts, the entire cancelBid() transaction reverts, leaving all committed funds untouched and the entity unable to cancel.

A realistic trigger is a USDC or USDT address blacklist. If any wallet associated with an entity is blacklisted by the token issuer between commitment and cancellation stages, safeTransfer to that wallet will revert. Because cancelBid() has no mechanism to skip or isolate a failing pair, the entity is permanently unable to exercise full cancellation.

With reduceCommitmentEnabled = false (the default), there is no alternative user-facing path. With reduceCommitmentEnabled = true, the entity can use reduceCommitment() to recover individual pairs that are not blocked, but the blacklisted wallet/token pair remains unrecoverable through user-facing functions.

The only admin recourse is recoverTokens(), which requires the TOKEN_RECOVERER_ROLE and transfers tokens to an arbitrary to address with no knowledge of the original committed wallet or amount. Critically, recoverTokens() performs a raw safeTransfer with no state updates. After the call, walletState.committedAmountByToken, _totalCommittedAmountByToken, and state.currentBid.amount all still reflect the original committed amount. The entity is left in a permanently inconsistent state: the tokens are gone from the contract but the accounting still records them as committed. Admin intervention via recoverTokens() fixes the stuck funds but corrupts the accounting, i.e. there is no clean recovery path.

Note that _refund() in the Done stage has been acknowledged as having this same structural issue.

Recommendation

Consider a dedicated admin function that refunds a specific wallet/token pair and correctly updates all associated accounting state to provide a clean and auditable recovery path that recoverTokens() cannot offer.

Coinbase

We added an admin function forceReduceCommitment gated by COMMITMENT_REDUCER_ROLE that performs a targeted refund with proper state updates. Unlike recoverTokens(), this function correctly decrements committedAmountByToken, _totalCommittedAmountByToken, and currentBid.amount.

Cantina

Fixed as recommended.

Description

The contract emits WalletRefunded and EntityRefunded exclusively from _refund(), which is only reachable via the Done stage through processRefunds() or claimRefund(). These events were previously the canonical signal that funds had been returned to an entity.

Prior to this PR, cancelBid() called _refund() internally, so a cancellation during the Cancellation stage also produced WalletRefunded and EntityRefunded events. This PR refactors cancelBid() to call _reduceCommitment() directly, which only emits CommitmentReduced. As a result, an entity that fully cancels during the Cancellation stage triggers no WalletRefunded or EntityRefunded events at any point in the sale lifecycle, including later in the Done stage, where _refund() is blocked by state.refunded = true.

Any offchain system that tracks funds returned to users by indexing WalletRefunded and EntityRefunded events will silently undercount. The gap is not detectable from those events alone . A fully-cancelled entity simply produces no refund events, which is indistinguishable from an entity that has not yet been processed. The total funds returned to users now requires combining two disjoint event sets: CommitmentReduced events (Cancellation stage) and WalletRefunded events (Done stage). Neither event set alone gives a complete picture.

This also affects totalRefundedAmount(), which only reflects Done stage refunds and does not account for Cancellation stage cancellations. The true total of funds returned to users is totalRefundedAmount() + totalCancelledAmount(), but no combined view is exposed.

Recommendation

Consider:

1. Emitting WalletRefunded and EntityRefunded from cancelBid() alongside CommitmentReduced, preserving the existing event-based accounting contract for offchain systems. Alternatively,
2. Exposing a combined view function that sums totalRefundedAmount() and totalCancelledAmount() so that the total funds returned to users is queryable without event reconstruction.

Coinbase

CommitmentReduced and WalletRefunded/EntityRefunded represent semantically distinct operations (voluntary cancellation vs. post-settlement refund of unallocated amounts) and we prefer to keep them separate. Offchain systems should index both event types to compute total funds returned. Unifying the events would also make the distinction between totalRefundedAmount() and totalCancelledAmount() inconsistent.

Cantina

Acknowledged.

Description

setReduceCommitmentEnabled() is a SALE_MANAGER_ROLE-controlled toggle that can be flipped on or off at any time, including during the active Cancellation stage. The function carries no stage restriction and no time-lock.

reduceCommitment(), the partial reduction path gates entirely on this flag. By contrast, cancelBid() (full cancellation) is unconditionally available during Cancellation regardless of the flag.

The design introduces two fairness concerns:

1. Selective access window. The manager can enable partial reductions for a narrow window, then disable the toggle. Participants who are not actively monitoring onchain state or who have not been pre-informed may miss the window entirely, while others benefit from it.
2. Race between toggle-off and openSettlement(). The Cancellation stage exists specifically to let participants react to preliminary allocation information communicated offchain. Because the manager can disable partial reductions and immediately call openSettlement(), a participant who receives their preliminary allocation and decides to partially exit can be front-run by these two manager actions before they can transact, locking them into a larger position than intended.

However, the worst case is bounded: full cancellation via cancelBid() always remains available, so participants can never be forced to accept an unwanted allocation outright. The concern is specifically with partial reductions, which allow an entity to stay in the sale at a reduced size rather than exit entirely.

Recommendation

Consider one or more of the following mitigations:

1. Restrict the toggle to pre-Cancellation. Only allow setReduceCommitmentEnabled() to be called before the Cancellation stage opens. This makes the policy known and fixed before participants need to act on it.
2. Allow the toggle to be enabled only once. Once reduceCommitmentEnabled is set to true, prevent it from being disabled again. This ensures that once the window opens, all participants have equal access to partial reductions for the remainder of the Cancellation stage.
3. Announce in advance. If the toggle must remain flexible, document and enforce offchain that any state change will be announced with a minimum notice period before taking effect.
4. Remove the toggle entirely. If partial reductions are intended to be a standard feature of the Cancellation stage, always allow them and remove the flag. This eliminates the asymmetry between full and partial cancellation.

Coinbase

The fundamental trust assumption of the sale lies with the owner/admin, who already has the authority to skip the Cancellation stage entirely and set allocations directly. Partial cancellation toggle manipulation is strictly less impactful than these existing trusted capabilities.

Cantina

Acknowledged.

Description

During initialization, init.admin is unconditionally granted every operational role in the contract:

1. DEFAULT_ADMIN_ROLE: grants/revokes roles, withdraws proceeds, sets proceeds receiver, force-sets stage via unsafeSetStage()
2. SALE_MANAGER_ROLE: controls stage transitions, toggles, and operational parameters
3. SETTLER_ROLE: sets allocations for all entities
4. SETTLEMENT_FINALIZER_ROLE: finalizes settlement and moves sale to Done
5. PAUSER_ROLE: pauses the sale
6. REFUNDER_ROLE: processes refunds for any entity

The comment at line 491 explicitly frames this as intentional: // the admin should have all operational roles by default. The default state therefore concentrates all authority in a single address, which is risky.

Example Scenario: A sale is deployed with a single init.admin key managed by the sale operator. During the Cancellation stage, while participants are reviewing their preliminary allocations, the admin key is compromised. The attacker then executes the following steps:

1. Calls setProceedsReceiver() to redirect proceeds to an attacker-controlled address
2. Calls unsafeSetStage(Stage.Settlement), immediately ending the Cancellation stage
3. Calls setAllocations() to set every participant's allocation equal to their full committed amount, maximizing proceeds and leaving nothing for refunds
4. Calls finalizeSettlement() to lock in the allocations and move to Done
5. Calls withdraw() to drain all committed proceeds to the attacker-controlled address

Recommendation

Consider the following mitigations:

1. Enforce role separation at initialization. Require distinct addresses for operationally independent roles, specifically SETTLER_ROLE, SETTLEMENT_FINALIZER_ROLE, and SALE_MANAGER_ROLE, rather than defaulting all to init.admin. This forces the deployer to make an explicit decision about trust boundaries.
2. Restrict DEFAULT_ADMIN_ROLE to role administration only. The DEFAULT_ADMIN_ROLE holder currently also controls proceeds withdrawal and unsafeSetStage(). These capabilities should be gated on separate roles so that the role administrator cannot unilaterally influence sale outcomes.
3. Renounce or timelock DEFAULT_ADMIN_ROLE after initialization. Since a sale has a fixed lifecycle, DEFAULT_ADMIN_ROLE should be renounced post-deployment if role membership is static. If key rotation must remain possible, transfer it to a timelock so any role change is subject to a mandatory delay, giving participants time to observe and exit.
4. Gate unsafeSetStage() behind a multisig and timelock. This function is the most dangerous capability in the contract as it allows any stage constraint to be circumvented by a single key.

Coinbase

Granting all roles to init.admin at initialization is intentional for deployment simplicity. It's up to sale owners to define their own operational security and redistribute roles accordingly post-initialization. The contract provides the full role-based access control machinery via OpenZeppelin's AccessControlEnumerable for owners to configure as they see fit. The initialization state is transient and doesn't represent the operational role distribution.

Cantina

Acknowledged.

Description

Before PR #9, cancelBid() called _refund(), which computed the refund as committedAmountByToken - acceptedAmountByToken. Even if unsafeSetStage() moved the sale backward to Cancellation from Settlement or Done, _refund() would only return the unallocated portion. The accepted amount was protected.

for (uint256 i = 0; i < wallets.length; i++) {            WalletState storage walletState = state.walletStates[wallets[i]];            for (uint256 j = 0; j < numTokens; j++) {                IERC20 token = _paymentTokens[j];                uint256 refundAmount =                    walletState.committedAmountByToken[token] - walletState.acceptedAmountByToken[token];
                // nothing to refund                if (refundAmount == 0) {                    continue;                }

After PR #9, cancelBid() calls _reduceCommitment(), which refunds the full committedAmountByToken with no awareness of acceptedAmountByToken. If unsafeSetStage() moves the sale back to Cancellation after allocations have been set in Settlement, an entity can call cancelBid() and recover their full committed amount, including the portion already marked as accepted proceeds.

function _reduceCommitment(bytes16 entityID, address wallet, IERC20 token, uint256 amount) internal {        EntityState storage state = _entityStateByID[entityID];
        if (!state.wallets.contains(wallet)) {            revert WalletNotAssociatedWithEntity(wallet, entityID);        }
        if (!_isValidPaymentToken[token]) {            revert InvalidPaymentToken(address(token));        }
        if (amount == 0) {            revert ZeroAmount();        }
        WalletState storage walletState = state.walletStates[wallet];        if (walletState.committedAmountByToken[token] < amount) {            revert ReductionExceedsCommitment(                entityID, wallet, address(token), amount, walletState.committedAmountByToken[token]            );        }

Example scenario:

1. Entity committed 5000 USDC. Settlement sets acceptedAmountByToken = 3000.
2. Admin calls unsafeSetStage(Cancellation) for an emergency correction.
3. Entity calls cancelBid(). _reduceCommitment() transfers 5000 USDC back to the entity.
4. 3000 USDC that was designated as sale proceeds is drained. The contract's token balance no longer covers its obligations.

The precondition is DEFAULT_ADMIN_ROLE calling unsafeSetStage(), which is documented as exceptional. But the old code handled this safely as a byproduct of _refund() design. The new code is a regression in that safety property.

Recommendation

It is recommended to consider the acceptedAmountByToken when processing refunds in _reduceCommitment().

Coinbase

We added a defensive check in _reduceCommitment() that caps the refundable amount at committedAmountByToken - acceptedAmountByToken.

Cantina

Fixed as recommended.

Description

replaceBidWithPermit() uses amountDelta as value of ptoken.permit().

function replaceBidWithPermit(        IERC20 token,        Bid calldata bid,        PurchasePermitV3 calldata purchasePermit,        bytes calldata purchasePermitSignature,        uint256 erc20PermitDeadline,        bytes calldata erc20PermitSignature    ) external onlyStage(Stage.Commitment) onlyUnpaused {        uint256 amountDelta = _processBid(token, bid, purchasePermit, purchasePermitSignature);        if (amountDelta > 0) {            // Permit signatures can be grabbed from the mempool, allowing attackers to execute them before the actual bid is placed,            // which will cause the call to `ptoken.permit` to revert.            // The sale contract should be able to handle this gracefully and not revert when the bid transaction is eventually included.            // To do this, we wrap the call to `ptoken.permit` in a try-catch block and ignore the revert. This will also ignore any other errors,            // which is fine because this method effectively just becomes equivalent to `replaceBidWithApproval`.            IERC20Permit ptoken = IERC20Permit(address(token));            try ptoken.permit({                owner: msg.sender,                spender: address(this),                value: amountDelta,                deadline: erc20PermitDeadline,                r: bytes32(erc20PermitSignature[0:32]),                s: bytes32(erc20PermitSignature[32:64]),                v: uint8(bytes1(erc20PermitSignature[64]))            }) {}                catch {}
            token.safeTransferFrom(msg.sender, address(this), amountDelta);        }

It is worth noting that if another wallet of the same entity frontruns the transaction (for example, by slightly increasing currentBid.amount), amountDelta will not be as expected.

Since the value contained in the erc20PermitSignature is already determined, this will cause ptoken.permit() to fail, and replaceBidWithPermit() will fail due to insufficient approval.

Recommendation

It is recommended to allow users to provide the erc20PermitValue parameter in replaceBidWithPermit() instead of using the amountDelta, which may change.

Or just document the issue to inform users.

Coinbase

The scenario requires two wallets of the same entity submitting bids concurrently, which is uncommon in practice. No funds are at risk as the transaction reverts cleanly. Users encountering this can retry or use replaceBidWithApproval(), which is not subject to this constraint. Adding a separate erc20PermitValue parameter would be a breaking change to the interface, which we'd like to avoid at this stage.

Cantina

Acknowledged.

Description

cancelBid() and _refund() both iterate over all wallet/token pairs and transfer committed funds back to the entity. However, they handle the state.refunded flag differently:

1. _refund() sets state.refunded = true immediately at entry, before any loop or transfer.
2. cancelBid() never sets state.refunded = true directly. It relies on _reduceCommitment() to set the flag, which only does so when state.currentBid.amount reaches zero in the last iteration of the loop.

For entities with a single wallet and a single payment token, there is only one iteration and so state.refunded is set before the transfer and the behavior is equivalent. For entities with multiple wallets or tokens, state.refunded remains false across all but the final transfer. This is an inconsistency in how two structurally identical functions handle the same guard, and deviates from the established safe pattern in the codebase.

In the current deployment configuration, where payment tokens are standard stablecoins with no transfer callbacks, there is no exploitable path. The risk is latent: if a payment token with a transfer callback is ever added, the unprotected window between the entry guard check and the final state.refunded = true write would be active across all intermediate transfers, allowing a reentrant call to pass the AlreadyRefunded guard.

Recommendation

Consider setting state.refunded = true at the entry of cancelBid(), before the transfer loop, matching the pattern already established in _refund(). The if (state.currentBid.amount == 0) { state.refunded = true; } check inside _reduceCommitment() can remain as-is because it is still needed for the reduceCommitment() path, where an entity that fully reduces its commitment via that function must also be marked as refunded.

Coinbase

On the pattern consistency point: we considered setting state.refunded = true at the entry of cancelBid(), as _refund() does. We chose not to because _reduceCommitment() is shared by both cancelBid() and reduceCommitment(), and already sets the flag when the full commitment reaches zero. Adding another write in cancelBid() would spread responsibility for this flag across multiple call sites rather than keeping it in one place. We prefer _reduceCommitment() as the single authoritative location. It also preserves that cancelBid() and reduceCommitment(allCommitments) are functionally equivalent, which would break otherwise.

The latent reentrancy concern is mitigated by the check-effects-interactions pattern already upheld within _reduceCommitment(): committed amounts are decremented before each transfer, so any reentrant invocation of cancelBid() would find zero for already-processed pairs and skip them. No double-spending or misappropriation of funds is possible under current token configurations or with callback tokens.

Cantina

Acknowledged.

Description

During the Commitment stage, _processBid() enforces that newBid.amount >= purchasePermit.minAmount, ensuring every entity's committed amount meets the minimum participation threshold specified in their purchase permit. This constraint is issued offchain by Sonar and validated onchain at bid time.

minAmount is not stored onchain after the bid is accepted. It exists only in the purchase permit, which is consumed at bid submission and not referenced again. As a result, once the sale moves to the Cancellation stage, there is nothing preventing an entity from using reduceCommitment() to bring their currentBid.amount below the minAmount that was enforced when they committed.

Example scenario:

1. A purchase permit specifies minAmount = 5000e6.
2. The entity commits 8000e6, which passes the bid check.
3. During the Cancellation stage, the entity calls reduceCommitment() with 4000e6, reducing currentBid.amount to 4000e6, a value that would have been rejected at bid time. The contract accepts this without error.

If minAmount is a Commitment-stage constraint and the Cancellation stage is explicitly designed to allow entities to reduce their exposure then it is an implicit assumption that should be documented clearly. The offchain settlement system, which computes allocations based on remaining committed amounts, must be aware that entities can hold commitments below their original minAmount at settlement time and handle that case explicitly. If the offchain system assumes all remaining commitments are above minAmount, for example, to apply a minimum allocation floor then it will produce incorrect results for partially-reduced entities.

Recommendation

Consider:

1. Documenting explicitly that minAmount is enforced only at bid submission time and does not represent a floor on the committed amount through the rest of the sale lifecycle.
2. Having the offchain settlement system treat currentBid.amount as the authoritative committed amount at settlement time regardless of the original minAmount, and not assume any minimum floor on remaining commitments when computing allocations.

Coinbase

This is intentional behaviour. The minAmount is mainly a commitment-stage gate, limiting the number of participants by enforcing a minimum participation threshold when entities enter the sale. This isn't relevant in the Cancellation stage, so we allow entities to reduce their commitment by any amount for simplicity. We added natspec to reduceCommitment() and _processBid() clarifying that minAmount is enforced only at bid submission and doesn't constrain subsequent reductions.

Regarding the offchain settlement concern: the settlement system already computes allocations solely from the currentBid.amount values read from the contract at settlement time. It doesn't reference or assume any relationship to the original minAmount from the purchase permit. If a post-reduction minimum is needed, the settlement system can handle it by opting not to grant any allocation to entities that reduce their commitment below some minimum communicated off-chain.

Cantina

Acknowledged.

Description

_setAllocation() checks entityState.refunded and reverts with AlreadyRefunded if the flag is set. Prior to this change, refunded was only set during the Done stage via _refund(), so an entity entering the Settlement stage was guaranteed to have refunded = false. The separate cancelled flag tracked Cancellation stage exits.

This change removes the cancelled flag and repurposes refunded to cover both outcomes: an entity that fully cancels during the Cancellation stage now has refunded = true before Settlement begins. The _setAllocation() check, which was previously safe to apply to all entities entering Settlement, now rejects any entity that cancelled.

The consequence is that setAllocations(), which the settler calls in batches covering all participating entities, will revert if even a single cancelled entity is included in a batch. The settler's offchain system presumably computes allocations from commitment data and constructs these batches. Previously it could include all entities without concern; now it must independently track which entities cancelled during the Cancellation stage and filter them out before constructing each batch. Since cancelled no longer exists as a distinct onchain flag, the settler must derive this from event history (CommitmentReduced events where the entity's currentBid.amount reached zero) rather than from a simple state read.

Example scenario: 500 entities participate in a sale. During the Cancellation stage, 50 entities fully cancel. The settler's offchain system, unaware of the semantic change to refunded, constructs allocation batches from all 500 entities. The first batch containing any of the 50 cancelled entities reverts entirely. The settler must identify all cancelled entities from event history, reconstruct the batches excluding them, and resubmit while the sale is in the Settlement stage awaiting finalization.

Recommendation

Consider:

1. Restoring the cancelled field on EntityState, set when currentBid.amount reaches zero during the Cancellation stage, allowing the settler to distinguish cancelled entities from those requiring allocation via a simple state read rather than event reconstruction. Alternatively,
2. Having _setAllocation() skip rather than revert for fully-cancelled entities, with an accompanying event, so that a batch containing cancelled entities degrades gracefully rather than failing entirely.

Coinbase

The revert is a desirable safety check that prevents accidentally setting allocations for entities that have fully cancelled. The offchain systems construct allocation batches from live contract state. Filtering out entities with zero remaining commitment is straightforward and already implemented, since unset allocations default to zero anyway. We prefer the explicit revert over silent skipping because it surfaces batch construction errors rather than masking them.

Cantina

Acknowledged.

Description

The Closed stage previously provided a stable intermediate state between the Commitment and Cancellation/Settlement stages. Two useful operational safety capabilities are lost with its removal:

1. No way to halt new bids without also halting cancellations

The Closed stage blocked new bids while leaving all other contract functionality unaffected. The only remaining mechanism to prevent new bids now is pause(), which applies the onlyUnpaused modifier globally. This also blocks cancelBid() and reduceCommitment() as a side effect. If the manager needs to freeze bidding to review commitment data before deciding on the next stage, they must now either accept that new bids may arrive during the review window, or block participants from exercising their cancellation rights while the review is in progress. These were previously independent concerns.

2. Premature openCancellation() is irreversible without unsafeSetStage()

Previously, a premature closeCommitment() was recoverable. The manager could call openCommitment() from the Closed stage to reopen bidding. openCommitment() now only accepts the PreOpen stage, making any transition out of Commitment permanent through normal functions. If openCancellation() is called before the intended window closes, participants can immediately begin cancelling bids. The only recovery path is unsafeSetStage(), an emergency function that carries its own risks. The Closed stage acted as a reversible intermediate step; without it, Commitment stage transitions are one-way under normal operation.

Recommendation

Consider:

1. Restoring a mechanism to pause new bids independently of cancellations, for example, a dedicated bidsEnabled flag checked in _processBid(), so that the manager can freeze the commitment surface without affecting participant withdrawal rights.
2. Documenting explicitly that openCancellation() and openSettlement() are one-way transitions under normal operation and that operators should verify commitment data is final before calling either function.

Coinbase

Bidding and cancellation are gated by separate, mutually exclusive stages (Commitment and Cancellation respectively). So pausing during the respective stages only affects the current functionality, so the concern about pause() blocking cancellations while trying to freeze bids doesn't apply given this stage separation, assuming sale operators unpause the contract when transitioning stages. One-way stage transitions under normal operation are intentional and provide stronger guarantees to participants about lifecycle progression.

Cantina

Acknowledged.

Description

As designed for partial reduction, reduceCommitment() accepts a caller-supplied list of (wallet, token, amount) tuples and processes each entry against live storage without verifying that the list is exhaustive. If any wallet/token pair with a non-zero committed balance is omitted, the transaction succeeds and the entity is left in a partially reduced state with no onchain indication that the exit was incomplete.

cancelBid(), by contrast, reads all wallets and payment tokens directly from contract storage and cannot miss any pair.

An entity that wants to verify exactly what they are exiting, particularly after a complex series of partial reductions across multiple wallets and tokens, may construct an explicit list for reduceCommitment() instead of using cancelBid(). This verification-oriented approach is what makes the incompleteness risk non-obvious: the entity believes they have carefully enumerated every remaining position, when in practice a missed or stale entry causes the transaction to succeed with a residual committed balance left behind.

Example scenario:

1. An entity has two wallets, W1 and W2, each with 1,000 USDC committed (total bid: 2,000 USDC).
2. During the Cancellation stage, they partially reduce W1 by 500 USDC via reduceCommitment(), bringing the total bid to 1,500 USDC. They then decide to fully exit.
3. Wanting to control the exact amounts being returned, they call reduceCommitment() again with [(W1, USDC, 500), (W2, USDC, 1000)], intending to clear all remaining commitments.
4. However, their client constructed the list from a cached state and silently dropped the W2 entry.
5. The call succeeds, W1's balance reaches zero, but W2's 1,000 USDC remains committed. currentBid.amount is 1,000 rather than 0, so refunded stays false. The entity has no indication anything went wrong.
6. When openSettlement() is subsequently called, the settler sees W2's 1,000 USDC and allocates accordingly, leaving the entity with an unwanted position they believed they had exited.

Recommendation

Consider:

1. Documenting frontend interface to clearly distinguish the two functions: cancelBid() is the safe, complete path for full cancellation; reduceCommitment() is only for intentional partial reductions.
2. Validating completeness onchain. reduceCommitment() could accept an optional complete flag asserting that no committed balance should remain after the call, reverting if any wallet/token pair for the entity still has a non-zero committed amount.
3. Emitting a warning event on partial reduction. If currentBid.amount > 0 after processing the list, emit a distinct event to make the partial state visible to offchain monitors and the entity itself.

Coinbase

This is by design. reduceCommitment() accepts caller-supplied wallet/token/amount tuples for intentional partial reductions. cancelBid() is the safe, exhaustive path for full cancellation, reading all wallets and payment tokens directly from contract storage. The scenario described requires a client that silently drops entries from the reduction list, which is a client-side integration concern. Entities can verify their remaining commitment via public view functions before the Cancellation stage ends.

We improved the natspec on both functions to make the distinction and the incompleteness risk explicit: reduceCommitment() now documents that omitted pairs are left unchanged and carried into settlement, and cross-references cancelBid() as the exhaustive alternative (and vice versa).

Cantina

Acknowledged.

Description

The contract resolves authorization for both cancelBid() and reduceCommitment() by mapping msg.sender to an entityID, and then permitting the caller to act on any wallet that belongs to that same entity. For cancelBid(), this means any single wallet can atomically cancel every other wallet's committed position. For reduceCommitment(), any wallet can specify arbitrary reductions across other wallets' committed balances. In both cases, funds are returned to the respective committing wallet, not to the caller.

This design assumes all wallets grouped under an entityID represent a single coordinated unit operating with mutual trust. However, entityID groupings are determined offchain by the Sonar system, and in practice an entity may have wallets operated by independent sub-units, for example, distinct legal entities, departments, or custodians that share an entityID but do not have authority over each other's positions.

In a multi-wallet entity where wallets are operated independently, any one wallet can:

1. Fully cancel the committed positions of all other wallets in the entity via a single cancelBid() call, forcing immediate refunds without the consent of the other wallets.
2. Selectively reduce the committed balance of any specific wallet via reduceCommitment(), partially or fully exiting another sub-unit's position In both cases the affected wallets receive their funds back, so there is no direct loss of funds. However, a sub-unit whose position is cancelled by another wallet loses its place in the sale and has no recourse once the Cancellation stage ends or Settlement opens. The harm is the loss of the intended allocation rather than the loss of capital.

Example scenario: Two subsidiary entities are grouped under a single entityID, each operating an independent wallet: W1 (500 USDC) and W2 (1,000 USDC). During the Cancellation stage, W1's operator, without consulting W2, calls reduceCommitment() with [(W2, USDC, 1000)], fully exiting W2's position while leaving its own intact. W2 receives its funds back but loses its intended allocation with no recourse.

Recommendation

Consider one or more of the following mitigations:

1. Document the trust assumption explicitly. Make clear in the contract documentation that all wallets grouped under an entityID are assumed to be fully trusted by one another. Operators should not group wallets belonging to independent sub-units under a single entityID if those sub-units have distinct allocation interests.
2. Require per-wallet authorization for cross-wallet reductions. reduceCommitment() could require that each reduction entry is either submitted by the wallet being reduced, or accompanied by an offchain signature from that wallet, rather than relying solely on entity-level authorization.
3. Restrict cancelBid() to the calling wallet's commitments only. Rather than cancelling all entity wallets, cancelBid() could cancel only the commitments of msg.sender, with a separate privileged function for full entity-wide cancellation.

Coinbase

Entity-level authorization is the intended design. All wallets under an entity are associated during the commitment process via purchase permits signed by the Sonar system. In both cancelBid() and reduceCommitment(), funds are always returned to the committing wallet, not to the caller, so no misappropriation is possible. Per-wallet authorization would add complexity and gas costs for a scenario that only results in an overreduction of an entity's commitment in the worst case, so we opted for simplicity here.

We added documentation to the contract-level natspec making the mutual trust assumption explicit.

Cantina

Acknowledged.

Description

The protocol performs address(0) checks on almost all addresses, except for extraManagers and extraPausers.

// grant extra roles        if (init.extraSettler != address(0)) {            _grantRole(SETTLER_ROLE, init.extraSettler);        }
        if (init.extraRefunder != address(0)) {            _grantRole(REFUNDER_ROLE, init.extraRefunder);        }
        for (uint256 i = 0; i < init.extraManagers.length; i++) {            _grantRole(SALE_MANAGER_ROLE, init.extraManagers[i]);        }
        for (uint256 i = 0; i < init.extraPausers.length; i++) {            _grantRole(PAUSER_ROLE, init.extraPausers[i]);        }

Recommendation

It is recommended to add address(0) check on extraManagers and extraPausers

// grant extra roles        if (init.extraSettler != address(0)) {            _grantRole(SETTLER_ROLE, init.extraSettler);        }        if (init.extraRefunder != address(0)) {            _grantRole(REFUNDER_ROLE, init.extraRefunder);        }        for (uint256 i = 0; i < init.extraManagers.length; i++) {+           require(init.extraManagers[i] != address(0),"...");            _grantRole(SALE_MANAGER_ROLE, init.extraManagers[i]);        }        for (uint256 i = 0; i < init.extraPausers.length; i++) {+           require(init.extraPausers[i] != address(0),"...");            _grantRole(PAUSER_ROLE, init.extraPausers[i]);        }

Description

cancelBid() constructs its inputs directly from contract storage - wallets from state.wallets.values() and tokens from _paymentTokens and then calls _reduceCommitment() for each pair. However, _reduceCommitment() re-validates all three of the following, which are already guaranteed in the cancelBid() path:

1. state.wallets.contains(wallet): wallets come from state.wallets.values()
2. _isValidPaymentToken[token]: tokens come from _paymentTokens
3. amount == 0: cancelBid() already checks if (amount > 0) before calling

These checks are meaningful in the reduceCommitment() path where inputs are caller-supplied, but add NxM redundant SLOADs per cancelBid() call for an entity with N wallets and M tokens.

Recommendation

Consider splitting _reduceCommitment() into two variants:

1. A checked version used by reduceCommitment() that validates caller-supplied inputs, and
2. An unchecked internal version used by cancelBid() that skips these guards entirely

Coinbase

We prefer maintaining a single _reduceCommitment() implementation shared by both cancelBid() and reduceCommitment(). Splitting into checked and unchecked variants introduces a second internal function that must be kept in sync, increasing the surface area for bugs. The associated gas overhead is acceptable for typical entity sizes.

Cantina

Acknowledged.

Description

PR 9 introduced walletState.cancelledAmountByToken[token] and _totalCancelledAmountByToken[token] for audit tracking. Both are updated on every call to _reduceCommitment(), and are noted in the code as being used for audit purposes only with no effect on control flow.

On a full cancelBid() across N wallets and M tokens, this results in up to 2×N×M cold SSTOREs. Additionally, _totalCancelledAmountByToken[token] is written once per wallet per token, even though it is keyed only by token, which means that the same global counter is updated redundantly across all N wallets for each token.

Recommendation

If the CommitmentReduced event already captures entity, wallet, token, and amount, consider whether the onchain cancelledAmountByToken and _totalCancelledAmountByToken storage fields are necessary for audit needs, because dropping them would eliminate the cold SSTORE overhead entirely.

If they must be retained, consider accumulating a local sum for _totalCancelledAmountByToken outside the per-wallet inner loop and write to the global counter once per token rather than once per wallet.

Coinbase

The cancelledAmountByToken and _totalCancelledAmountByToken storage fields serve the totalCancelledAmount() public view function, which our offchain systems use for accounting. The CommitmentReduced event captures the same data, but having on-chain queryable state means we don't need to rely on event indexing. The redundant write pattern for _totalCancelledAmountByToken (once per wallet per token rather than once per token) is a consequence of keeping a single _reduceCommitment() implementation shared by both cancelBid() and reduceCommitment(). Splitting into separate code paths or accumulating sums outside the loop would save gas but increases the surface area for bugs, which we'd rather avoid.

Cantina

Acknowledged.