Clove

Description

SpotSettlement.settleSpotBatch tracks replay with fillUsage[fillId] and writes the mapping entry directly from the fillId value supplied in each MatchedFill. The function does not derive a canonical fill identifier from the full match semantics before checking replay state, which allows equivalent settlement semantics to be replayed with fresh fillId values while keeping the operator-level checks valid.

for (uint256 i = 0; i < fills.length; i++) {    bytes32 fillId = fills[i].fillId;    uint256 priorBatch = fillUsage[fillId];    if (priorBatch != 0) revert FillAlreadyConsumed(fillId, priorBatch);    fillUsage[fillId] = batch.batchId;    emit FillConsumed(fillId, batch.batchId);}

The same function does not consume NonceManager nonces or verify order intent expiry for either side of a match, so replay prevention is disconnected from user-intent state.

function verifyOrderIntent(OrderIntent calldata intent, bytes calldata sig) external view returns (bool) {    bytes32 structHash = keccak256(        abi.encode(            ORDER_INTENT_TYPEHASH,            intent.user,            intent.baseToken,            intent.quoteToken,            intent.amount,            intent.limitPrice,            intent.nonce,            intent.expiry,            intent.isBuy        )    );    bytes32 digest = keccak256(abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR, structHash));    return _recoverSigner(digest, sig) == intent.user;}

function useNonce(address user, uint256 nonce) external {    uint256 floor = minValidNonce[user];    if (nonce < floor) revert NonceTooLow(floor, nonce);    if (nonceUsed[user][nonce]) revert NonceAlreadyUsed(user, nonce);}

This is critical because valid economic matches can be represented under alternate identifiers and still advance state when user-level replay protections are not enforced in settlement execution.

There is a second gap in the same settlement path. After the replay checks, SpotSettlement forwards batch.deltas directly to AccountLedger without any on-chain check that the deltas are economically consistent with the provided fills and without any per-token conservation constraint. That means replay protection alone does not prevent an authorized operator from submitting internally valid signatures over economically invalid balance transitions.

IAccountLedger.Delta[] memory ledgerDeltas = new IAccountLedger.Delta[](batch.deltas.length);...IAccountLedger(accountLedger).applyNetDeltas(ledgerDeltas, batch.batchId);

Recommendation

Derive a canonical fill hash inside settlement and use that hash as the replay key. Add intent signature verification and expiry checks for both participants, and consume nonces through NonceManager before marking the canonical fill as used.

In the same hardening pass, add on-chain consistency checks that bind deltas to validated fill economics and enforce conservation constraints before calling AccountLedger.applyNetDeltas.

bytes32 canonicalFillId = keccak256(    abi.encode(        fills[i].maker,        fills[i].taker,        fills[i].makerOrderHash,        fills[i].takerOrderHash,        fills[i].baseAmount,        fills[i].quoteAmount    ));if (fillUsage[canonicalFillId] != 0) revert FillAlreadyConsumed(canonicalFillId, fillUsage[canonicalFillId]);nonceManager.useNonce(maker.user, makerIntent.nonce);fillUsage[canonicalFillId] = batch.batchId;

Clove: Fixed in PR #12.

Cantina: Fix verified. Canonical fill replay and token conservation were implemented, but nonce integration into settlement execution was not wired in, so the issue is not 100% addressed.

Description

The production deployment script instantiates MockERC20 contracts as core collateral assets. MockERC20 exposes an unrestricted mint function, so any caller can create arbitrary balances. In an isolated local environment this can be considered intentiona but in a production-labeled workflow this is a critical trust failure because collateral integrity is no longer anchored to a real asset supply.

MockERC20 usdc = new MockERC20("USD Coin", "USDC", 6);MockERC20 weth = new MockERC20("Wrapped Ether", "wETH", 18);MockERC20 wbtc = new MockERC20("Wrapped Bitcoin", "wBTC", 8);

function mint(address to, uint256 amount) external {    _mint(to, amount);}

