Coinbase: Sonar SettlementSale

Description

There is an inconsistency in the validation of proceedsReceiver between the constructor and setProceedsReceiver(). The setProceedsReceiver() function properly validates that the new address is not address(0), but the constructor lacks this same validation check.

This inconsistency allows proceedsReceiver to be initialized to address(0) during deployment. As a result, if proceedsReceiver is set to address(0) in the constructor, subsequent calls to _withdrawPartial() will send tokens to address(0), permanently burning those funds.

Recommendation

Consider either one of the following fixes:

1. Add a zero address check in _withdrawPartial():

function _withdrawPartial(uint256 amount) internal {                                                                                                                       if (proceedsReceiver == address(0)) {                                                                                                                                      revert InvalidProceedsReceiver();                                                                                                                                  }                                                                                                                                                                      // ... rest of function                                                                                                                                            }

2. Add a zero address check in the constructor (Recommended)

Description

The _refund() function iterates through all supported payment tokens and transfers each back to the user's wallet. If any single token transfer reverts—due to blocklisting (e.g., USDC/USDT), or other internal token failures—the entire transaction reverts, effectively locking the user out of all their refundable funds, including tokens that would otherwise transfer successfully.

While recoverTokens() exists as an admin escape hatch, using it only recovers the raw tokens without updating the user's commitment state. This leaves the protocol in an inconsistent state, with the user's records showing funds that no longer exist in the contract.

Recommendation

There are multiple ways to handle this issue:

1. Extend the recoverTokens() function to adjust the internal state of the sale so that the state matches the overall refund.
2. Modify the _refund() function to do partial refunds without marking the state.refunded as valid, if the user is not fully refunded.

Alternatively, if you beleive the sonar onboarding will reduce the likelihood of user wallets getting blocked during the sale window, then add clear documentation about this risk of diversifying bids with different tokens.

Coinbase

Since the likelihood of this is extremely small and one can triage it in the worst case using recoverTokens, we don't implement dedicated logic for it for now. We'll think about including partial refunds in future iterations.

Description

The protocol assumes all payment tokens (e.g., USDC, USDT, DAI) maintain equal USD value. While the constructor enforces matching decimals, it does not guarantee value parity. This assumption is embedded throughout the contract and could lead to accounting inconsistencies if a stablecoin depegs.

Affected Code Paths include:

1. Bid Amount Delta Calculation: A user could bid 100 USDC, then increase to 150 using USDT. If USDT depegs to $0.95, the protocol treats a 50 USDT delta as 50 USDC.

uint256 amountDelta = newBid.amount - previousBid.amount;

2. Total Commitment Aggregation: Sums raw amounts across tokens without value weighting.

function totalCommittedAmount() external view returns (uint256) {                                                                                                          return _sumByToken(_totalCommittedAmountByToken);                                                                                                                  }

3. Total Accepted Amount: Same issue as above—proceeds calculation assumes parity.

function totalAcceptedAmount() public view returns (uint256) {                                                                                                         return _sumByToken(_totalAcceptedAmountByToken);                                                                                                                 }

Other areas of the code include settlement, which also assumes the token's value remains constant during the Auction-to-Settlement phases.

Recommendation

1. Document and Accept Risk
   Clearly document this assumption and establish operational procedures to pause/remove depegged tokens:

/// @notice CRITICAL ASSUMPTION: All payment tokens must maintain 1:1 USD parity.                                                                                    /// If a token depegs, the sale should be paused.

2. Add Token Removal Mechanism: Implement the ability to delist a payment token mid-sale.

3. Alternatively, consider developing an off-chain depeg watcher with capabilities to automatically pause the sale if a depeg happens.

Coinbase

This is communicated to projects. The header comment of the contract has been extended.

Description

SettlementSale concentrates significant authority in privileged roles. By default, DEFAULT_ADMIN_ROLE controls or can grant all critical roles. Users (both teams running their sales & bidders) must fully trust operator honesty and key security for the entire sale lifecycle.

1. Roles with fund control:

2. Roles that can cause operational failures:

Key trust assumptions include:

• Allocations are computed offchain with no onchain verification.
• No settlement deadline; funds can be locked indefinitely.
• No user-controlled emergency exit.
• Payment tokens are immutable post-deploy.
• A single compromised admin can escalate privileges and drain funds.

Recommendation

1. Multisig + Timelock all privileged roles, especially DEFAULT_ADMIN_ROLE, TOKEN_RECOVERER_ROLE, SETTLER_ROLE
2. Add settlement deadline with automatic user exit if exceeded
3. Restrict recoverTokens() to non-payment tokens only and introduce a new privileged function to recover user tokens (eg blacklist impact) by modifying state accordingly.
4. Implement allocation verification via Merkle proofs or commit-reveal
5. Default claimRefundEnabled = true to reduce refunder dependency

Coinbase

The centralisation is intentional and by design, since investors already place significant trust in the project to honour their purchases.

We recommend the use of a multisig (typically at least 3-of-5) for all projects launching with us. The emergency escape hatch functions, such as unsafeSetStage() and recoverTokens(), are intended as training wheels. In a future version, they may be modified to avoid interfering with internal bookkeeping or removed entirely. For now, we prefer to retain this additional safety net.

Description

Several state-changing functions throughout the contract do not emit events upon successful execution. Events are essential for:

• Off-chain monitoring: Enables real-time tracking of protocol activity by indexers, dashboards, and alerting systems
• Transparency: Provides an immutable audit trail of all significant state changes
• Integration: Allows external services (e.g., The Graph, Dune Analytics) to index and query historical protocol data
• Incident response: Facilitates forensic analysis and debugging when issues arise

Recommendation

Consider adding events to all state-changing functions listed above.

Description

The supportsInterface function includes the virtual keyword. The virtual keyword indicates that a function can be overridden by derived classes.

However, SettlementSale is a final contract not designed to be inherited. Including virtual on a function in a non-inheritable contract serves no purpose and adds unnecessary bytecode.

Recommendation

Consider removing the virtual keyword.

Description

The closeAuctionAtTimestamp parameter determines when the auction automatically transitions from Auction to Closed stage. This value is set in three instances without validating that it is either 0 (disabled) or a future timestamp:

1. Constructor:

closeAuctionAtTimestamp = init.closeAuctionAtTimestamp;

2. reopenAuction():

closeAuctionAtTimestamp = newCloseAuctionAtTimestamp;

The NatSpec even acknowledges this: "NB: if the newCloseAuctionAtTimestamp is in the past, the auction will be closed again immediately." for the reopenAuction() function.

3. setCloseAuctionAtTimestamp():

closeAuctionAtTimestamp = timestamp;

Recommendation

Consider enforcing uniform validation between the three instances:

1. In the constructor: allow setting either 0 or a future timestamp.
2. In setCloseAuctionAtTimestamp: allow setting either 0 or a current or future timestamp
3. In reopenAuction: allow setting a future timestamp, as it will be redundant if the function is invoked successfully without reopening an auction.

Coinbase

The automatic auction closing has been moved to timestamps on the permit.

Description

The constructor grants PURCHASE_PERMIT_SIGNER_ROLE to init.purchasePermitSigner without validating that it is not an address(0).

While OpenZeppelin's ECDSA library includes native address(0) validation during signature recovery (preventing address(0) from ever being accepted as a valid signer), deploying with an invalid signer address would result in a non-functional sale where all bids revert with UnauthorizedSigner(address(0)).

Failing at deployment time is preferable to failing at runtime when users attempt to bid.

Recommendation

Add explicit zero address validation in the constructor:

+ if (init.purchasePermitSigner == address(0)) {                                                                                                                     +     revert ZeroAddress();                                                                                                                                          + }

Description

The setCloseAuctionAtTimestamp() function can be called at any stage of the sale:

function setCloseAuctionAtTimestamp(uint64 timestamp) external onlyRole(SALE_MANAGER_ROLE) {                                                                           closeAuctionAtTimestamp = timestamp;                                                                                                                              }

This parameter is only relevant during PreOpen (before the auction starts) and Auction (while bids are being accepted) stages.

Modifying it during Closed, Cancellation, Settlement, or Done stages serves no functional purpose since:

1. The auction has already closed
2. Stage transitions past Closed are controlled by manualStage, not the timestamp
3. Could cause confusion in off-chain systems monitoring the contract state

Recommendation

Add stage restriction to limit when this function can be called:

- function setCloseAuctionAtTimestamp(uint64 timestamp) external onlyRole(SALE_MANAGER_ROLE) {                                                                       + function setCloseAuctionAtTimestamp(uint64 timestamp) external onlyRole(SALE_MANAGER_ROLE) onlyStages(Stage.PreOpen, Stage.Auction) {                                  closeAuctionAtTimestamp = timestamp;                                                                                                                           }

Description

The _trackEntity() function documentation states that an entity can use multiple addresses "up to maxAddressesPerEntity". However, there is no maxAddressesPerEntity variable defined in the contract, and no limit is placed on the number of wallets an entity can associate with.

Recommendation

If this limit is intended to be enforced by the off-chain permit signer, update the NatSpec comment accordingly. If on-chain enforcement was intended, consider adding it.

Description

Multiple-view functions iterate over all wallets associated with an entity without an upper bound. Since maxAddressesPerEntity is documented but not enforced on-chain, an entity can accumulate an arbitrary number of wallet addresses, causing these functions to exceed gas limits and revert.

Functions that could be directly impacted include:

Functions that are indirectly impacted include:

While view functions called via RPC do not consume user gas, they remain subject to:

1. Node gas limits - eth_call enforces gas limits
2. On-chain integrations - Any external contract calling these functions via the ICommitmentDataReader or IEntityAllocationDataReader interfaces will revert, breaking composability

Recommendation

Consider documenting the behavior, if unbounded wallets per entity is a requirement.

Description

The PurchasePermitV2Lib in PurchasePermitV2.sol uses a simple eth_sign style message hash without an EIP-712 domain separator.

Though the SaleUUID being unique for a sale protects the protocol against cross-chain replay attacks and same-chain cross-contract replay attacks, it is generally a good practice to include the chain_id and contract_address in the signature.

Recommendation

Consider enhancing the permit signature process by implementing EIP-712 structured data signing with a domain separator.

Coinbase

Since we already have strong replay protection with the UUID, we prefer to keep eth_sign over EIP-712 for simplicity, since the backend system does not know the contract address in every case. A comment about this will be added.

Description

The custom errors used in SettlementSale.sol have several consistency and completeness issues:

1. Inconsistent parameter naming: Errors use different conventions — got/want (InvalidSaleUUID),
   expected/actual (UnexpectedTotalAcceptedAmount), and new/previous (BidAmountCannotBeLowered).

2. Inconsistent token type: Some errors use address (InvalidPaymentToken), others use IERC20
   (WithdrawalExceedsAvailable, AllocationExceedsCommitment).

3. Missing context in key errors: InvalidStage(Stage) — only shows current stage, not what was expected, and PurchasePermitExpired() — no timestamps for debugging

Recommendation

Consider fixing the inconsistencies in the error messages mentioned above.

Description

Events in SettlementSale.sol have naming and type inconsistencies:

1. Inconsistent wallet/address naming: Some events use addr, others use wallet for the same concept.
2. Token type inconsistency: Events use an IERC20 indexed token, but indexed interface types log the address
   anyway. Using an address would be more explicit and consistent with errors.

Recommendation

Consider enforcing consistent naming & type pattern:

event EntityInitialized(bytes16 indexed entityID, address indexed wallet);                                    event WalletInitialized(bytes16 indexed entityID, address indexed wallet);                                    event BidPlaced(bytes16 indexed entityID, address indexed wallet, Bid bid);                                   event BidCancelled(bytes16 indexed entityID, address indexed wallet, uint256 amount);                         event AllocationSet(bytes16 indexed entityID, address indexed wallet, address indexed token, uint256          acceptedAmount);                                                                                              event WalletRefunded(bytes16 indexed entityID, address indexed wallet, address indexed token, uint256 amount);event ProceedsWithdrawn(address indexed receiver, address indexed token, uint256 amount);

Description

The SettlementSale.sol contract currently uses a constructor-based initialization pattern. While this approach works for standard contract deployments, it is incompatible with gas-efficient clone-factory patterns, such as EIP-1167 minimal proxies.

When deploying contracts via Clones.clone() or Clones.cloneDeterministic(), the implementation contract's constructor is not executed for cloned instances. Instead, clones require an explicit initialize() function that can be called post-deployment to set up contract state.

Given that the platform intends to deploy many instances of this sale contract, the current architecture would require full contract deployment for each sale, resulting in significantly higher gas costs than minimal proxy deployments.

Recommendation

Consider refactoring the contract to use OpenZeppelin's Initializable pattern alongside the existing constructor:

1. Inherit from @openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol
2. Convert the constructor logic into an initialize() function with the initializer modifier
3. Convert immutable variables (e.g., SALE_UUID) to regular state variables set during initialization

Cantina

The fixes look good, but they introduce additional risk assumptions, such as that saleUUID may be modified by an upgrade. In the earlier code versions, it was immutable and could not be updated. However, the saleUUID not being immutable is not an issue if the proxy isn't upgradeable (e.g., for simple clones).

Description

The PurchasePermitPayload struct wraps a single boolean field:

struct PurchasePermitPayload {                                                                                                                                         bool forcedLockup;                                                                                                                                               }

This struct is decoded in _processBid():

PurchasePermitPayload memory payload = abi.decode(purchasePermit.payload, (PurchasePermitPayload));                                                                  if (payload.forcedLockup && !newBid.lockup) {                                                                                                                            revert BidMustHaveLockup();                                                                                                                                      }

Using a struct to wrap a single boolean introduces unnecessary overhead:

• Additional gas cost for struct ABI encoding/decoding vs. raw boolean
• Extra memory allocation for the struct
• Increased code complexity

Recommendation

Consider encoding and decoding the boolean directly.

Coinbase

We prefer keeping this for now, since it's instructive for projects that want to implement their own custom contracts.