If this script is ever used on a shared testnet or live environment, any external actor can mint collateral-like tokens, deposit them and drain honest reserves through normal withdrawal paths.

Impact is high because it can invalidate the protocol's entire accounting model and compromise all counterparties relying on those balances.

Recommendation

Ensure these MockERC20 contracts are not used for production deployments and deploy ERC20 contracts instead.

Clove: Fixed in PR #13 (commit 28027e4). SetupProd.s.sol is renamed to SetupDev.s.sol and now contains explicit warnings about unrestricted mock minting.

Cantina: Fix verified.

Description

The functions creditDeposit, lock, unlock and applyNetDeltas currently use nonReentrant even though they perform only internal state updates and do not invoke external callbacks. Entering and exiting ReentrancyGuard writes to and reads dedicated storage, which adds fixed overhead on every call even when no reentrant surface exists. In high-volume settlement and balance update flows this overhead compounds and adds non-trivial gas cost without increasing safety because withdrawals are already protected by separate checks and external-call paths.

Removing the modifier from these storage-only functions preserves behavior and lowers gas consumption on every call path.

Recommendation

Restrict nonReentrant to paths that perform external calls to vault and keep a minimal set of guarded entry points. For example:

function creditDeposit(address user, address token, uint256 amount)    external    onlyRole(VAULT_ROLE)    whenNotPaused{    ...}

Clove: Fixed in PR #14 (commit a69aa3f). AccountLedger.creditDeposit, lock, unlock and applyNetDeltas no longer use nonReentrant in PR 14, matching the storage-only optimization request.

Cantina: Fix verified. The same PR removes unnecessary nonReentrant from the storage-only functions while preserving it on the external-call withdrawal paths (debitForWithdrawal / debitForWithdrawalWithUnwrap).

Description

SpotSettlement._recoverSigner(...) is a custom signer recovery helper and is also used indirectly by verifyOrderIntent. The implementation accepts a 65-byte signature after basic parsing and passes it to ecrecover without low-s enforcement.

function _recoverSigner(bytes32 hash, bytes calldata signature) internal pure returns (address) {    if (signature.length != 65) revert InvalidOperatorSignature();
    bytes32 r;    bytes32 s;    uint8 v;
    assembly {        r := calldataload(signature.offset)        s := calldataload(add(signature.offset, 32))        v := byte(0, calldataload(add(signature.offset, 64)))    }
    if (v < 27) v += 27;
    address recovered = ecrecover(hash, v, r, s);    if (recovered == address(0)) revert InvalidOperatorSignature();    return recovered;}

Signature malleability through high-s variants is not prevented. In many current flows this may be harmless, but it becomes risky if signatures are treated as unique identifiers or persisted as proof artifacts.

Recommendation

Consider using ECDSA.recover from OpenZeppelin for standardized recovery semantics. This enforces canonical signatures, including valid v handling and low-s checks.

using ECDSA for bytes32;address recovered = hash.toEthSignedMessageHash().recover(signature);

If your stack uses short signatures, add explicit support for EIP-2098 packed signatures alongside the 65-byte path.

Clove: Fixed in PR #14 (commit a69aa3f). SpotSettlement._recoverSigner now uses ECDSA.tryRecover, replacing the custom parser/recover logic with OpenZeppelin standard recovery checks (including low-s/v validation).

Cantina: Fix verified. The custom hand-rolled recovery path was replaced by OZ ECDSA recovery in PR 14.

Description

The DOMAIN_SEPARATOR is set as immutable, if a hard fork results in a chain-id change, the contract may continue validating signatures tied to the pre-fork domain, which enables replay attack across forked chains.

Recommendation

Derive the DOMAIN_SEPARATOR from the current block.chainid (or cache it with the chain id and recompute when block.chainid changes), following the EIP-712 pattern used by Openzeppelin.

Clove: Fixed in PR #21.

Cantina: Fix verified. The domain separator replay exposure across chain-id changes was addressed with a fork-safe/chain-id-aware EIP-712 pattern.

Description

SpotSettlement grants role state in constructor from the admin and operator parameters without zero-address validation. If any are set to zero, role recovery and emergency controls can become impossible depending on which constructor argument is affected.

constructor(address admin, address operator, address initialAccountLedger, address initialNonceManager) {    _grantRole(DEFAULT_ADMIN_ROLE, admin);    _grantRole(PAUSER_ROLE, admin);    _grantRole(OPERATOR_ROLE, operator);
    accountLedger = initialAccountLedger;    nonceManager = initialNonceManager;}

Because there is no zero guard, governance and operator pathways can be permanently unusable from an invalid deployment configuration.

Recommendation

Fail deployment on zero-address role parameters for both admin and operator, and apply the same pattern to any other security-critical role-initializing constructor.

if (admin == address(0) || operator == address(0)) revert ZeroAddress();

Clove: Fixed in PR #15. SpotSettlement now guards admin and operator constructor args with ZeroAddress, so role assignment can’t be initialized to address(0) and permanently brick role controls.

Cantina: Fix verified. This matches the issue’s exact remediation (admin == 0 or operator == 0 now reverts before role grants), so the zero-address role-bricking condition is addressed in that PR.

Description

VaultV2 constructor assigns DEFAULT_ADMIN_ROLE and PAUSER_ROLE directly from the admin argument and initialLedger is optional only by zero check. A zero admin leaves role administration and policy control unassignable.

constructor(address admin, address initialLedger) {    _grantRole(DEFAULT_ADMIN_ROLE, admin);    _grantRole(PAUSER_ROLE, admin);    if (initialLedger != address(0)) {        accountLedger = initialLedger;    }}

A misconfigured zero role address can make role recovery and operational recovery actions unreachable for the vault path.

Recommendation

Consider adding the following checks in the constructor:

if (admin == address(0)) revert ZeroAddress();if (initialLedger != address(0) && initialLedger.code.length == 0) revert InvalidAddress();

Clove: Fixed in PR #15. VaultV2 now reverts if admin == address(0) in the constructor, so role setup for DEFAULT_ADMIN_ROLE / PAUSER_ROLE can’t be initialized to zero and brick role control.

Cantina: Fix verified.

Description

VaultV2.deposit credits internal accounting with the user-declared amount and then performs safeTransferFrom with that same amount, without measuring the effective token delta on the vault address. If a token has transfer tax, rebasing behavior or non-standard accounting, the vault can receive less than amount while internal balances still record a full credit.

Recommendation

If you want to support fee on transfer tokens consider computing the actual received values and use balanceAfter - balanceBefore as the credited amount, or restrict the supported token set to standard fixed-supply transfer semantics.

uint256 before = IERC20(token).balanceOf(address(this));IERC20(token).safeTransferFrom(msg.sender, address(this), amount);uint256 received = IERC20(token).balanceOf(address(this)) - before;totalTokenBalance[token] += received;IAccountLedger(accountLedger).creditDeposit(receiver, token, received);

Clove: Fixed in PR #22 (commit c1c12de). VaultV2.deposit now computes balanceOf before/after transfer and credits only the actual received amount; this directly addresses transfer-fee/non-standard token behavior.

Cantina: Fix verified. The PR includes VaultV2 delta-based deposit accounting and adds tests for fee-on-transfer handling (contracts/test/v2/VaultFeeOnTransfer.t.sol).

Description

The perpClearing variable is defined in SpotSettlement but never used. Since the contract is not upgradeable, this placeholder serves no practical purpose. It introduces dead code that may confuse reviewers and complicate maintenance.

Recommendation

Remove perpClearing and its associated roles to maintain the code clean.

Clove: Fixed in PR #20 (commit 78454b0 (https://github.com/Clove-Labs/clob/commit/78454b01bd7132f7ec6906481a5ef05d591b9e)).

Cantina: Fix verified. The unused perpClearing role/state in AccountLedger was removed, including the dead PERP_CLEARING_ROLE references.