From 2357be423d662c22cc43f106f2d7c7118e5530a4 Mon Sep 17 00:00:00 2001 From: detoo Date: Fri, 3 Jul 2026 09:20:47 -0700 Subject: [PATCH 1/4] fix: audit: cert legal ownership --- .../analysis/dealManager secondary trades.md | 33 ++++++++++--------- src/interfaces/ISecondaryTradeStorage.sol | 1 + src/storage/SecondaryTradeStorage.sol | 5 +++ test/DealManagerSecondaryTradeTest.t.sol | 32 ++++++++++++++++++ 4 files changed, 55 insertions(+), 16 deletions(-) diff --git a/specs/analysis/dealManager secondary trades.md b/specs/analysis/dealManager secondary trades.md index 26662a1..f4fed38 100644 --- a/specs/analysis/dealManager secondary trades.md +++ b/specs/analysis/dealManager secondary trades.md @@ -99,14 +99,14 @@ stateDiagram-v2 ### Settlement escrow status transitions -| From | Event | To | Notes | -|------------|-------------------------------------------------------|-------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| *(none)* | `acceptOffer()` — SELL offer | `ACCEPTED` | `safeTransferFrom` buyer pulls payment; escrow written directly as ACCEPTED | -| *(none)* | `acceptOffer()` — BUY offer | `ACCEPTED` | Funds already in contract from `postOffer()`; escrow written directly as ACCEPTED | -| `ACCEPTED` | `finalizeDeal()` — conditions met, before expiry | `FINALIZED` | Reverts past `secEscrow.expiry`; pays seller (minus fee), splits fee to integrator/platform, calls `IssuanceManager.secondaryTransfer` to mint buyer cert and void/decrement seller cert, consuming the lot's reserved units | -| `ACCEPTED` | `voidSecondaryTradeAgreement()` — party requests void | `VOIDED` | Each party (offeror or counterparty) requests; the escrow flips to VOIDED only once both have requested (or it is past expiry). A lone request just records intent and the counterparty can still finalize. | +| From | Event | To | Notes | +|------------|--------------------------------------------------------------------|-------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| *(none)* | `acceptOffer()` — SELL offer | `ACCEPTED` | `safeTransferFrom` buyer pulls payment; escrow written directly as ACCEPTED | +| *(none)* | `acceptOffer()` — BUY offer | `ACCEPTED` | Funds already in contract from `postOffer()`; escrow written directly as ACCEPTED | +| `ACCEPTED` | `finalizeDeal()` — conditions met, before expiry | `FINALIZED` | Reverts past `secEscrow.expiry`; pays seller (minus fee), splits fee to integrator/platform, calls `IssuanceManager.secondaryTransfer` to mint buyer cert and void/decrement seller cert, consuming the lot's reserved units | +| `ACCEPTED` | `voidSecondaryTradeAgreement()` — party requests void | `VOIDED` | Each party (offeror or counterparty) requests; the escrow flips to VOIDED only once both have requested (or it is past expiry). A lone request just records intent and the counterparty can still finalize. | | `ACCEPTED` | `syncVoidedSecondaryTradeAgreement()` — registry voided externally | `VOIDED` | Callable by anyone; guards via `isVoided()` check | -| `ACCEPTED` | `voidExpiredDeal()` — past `secEscrow.expiry` | `VOIDED` | Callable past expiry only | +| `ACCEPTED` | `voidExpiredDeal()` — past `secEscrow.expiry` | `VOIDED` | Callable past expiry only | All `VOIDED` paths share the same asset handling, symmetric between sides. The acceptor's asset is returned immediately: SELL refunds the buyer's payment (pulled per settlement at `acceptOffer()`), BUY releases the seller's @@ -259,13 +259,14 @@ flowchart TD ## 6. Key Invariants -| Invariant | Where enforced | -|---------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `offerId` never registered in `CyberAgreementRegistry` | `postOffer()` makes no registry call | -| `settlementAgreementId` always fully signed by both parties at creation | `acceptOffer()`: `signContractWithEscrow(offeror)` + `signContractFor(acceptor)` | -| Buyer's payment enters contract custody before settlement `ACCEPTED` is set | SELL: `safeTransferFrom` then status flip in same tx; BUY: custody at `postOffer()` | -| Seller cert units are reserved before any settlement is created | SELL: `reserveUnits` at `postOffer()`; BUY: `reserveUnits` at `acceptOffer()` per settlement | -| Each reserved unit is released or consumed exactly once (amount-based reservations, no IDs) | finalize: `secondaryTransfer` consumes the lot; void: BUY releases the lot, SELL releases only if offer `CANCELLED` (else lot returns to free pool); cancel: releases only `units - unitsAccepted` (the free pool) — accepted lots resolve later at finalize or void | -| Each unit of BUY consideration leaves custody exactly once (payout or refund) | finalize: paid to seller; void: refunded only if offer `CANCELLED` (else returns to free pool); cancel: refunds only `consideration - paymentAccepted` (the free pool) — accepted lots resolve later at finalize or void | -| Fee always split: integrator portion + platform portion | `_finalizeSecondaryEscrow`: `integratorFee + platformFee == totalFee` | +| Invariant | Where enforced | +|----------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `offerId` never registered in `CyberAgreementRegistry` | `postOffer()` makes no registry call | +| `settlementAgreementId` always fully signed by both parties at creation | `acceptOffer()`: `signContractWithEscrow(offeror)` + `signContractFor(acceptor)` | +| Buyer's payment enters contract custody before settlement `ACCEPTED` is set | SELL: `safeTransferFrom` then status flip in same tx; BUY: custody at `postOffer()` | +| Only the cert's legal owner of record may list its units for sale | SELL: `postOffer()` requires `legalOwnerOf(tokenId) == offeror`; BUY: `acceptOffer()` requires `legalOwnerOf(sellerTokenId) == acceptor` — nothing downstream binds the token to the seller, so this guards against selling (and being paid for) someone else's units | +| Seller cert units are reserved before any settlement is created | SELL: `reserveUnits` at `postOffer()`; BUY: `reserveUnits` at `acceptOffer()` per settlement | +| Each reserved unit is released or consumed exactly once (amount-based reservations, no IDs) | finalize: `secondaryTransfer` consumes the lot; void: BUY releases the lot, SELL releases only if offer `CANCELLED` (else lot returns to free pool); cancel: releases only `units - unitsAccepted` (the free pool) — accepted lots resolve later at finalize or void | +| Each unit of BUY consideration leaves custody exactly once (payout or refund) | finalize: paid to seller; void: refunded only if offer `CANCELLED` (else returns to free pool); cancel: refunds only `consideration - paymentAccepted` (the free pool) — accepted lots resolve later at finalize or void | +| Fee always split: integrator portion + platform portion | `_finalizeSecondaryEscrow`: `integratorFee + platformFee == totalFee` | | Closing conditions checked at finalize; threshold conditions checked at post/accept AND re-checked at finalize | `finalizeSecondaryTradeAgreement` walks `offer.closingConditions` and also re-runs `_checkThresholdConditions` (both snapshotted from DealManager config at `postOffer`), so eligibility lost after acceptance blocks the asset transfer | diff --git a/src/interfaces/ISecondaryTradeStorage.sol b/src/interfaces/ISecondaryTradeStorage.sol index d7d5bce..17c19ec 100644 --- a/src/interfaces/ISecondaryTradeStorage.sol +++ b/src/interfaces/ISecondaryTradeStorage.sol @@ -213,6 +213,7 @@ interface ISecondaryTradeStorage { error IntegratorNotWhitelisted(); error UnitsExceedOffer(); error NotOfferor(); + error NotCertOwner(); error MissingCertPrinter(); error NotPartyToAgreement(); error OfferAlreadyExists(); diff --git a/src/storage/SecondaryTradeStorage.sol b/src/storage/SecondaryTradeStorage.sol index b155a47..133234f 100644 --- a/src/storage/SecondaryTradeStorage.sol +++ b/src/storage/SecondaryTradeStorage.sol @@ -46,6 +46,7 @@ import "openzeppelin-contracts/token/ERC20/utils/SafeERC20.sol"; import {ECDSA} from "openzeppelin-contracts/utils/cryptography/ECDSA.sol"; import "../interfaces/ICyberAgreementRegistry.sol"; import "../interfaces/IIssuanceManager.sol"; +import {ICyberCertPrinter} from "../interfaces/ICyberCertPrinter.sol"; import "../interfaces/IDealManagerFactory.sol"; import "../interfaces/IDealManager.sol"; import "openzeppelin-contracts/utils/introspection/ERC165Checker.sol"; @@ -219,6 +220,8 @@ library SecondaryTradeStorage { _checkThresholdConditions(offerId, bytes32(0)); if (params.side == OfferSide.SELL) { + if (ICyberCertPrinter(params.certPrinter).legalOwnerOf(params.tokenId) != offeror) + revert ISecondaryTradeStorage.NotCertOwner(); // Reserve units on the seller's cert (routed through IssuanceManager, the only caller the printer allows) DealManagerStorage.getIssuanceManager().increaseUnitsReserved(params.certPrinter, params.tokenId, params.units); } else { @@ -380,6 +383,8 @@ library SecondaryTradeStorage { tokenId = params.sellerTokenId; buyer = offer.offeror; endorsementSig = params.openEndorsementSig; + if (ICyberCertPrinter(certPrinter).legalOwnerOf(tokenId) != acceptor) + revert ISecondaryTradeStorage.NotCertOwner(); // Reserve units on the seller's cert at acceptance (bid flow, routed through IssuanceManager) DealManagerStorage.getIssuanceManager().increaseUnitsReserved(certPrinter, tokenId, params.units); } diff --git a/test/DealManagerSecondaryTradeTest.t.sol b/test/DealManagerSecondaryTradeTest.t.sol index 1d87447..c76fcb5 100644 --- a/test/DealManagerSecondaryTradeTest.t.sol +++ b/test/DealManagerSecondaryTradeTest.t.sol @@ -805,6 +805,38 @@ contract DealManagerSecondaryTradeTest is Test { dm.postOffer(p); } + // A non-owner cannot list someone else's Ledger Entry Token for sale: without the ownership guard the + // attacker would be paid at finalize while the real owner's cert is decremented (buyer does not own + // sellerTokenId, which belongs to `seller`). + function test_RevertIf_PostOffer_Sell_NotCertOwner() public { + PostOfferParams memory p = _defaultSellOfferParams(); + vm.prank(buyer); + vm.expectRevert(ISecondaryTradeStorage.NotCertOwner.selector); + dm.postOffer(p); + } + + // BUY counterpart: a non-owner acceptor cannot sell someone else's Ledger Entry Token into a bid + // (attacker supplies sellerTokenId, owned by `seller`). + function test_RevertIf_AcceptBid_NotCertOwner() public { + (address attacker, uint256 attackerKey) = makeAddrAndKey("attacker"); + bytes32 offerId = _postBid(); + + AcceptOfferParams memory p = AcceptOfferParams({ + offerId: offerId, + units: UNITS, + buyerName: SELL_ACCEPT_BUYER_NAME, + buyerHostingMode: HostingMode.DIRECT, + adminMultisig: address(0), + sellerTokenId: sellerTokenId, + acceptorPartyValues: new string[](0), + acceptorAgreementSig: _acceptorSig(offerId, attacker, attackerKey), + openEndorsementSig: OPEN_ENDORSEMENT_SIG + }); + vm.prank(attacker); + vm.expectRevert(ISecondaryTradeStorage.NotCertOwner.selector); + dm.acceptOffer(p); + } + // ───────────────────────────────────────────────────────────────────────── // threshold conditions in an Offer // ───────────────────────────────────────────────────────────────────────── From 877534aff509d4e7d81d9a27fc5652926b332e95 Mon Sep 17 00:00:00 2001 From: detoo Date: Fri, 3 Jul 2026 09:38:02 -0700 Subject: [PATCH 2/4] fix: audit: cert printer validation --- .../analysis/dealManager secondary trades.md | 1 + src/IssuanceManager.sol | 6 ++++ src/interfaces/IIssuanceManager.sol | 1 + src/interfaces/ISecondaryTradeStorage.sol | 1 + src/storage/IssuanceManagerStorage.sol | 10 ++++++ src/storage/SecondaryTradeStorage.sol | 2 ++ ...DealManagerSecondaryTradeIndexerTest.t.sol | 12 +++---- test/DealManagerSecondaryTradeTest.t.sol | 32 +++++++++++++++---- ...IssuanceManagerSecondaryTransferTest.t.sol | 10 +++--- ...ntTest.t.sol => IssuanceManagerTest.t.sol} | 30 +++++++++++++---- 10 files changed, 81 insertions(+), 24 deletions(-) rename test/{IssuanceManagerCertificateCreatedEventTest.t.sol => IssuanceManagerTest.t.sol} (85%) diff --git a/specs/analysis/dealManager secondary trades.md b/specs/analysis/dealManager secondary trades.md index f4fed38..40085f5 100644 --- a/specs/analysis/dealManager secondary trades.md +++ b/specs/analysis/dealManager secondary trades.md @@ -264,6 +264,7 @@ flowchart TD | `offerId` never registered in `CyberAgreementRegistry` | `postOffer()` makes no registry call | | `settlementAgreementId` always fully signed by both parties at creation | `acceptOffer()`: `signContractWithEscrow(offeror)` + `signContractFor(acceptor)` | | Buyer's payment enters contract custody before settlement `ACCEPTED` is set | SELL: `safeTransferFrom` then status flip in same tx; BUY: custody at `postOffer()` | +| An offer's `certPrinter` must be a printer this SPV's IssuanceManager created and tracks | `postOffer()` requires `IssuanceManager.isPrinter(certPrinter)` (authoritative registry check, both sides); `acceptOffer()` reuses `offer.certPrinter`, so a buyer can never pay for a cert minted on a fake/foreign printer | | Only the cert's legal owner of record may list its units for sale | SELL: `postOffer()` requires `legalOwnerOf(tokenId) == offeror`; BUY: `acceptOffer()` requires `legalOwnerOf(sellerTokenId) == acceptor` — nothing downstream binds the token to the seller, so this guards against selling (and being paid for) someone else's units | | Seller cert units are reserved before any settlement is created | SELL: `reserveUnits` at `postOffer()`; BUY: `reserveUnits` at `acceptOffer()` per settlement | | Each reserved unit is released or consumed exactly once (amount-based reservations, no IDs) | finalize: `secondaryTransfer` consumes the lot; void: BUY releases the lot, SELL releases only if offer `CANCELLED` (else lot returns to free pool); cancel: releases only `units - unitsAccepted` (the free pool) — accepted lots resolve later at finalize or void | diff --git a/src/IssuanceManager.sol b/src/IssuanceManager.sol index a9484b9..0435af0 100644 --- a/src/IssuanceManager.sol +++ b/src/IssuanceManager.sol @@ -553,6 +553,12 @@ contract IssuanceManager is Initializable, BorgAuthACL, UUPSUpgradeable { return IssuanceManagerStorage.getPrinters()[index]; } + /// @notice Whether `printer` is a certificate printer this IssuanceManager created and still tracks + /// @dev Authoritative membership check against the registry; a self-reporting printer cannot forge it + function isPrinter(address printer) external view returns (bool) { + return IssuanceManagerStorage.isPrinter(printer); + } + /// @notice Sets the URI builder contract address /// @dev Only callable by owner /// @param _uriBuilder New URI builder contract address diff --git a/src/interfaces/IIssuanceManager.sol b/src/interfaces/IIssuanceManager.sol index 57a441f..be45adf 100644 --- a/src/interfaces/IIssuanceManager.sol +++ b/src/interfaces/IIssuanceManager.sol @@ -405,6 +405,7 @@ interface IIssuanceManager { function cyberCertPrinterBeacon() external view returns (UpgradeableBeacon); function cyberScripBeacon() external view returns (UpgradeableBeacon); function printers(uint256 index) external view returns (address); + function isPrinter(address printer) external view returns (bool); function setUriBuilder(address _uriBuilder) external; /// @notice Single-source signal for the buyer's newly minted Ledger Entry Token at secondary settlement. diff --git a/src/interfaces/ISecondaryTradeStorage.sol b/src/interfaces/ISecondaryTradeStorage.sol index 17c19ec..4403c4e 100644 --- a/src/interfaces/ISecondaryTradeStorage.sol +++ b/src/interfaces/ISecondaryTradeStorage.sol @@ -215,6 +215,7 @@ interface ISecondaryTradeStorage { error NotOfferor(); error NotCertOwner(); error MissingCertPrinter(); + error UnknownCertPrinter(); error NotPartyToAgreement(); error OfferAlreadyExists(); /// @notice Caller is not the signer they claim to be (signer must equal msg.sender) diff --git a/src/storage/IssuanceManagerStorage.sol b/src/storage/IssuanceManagerStorage.sol index 6df8e82..05a370a 100644 --- a/src/storage/IssuanceManagerStorage.sol +++ b/src/storage/IssuanceManagerStorage.sol @@ -234,6 +234,16 @@ library IssuanceManagerStorage { return issuanceManagerStorage().printers; } + /// @dev Linear membership scan over the printer registry. The list is admin-curated and small, and it is + /// the only authoritative source of printers created by this IssuanceManager (mirrors removePrinter). + function isPrinter(address printer) internal view returns (bool) { + address[] storage printers = issuanceManagerStorage().printers; + for (uint256 i = 0; i < printers.length; i++) { + if (printers[i] == printer) return true; + } + return false; + } + // Setters function setCORP(address _corp) internal { issuanceManagerStorage().CORP = _corp; diff --git a/src/storage/SecondaryTradeStorage.sol b/src/storage/SecondaryTradeStorage.sol index 133234f..56d6004 100644 --- a/src/storage/SecondaryTradeStorage.sol +++ b/src/storage/SecondaryTradeStorage.sol @@ -148,6 +148,8 @@ library SecondaryTradeStorage { // validate parameters if (params.certPrinter == address(0)) revert ISecondaryTradeStorage.MissingCertPrinter(); + if (!DealManagerStorage.getIssuanceManager().isPrinter(params.certPrinter)) + revert ISecondaryTradeStorage.UnknownCertPrinter(); // Validate integrator address integrator = params.integrator != address(0) diff --git a/test/DealManagerSecondaryTradeIndexerTest.t.sol b/test/DealManagerSecondaryTradeIndexerTest.t.sol index 9b50126..f938e62 100644 --- a/test/DealManagerSecondaryTradeIndexerTest.t.sol +++ b/test/DealManagerSecondaryTradeIndexerTest.t.sol @@ -71,9 +71,9 @@ import {CertificateDetails, ICyberCertPrinter} from "../src/interfaces/ICyberCer // Reuse the payment-token mock and the minimal CyberCorp / uriBuilder fixtures from sibling test files. import {SecERC20Mock} from "./DealManagerSecondaryTradeTest.t.sol"; import { - MockCyberCorpForCertEvent, - MockUriBuilderForCertEvent -} from "./IssuanceManagerCertificateCreatedEventTest.t.sol"; + MockCyberCorpForIM, + MockUriBuilderForIM +} from "./IssuanceManagerTest.t.sol"; /// @title DealManagerSecondaryTradeIndexerTest /// @notice Simulates an off-chain indexer (e.g. powering the Legion UI) and proves the secondary-trade @@ -224,7 +224,7 @@ contract DealManagerSecondaryTradeIndexerTest is Test { ICyberCertPrinter public certPrinter; IssuanceManager public im; CyberAgreementRegistry public registry; - MockCyberCorpForCertEvent public corp; + MockCyberCorpForIM public corp; DealManagerFactory public dmFactory; DealManager public dm; BorgAuth public auth; @@ -244,7 +244,7 @@ contract DealManagerSecondaryTradeIndexerTest is Test { paymentToken = new SecERC20Mock(); auth = new BorgAuth(owner); - corp = new MockCyberCorpForCertEvent(); + corp = new MockCyberCorpForIM(); // Real IssuanceManager + CyberCertPrinter, deployed through the IssuanceManagerFactory beacon stack // (mirrors IssuanceManagerSecondaryTransferTest), so the secondary-transfer logs are the real ones. @@ -263,7 +263,7 @@ contract DealManagerSecondaryTradeIndexerTest is Test { ) ); im = IssuanceManager(imFactory.deployIssuanceManager(imSalt)); - im.initialize(address(auth), address(corp), address(new MockUriBuilderForCertEvent()), address(imFactory)); + im.initialize(address(auth), address(corp), address(new MockUriBuilderForIM()), address(imFactory)); registry = CyberAgreementRegistry( address( diff --git a/test/DealManagerSecondaryTradeTest.t.sol b/test/DealManagerSecondaryTradeTest.t.sol index c76fcb5..486e559 100644 --- a/test/DealManagerSecondaryTradeTest.t.sol +++ b/test/DealManagerSecondaryTradeTest.t.sol @@ -69,11 +69,11 @@ import { SecondaryEscrowStatus } from "../src/storage/SecondaryTradeStorage.sol"; import {CyberAgreementUtils} from "./libs/CyberAgreementUtils.sol"; -// Minimal CyberCorp / uriBuilder fixtures for the real IssuanceManager, shared with the cert-event test. +// Minimal CyberCorp / uriBuilder fixtures for the real IssuanceManager, shared from IssuanceManagerTest. import { - MockCyberCorpForCertEvent, - MockUriBuilderForCertEvent -} from "./IssuanceManagerCertificateCreatedEventTest.t.sol"; + MockCyberCorpForIM, + MockUriBuilderForIM +} from "./IssuanceManagerTest.t.sol"; import {Test, console2} from "forge-std/Test.sol"; // ───────────────────────────────────────────────────────────────────────────── @@ -201,7 +201,7 @@ contract DealManagerSecondaryTradeTest is Test { ICyberCertPrinter public certPrinter; IssuanceManager public im; CyberAgreementRegistry public registry; - MockCyberCorpForCertEvent public corp; + MockCyberCorpForIM public corp; DealManagerFactory public dmFactory; DealManager public dm; BorgAuth public auth; @@ -237,7 +237,7 @@ contract DealManagerSecondaryTradeTest is Test { paymentToken = new SecERC20Mock(); auth = new BorgAuth(owner); - corp = new MockCyberCorpForCertEvent(); + corp = new MockCyberCorpForIM(); // Real IssuanceManager + CyberCertPrinter, deployed through the IssuanceManagerFactory beacon stack. IssuanceManagerFactory imFactory = IssuanceManagerFactory( @@ -255,7 +255,7 @@ contract DealManagerSecondaryTradeTest is Test { ) ); im = IssuanceManager(imFactory.deployIssuanceManager(imSalt)); - im.initialize(address(auth), address(corp), address(new MockUriBuilderForCertEvent()), address(imFactory)); + im.initialize(address(auth), address(corp), address(new MockUriBuilderForIM()), address(imFactory)); // Real CyberAgreementRegistry behind a proxy, sharing the same BorgAuth. registry = CyberAgreementRegistry( @@ -805,6 +805,24 @@ contract DealManagerSecondaryTradeTest is Test { dm.postOffer(p); } + // An offer cannot point at a printer this SPV's IssuanceManager did not create: without the check a buyer + // could pay real tokens for a cert minted on a fake/foreign printer (0xBEEF is not in the registry). + function test_RevertIf_PostOffer_Sell_UnknownCertPrinter() public { + PostOfferParams memory p = _defaultSellOfferParams(); + p.certPrinter = address(0xBEEF); + vm.prank(seller); + vm.expectRevert(ISecondaryTradeStorage.UnknownCertPrinter.selector); + dm.postOffer(p); + } + + function test_RevertIf_PostOffer_Buy_UnknownCertPrinter() public { + PostOfferParams memory p = _defaultBuyOfferParams(); + p.certPrinter = address(0xBEEF); + vm.prank(buyer); + vm.expectRevert(ISecondaryTradeStorage.UnknownCertPrinter.selector); + dm.postOffer(p); + } + // A non-owner cannot list someone else's Ledger Entry Token for sale: without the ownership guard the // attacker would be paid at finalize while the real owner's cert is decremented (buyer does not own // sellerTokenId, which belongs to `seller`). diff --git a/test/IssuanceManagerSecondaryTransferTest.t.sol b/test/IssuanceManagerSecondaryTransferTest.t.sol index 0aa23bf..392fd48 100644 --- a/test/IssuanceManagerSecondaryTransferTest.t.sol +++ b/test/IssuanceManagerSecondaryTransferTest.t.sol @@ -15,9 +15,9 @@ import "forge-std/Test.sol"; import {ERC1967Proxy} from "openzeppelin-contracts/proxy/ERC1967/ERC1967Proxy.sol"; // Reuse the minimal real-contract fixture mocks rather than redeclaring them. import { - MockCyberCorpForCertEvent, - MockUriBuilderForCertEvent -} from "./IssuanceManagerCertificateCreatedEventTest.t.sol"; + MockCyberCorpForIM, + MockUriBuilderForIM +} from "./IssuanceManagerTest.t.sol"; /// @title IssuanceManagerSecondaryTransferTest /// @notice Exercises the real IssuanceManager.secondaryTransfer against a real CyberCertPrinter (no mocks), @@ -62,8 +62,8 @@ contract IssuanceManagerSecondaryTransferTest is Test { issuanceManager = IssuanceManager(imFactory.deployIssuanceManager(SALT)); issuanceManager.initialize( address(auth), - address(new MockCyberCorpForCertEvent()), - address(new MockUriBuilderForCertEvent()), + address(new MockCyberCorpForIM()), + address(new MockUriBuilderForIM()), address(imFactory) ); } diff --git a/test/IssuanceManagerCertificateCreatedEventTest.t.sol b/test/IssuanceManagerTest.t.sol similarity index 85% rename from test/IssuanceManagerCertificateCreatedEventTest.t.sol rename to test/IssuanceManagerTest.t.sol index 25c431a..c5520ac 100644 --- a/test/IssuanceManagerCertificateCreatedEventTest.t.sol +++ b/test/IssuanceManagerTest.t.sol @@ -12,7 +12,7 @@ import "../src/libs/auth.sol"; import {ERC1967Proxy} from "openzeppelin-contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {IssuanceManagerFactory} from "../src/IssuanceManagerFactory.sol"; -contract MockCyberCorpForCertEvent { +contract MockCyberCorpForIM { function cyberCORPName() external pure returns (string memory) { return "TestCorp"; } function cyberCORPType() external pure returns (string memory) { return "C-Corp"; } function cyberCORPJurisdiction() external pure returns (string memory) { return "DE"; } @@ -21,7 +21,7 @@ contract MockCyberCorpForCertEvent { function roundManager() external pure returns (address) { return address(0); } } -contract MockUriBuilderForCertEvent is IUriBuilder { +contract MockUriBuilderForIM is IUriBuilder { function buildCertificateUri( string memory, string memory, @@ -99,8 +99,8 @@ contract MockUriBuilderForCertEvent is IUriBuilder { ) external pure returns (string memory) { return "uri://mock"; } } -contract IssuanceManagerCertificateCreatedEventTest is Test { - bytes32 constant SALT = bytes32(keccak256("IssuanceManagerCertificateCreatedEventTest")); +contract IssuanceManagerTest is Test { + bytes32 constant SALT = bytes32(keccak256("IssuanceManagerTest")); IssuanceManager public issuanceManager; BorgAuth public auth; @@ -130,8 +130,8 @@ contract IssuanceManagerCertificateCreatedEventTest is Test { issuanceManager = IssuanceManager(imFactory.deployIssuanceManager(SALT)); issuanceManager.initialize( address(auth), - address(new MockCyberCorpForCertEvent()), - address(new MockUriBuilderForCertEvent()), + address(new MockCyberCorpForIM()), + address(new MockUriBuilderForIM()), address(imFactory) ); } @@ -192,6 +192,24 @@ contract IssuanceManagerCertificateCreatedEventTest is Test { ); } + function test_isPrinter_TrueForCreatedPrinter() public { + ICyberCertPrinter certPrinter = _deployPrinter("Cert", "CERT"); + assertTrue(issuanceManager.isPrinter(address(certPrinter)), "created printer should be tracked"); + } + + function test_isPrinter_FalseForUnknownAddress() public { + _deployPrinter("Cert", "CERT"); + assertFalse(issuanceManager.isPrinter(address(0xBEEF)), "foreign address is not a printer"); + assertFalse(issuanceManager.isPrinter(address(0)), "zero address is not a printer"); + } + + function test_isPrinter_TracksMultiplePrinters() public { + ICyberCertPrinter a = _deployPrinter("CertA", "CERTA"); + ICyberCertPrinter b = _deployPrinter("CertB", "CERTB"); + assertTrue(issuanceManager.isPrinter(address(a)), "first printer tracked"); + assertTrue(issuanceManager.isPrinter(address(b)), "second printer tracked"); + } + function _deployPrinter( string memory name, string memory symbol From 9ab05cf6f21e7b0fff78cf10053d7439aa48a221 Mon Sep 17 00:00:00 2001 From: detoo Date: Fri, 3 Jul 2026 10:31:22 -0700 Subject: [PATCH 3/4] fix: audit: settlement expiry --- .../analysis/dealManager secondary trades.md | 49 +++++------ src/DealManager.sol | 11 +++ src/interfaces/IDealManager.sol | 2 + src/storage/SecondaryTradeStorage.sol | 26 +++++- test/DealManagerSecondaryTradeTest.t.sol | 83 ++++++++++++++++++- 5 files changed, 141 insertions(+), 30 deletions(-) diff --git a/specs/analysis/dealManager secondary trades.md b/specs/analysis/dealManager secondary trades.md index 40085f5..395f461 100644 --- a/specs/analysis/dealManager secondary trades.md +++ b/specs/analysis/dealManager secondary trades.md @@ -50,18 +50,18 @@ stateDiagram-v2 ### Offer status transitions -| From | Event | To | Notes | -|----------------------|-----------------------------------------------|----------------------|-----------------------------------------------------------------------------------------------------------------| -| *(none)* | `postOffer()` | `LIVE` | SELL: reserves units on cert; BUY: pulls full consideration into contract | -| any non-terminal | `cancelOffer()` | `CANCELLED` | Releases/refunds only the free pool; accepted settlements will not cancel and will resolve on their own cadence | -| `LIVE` | `acceptOffer()` — partial fill | `PARTIALLY_ACCEPTED` | `unitsAccepted < units` | -| `LIVE` | `acceptOffer()` — full fill | `FULLY_ACCEPTED` | `unitsAccepted == units` | -| `PARTIALLY_ACCEPTED` | `acceptOffer()` — completes fill | `FULLY_ACCEPTED` | | -| `PARTIALLY_ACCEPTED` | settlement voided | `LIVE` | `unitsAccepted` decrements; if back to 0 and not terminal | -| `FULLY_ACCEPTED` | settlement voided, `unitsAccepted` drops to 0 | `LIVE` | Same logic as `PARTIALLY_ACCEPTED`: status set purely by `unitsAccepted == 0` check | -| `FULLY_ACCEPTED` | settlement voided, `unitsAccepted` still > 0 | `PARTIALLY_ACCEPTED` | `unitsAccepted` decrements but offer not empty yet | -| `FULLY_ACCEPTED` | last settlement finalized | `FINALIZED` | `unitsFinalized == units`; terminal and immutable. CANCELLED stays sticky if the offer was cancelled | -| any | `block.timestamp > validUntil` | `EXPIRED` (logical) | No status field change; enforced at `acceptOffer()` and at `finalizeDeal()` (settlement expiry) | +| From | Event | To | Notes | +|----------------------|-----------------------------------------------|----------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| *(none)* | `postOffer()` | `LIVE` | SELL: reserves units on cert; BUY: pulls full consideration into contract | +| any non-terminal | `cancelOffer()` | `CANCELLED` | Releases/refunds only the free pool; accepted settlements will not cancel and will resolve on their own cadence | +| `LIVE` | `acceptOffer()` — partial fill | `PARTIALLY_ACCEPTED` | `unitsAccepted < units` | +| `LIVE` | `acceptOffer()` — full fill | `FULLY_ACCEPTED` | `unitsAccepted == units` | +| `PARTIALLY_ACCEPTED` | `acceptOffer()` — completes fill | `FULLY_ACCEPTED` | | +| `PARTIALLY_ACCEPTED` | settlement voided | `LIVE` | `unitsAccepted` decrements; if back to 0 and not terminal | +| `FULLY_ACCEPTED` | settlement voided, `unitsAccepted` drops to 0 | `LIVE` | Same logic as `PARTIALLY_ACCEPTED`: status set purely by `unitsAccepted == 0` check | +| `FULLY_ACCEPTED` | settlement voided, `unitsAccepted` still > 0 | `PARTIALLY_ACCEPTED` | `unitsAccepted` decrements but offer not empty yet | +| `FULLY_ACCEPTED` | last settlement finalized | `FINALIZED` | `unitsFinalized == units`; terminal and immutable. CANCELLED stays sticky if the offer was cancelled | +| any | `block.timestamp > validUntil` | `EXPIRED` (logical) | No status field change; `acceptOffer()` enforces the offer's `validUntil`, while `finalizeDeal()` enforces the settlement's own `secEscrow.expiry` (acceptance + settlement window), which is decoupled from the offer expiry | --- @@ -259,15 +259,16 @@ flowchart TD ## 6. Key Invariants -| Invariant | Where enforced | -|----------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `offerId` never registered in `CyberAgreementRegistry` | `postOffer()` makes no registry call | -| `settlementAgreementId` always fully signed by both parties at creation | `acceptOffer()`: `signContractWithEscrow(offeror)` + `signContractFor(acceptor)` | -| Buyer's payment enters contract custody before settlement `ACCEPTED` is set | SELL: `safeTransferFrom` then status flip in same tx; BUY: custody at `postOffer()` | -| An offer's `certPrinter` must be a printer this SPV's IssuanceManager created and tracks | `postOffer()` requires `IssuanceManager.isPrinter(certPrinter)` (authoritative registry check, both sides); `acceptOffer()` reuses `offer.certPrinter`, so a buyer can never pay for a cert minted on a fake/foreign printer | -| Only the cert's legal owner of record may list its units for sale | SELL: `postOffer()` requires `legalOwnerOf(tokenId) == offeror`; BUY: `acceptOffer()` requires `legalOwnerOf(sellerTokenId) == acceptor` — nothing downstream binds the token to the seller, so this guards against selling (and being paid for) someone else's units | -| Seller cert units are reserved before any settlement is created | SELL: `reserveUnits` at `postOffer()`; BUY: `reserveUnits` at `acceptOffer()` per settlement | -| Each reserved unit is released or consumed exactly once (amount-based reservations, no IDs) | finalize: `secondaryTransfer` consumes the lot; void: BUY releases the lot, SELL releases only if offer `CANCELLED` (else lot returns to free pool); cancel: releases only `units - unitsAccepted` (the free pool) — accepted lots resolve later at finalize or void | -| Each unit of BUY consideration leaves custody exactly once (payout or refund) | finalize: paid to seller; void: refunded only if offer `CANCELLED` (else returns to free pool); cancel: refunds only `consideration - paymentAccepted` (the free pool) — accepted lots resolve later at finalize or void | -| Fee always split: integrator portion + platform portion | `_finalizeSecondaryEscrow`: `integratorFee + platformFee == totalFee` | -| Closing conditions checked at finalize; threshold conditions checked at post/accept AND re-checked at finalize | `finalizeSecondaryTradeAgreement` walks `offer.closingConditions` and also re-runs `_checkThresholdConditions` (both snapshotted from DealManager config at `postOffer`), so eligibility lost after acceptance blocks the asset transfer | +| Invariant | Where enforced | +|----------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `offerId` never registered in `CyberAgreementRegistry` | `postOffer()` makes no registry call | +| `settlementAgreementId` always fully signed by both parties at creation | `acceptOffer()`: `signContractWithEscrow(offeror)` + `signContractFor(acceptor)` | +| Buyer's payment enters contract custody before settlement `ACCEPTED` is set | SELL: `safeTransferFrom` then status flip in same tx; BUY: custody at `postOffer()` | +| An offer's `certPrinter` must be a printer this SPV's IssuanceManager created and tracks | `postOffer()` requires `IssuanceManager.isPrinter(certPrinter)` (authoritative registry check, both sides); `acceptOffer()` reuses `offer.certPrinter`, so a buyer can never pay for a cert minted on a fake/foreign printer | +| Only the cert's legal owner of record may list its units for sale | SELL: `postOffer()` requires `legalOwnerOf(tokenId) == offeror`; BUY: `acceptOffer()` requires `legalOwnerOf(sellerTokenId) == acceptor` — nothing downstream binds the token to the seller, so this guards against selling (and being paid for) someone else's units | +| Each settlement's expiry runs from acceptance, not the offer's expiry | `acceptOffer()` stamps both the registry contract deadline and `secEscrow.expiry` with `block.timestamp + settlementWindow` (per-DealManager, default 7d), so a lot accepted just before the offer lapses still gets a full finalize window and cannot be made unfinalizeable by a late acceptance | +| Seller cert units are reserved before any settlement is created | SELL: `reserveUnits` at `postOffer()`; BUY: `reserveUnits` at `acceptOffer()` per settlement | +| Each reserved unit is released or consumed exactly once (amount-based reservations, no IDs) | finalize: `secondaryTransfer` consumes the lot; void: BUY releases the lot, SELL releases only if offer `CANCELLED` (else lot returns to free pool); cancel: releases only `units - unitsAccepted` (the free pool) — accepted lots resolve later at finalize or void | +| Each unit of BUY consideration leaves custody exactly once (payout or refund) | finalize: paid to seller; void: refunded only if offer `CANCELLED` (else returns to free pool); cancel: refunds only `consideration - paymentAccepted` (the free pool) — accepted lots resolve later at finalize or void | +| Fee always split: integrator portion + platform portion | `_finalizeSecondaryEscrow`: `integratorFee + platformFee == totalFee` | +| Closing conditions checked at finalize; threshold conditions checked at post/accept AND re-checked at finalize | `finalizeSecondaryTradeAgreement` walks `offer.closingConditions` and also re-runs `_checkThresholdConditions` (both snapshotted from DealManager config at `postOffer`), so eligibility lost after acceptance blocks the asset transfer | diff --git a/src/DealManager.sol b/src/DealManager.sol index 742a29a..f7b8f19 100644 --- a/src/DealManager.sol +++ b/src/DealManager.sol @@ -89,6 +89,7 @@ contract DealManager is error ConditionDoesNotExist(); error NotRefImplementation(); event MinTradeThresholdSet(uint256 minUnits, uint256 minConsideration, address setter); + event SettlementWindowSet(uint256 window, address setter); /// @notice Maps agreement IDs to arrays of counter party values for closed deals. mapping(bytes32 => string[]) public counterPartyValues; @@ -500,6 +501,11 @@ contract DealManager is emit MinTradeThresholdSet(units, consideration, msg.sender); } + function setSettlementWindow(uint256 window) external onlyAdmin { + SecondaryTradeStorage.secondaryTradeStorage().settlementWindow = window; + emit SettlementWindowSet(window, msg.sender); + } + function setDefaultIntegrator(address integrator) external onlyAdmin { if (integrator != address(0)) { if (!IDealManagerFactory(DealManagerStorage.getUpgradeFactory()).isIntegratorWhitelisted(integrator)) @@ -560,6 +566,11 @@ contract DealManager is return SecondaryTradeStorage.secondaryTradeStorage().defaultIntegrator; } + /// @notice Effective settlement window (applies the default when unset). + function getSettlementWindow() external view returns (uint256) { + return SecondaryTradeStorage.getSettlementWindow(); + } + function getOffer(bytes32 offerId) external view returns (Offer memory) { return SecondaryTradeStorage.secondaryTradeStorage().offers[offerId]; } diff --git a/src/interfaces/IDealManager.sol b/src/interfaces/IDealManager.sol index a7898bc..11f3155 100644 --- a/src/interfaces/IDealManager.sol +++ b/src/interfaces/IDealManager.sol @@ -134,6 +134,8 @@ interface IDealManager { function cancelOffer(bytes32 offerAgreementId) external; function acceptOffer(AcceptOfferParams calldata params) external returns (bytes32 settlementAgreementId); function setMinTradeThreshold(uint256 units, uint256 consideration) external; + function setSettlementWindow(uint256 window) external; + function getSettlementWindow() external view returns (uint256); function setDefaultIntegrator(address integrator) external; function getOffer(bytes32 offerAgreementId) external view returns (Offer memory); function getSecondaryEscrow(bytes32 agreementId) external view returns (SecondaryEscrow memory); diff --git a/src/storage/SecondaryTradeStorage.sol b/src/storage/SecondaryTradeStorage.sol index 56d6004..90441c4 100644 --- a/src/storage/SecondaryTradeStorage.sol +++ b/src/storage/SecondaryTradeStorage.sol @@ -68,6 +68,12 @@ library SecondaryTradeStorage { bytes32 constant STORAGE_POSITION = keccak256("cybercorp.secondary.trade.storage.v1"); + /// @dev Fallback settlement window used when settlementWindow is unset (0), so an accepted lot always + /// gets a finalize window measured from acceptance rather than inheriting the (possibly imminent) offer + /// expiry. Must exceed any configured TimeSettlementPeriodCondition minimum delay for the lot to be + /// finalizeable; owners with a longer minimum (e.g. QMS-mode) set a larger window via setSettlementWindow. + uint256 constant DEFAULT_SETTLEMENT_WINDOW = 7 days; + // ── EIP-712 relayer-authorization constants (see the relayer overloads of post/cancel/acceptOffer) ── // The signed message binds the full structured params (so a wallet renders each named field), the // principal `forAddr`, and an unordered `nonce` that makes each authorization single-use independent @@ -113,6 +119,15 @@ library SecondaryTradeStorage { address[] closingConditions; // default closing set // Consumed relayer-authorization nonces: usedAuthNonce[forAddr][nonce], order-independent single-use. mapping(address => mapping(uint256 => bool)) usedAuthNonce; + // Per-DealManager settlement window: how long after acceptance a lot has to finalize before it can be + // voided as expired. Decouples settlement expiry from offer expiry (0 = DEFAULT_SETTLEMENT_WINDOW). + uint256 settlementWindow; + } + + /// @notice Effective settlement window, applying the default when unset (so legacy DealManager does not need migration) + function getSettlementWindow() internal view returns (uint256) { + uint256 window = secondaryTradeStorage().settlementWindow; + return window == 0 ? DEFAULT_SETTLEMENT_WINDOW : window; } function secondaryTradeStorage() internal pure returns (SecondaryTradeData storage ds) { @@ -333,6 +348,11 @@ library SecondaryTradeStorage { _checkMinTradeThreshold(remainderUnits, remainderConsideration); } + // Settlement window runs from acceptance, not the offer's validUntil: a lot accepted moments before + // the offer expires still gets the full window to finalize (and clear any settlement-period minimum + // delay), instead of a truncated or already-expired one. + uint256 settlementExpiry = block.timestamp + getSettlementWindow(); + // Create fully-signed settlement agreement via registry. // settlementAgreementIds.length is a push-only monotonic nonce: unique per acceptance even if prior // settlements are later voided (which decrements unitsAccepted but never shrinks the array). @@ -352,7 +372,7 @@ library SecondaryTradeStorage { settlementPartyValues, bytes32(0), address(this), - offer.validUntil + settlementExpiry ); // Offeror: DealManager (finalizer) attests commitment via signContractWithEscrow. // The registry does not verify escrowSigner's EIP-712 sig here; the offeror's @@ -417,7 +437,7 @@ library SecondaryTradeStorage { paymentToken: offer.paymentToken, paymentAmount: partialConsideration, units: params.units, - expiry: offer.validUntil, + expiry: settlementExpiry, status: SecondaryEscrowStatus.ACCEPTED, feeDestination: offer.integrator, offerId: params.offerId, @@ -449,7 +469,7 @@ library SecondaryTradeStorage { // Acceptance funds the escrow atomically, so this event carries the settlement's payment too. emit ISecondaryTradeStorage.OfferAccepted( params.offerId, settlementAgreementId, acceptor, params.units, offer.paymentToken, partialConsideration, - tokenId, offer.validUntil, + tokenId, settlementExpiry, buyerName, buyerHostingMode, adminMultisig, endorsementSig ); } diff --git a/test/DealManagerSecondaryTradeTest.t.sol b/test/DealManagerSecondaryTradeTest.t.sol index 486e559..13dbc91 100644 --- a/test/DealManagerSecondaryTradeTest.t.sol +++ b/test/DealManagerSecondaryTradeTest.t.sol @@ -494,7 +494,7 @@ contract DealManagerSecondaryTradeTest is Test { assertEq(se.paymentToken, address(paymentToken), "payment token"); assertEq(se.paymentAmount, payment, "payment amount for this lot"); assertEq(se.units, units, "units in this lot"); - assertEq(se.expiry, block.timestamp + 1 days, "expiry mirrors offer validUntil"); + assertEq(se.expiry, block.timestamp + 7 days, "expiry runs the settlement window from acceptance, not offer validUntil"); assertEq(se.feeDestination, address(0), "no integrator set -> fees route to platform"); assertEq(se.tokenId, sellerTokenId, "reservation target is the seller's tokenId"); @@ -1268,6 +1268,9 @@ contract DealManagerSecondaryTradeTest is Test { vm.expectRevert(); dm.setMinTradeThreshold(1, 1); + vm.expectRevert(); + dm.setSettlementWindow(7 days); + vm.expectRevert(); dm.setDefaultIntegrator(address(0)); @@ -3084,6 +3087,80 @@ contract DealManagerSecondaryTradeTest is Test { dm.acceptOffer(p); } + // ───────────────────────────────────────────────────────────────────────── + // Settlement window — settlement expiry runs from acceptance, not offer expiry + // ───────────────────────────────────────────────────────────────────────── + + function test_GetSettlementWindow_DefaultsWhenUnset() public view { + assertEq(dm.getSettlementWindow(), 7 days, "unset window falls back to the 7-day default"); + } + + function test_SetSettlementWindow_UpdatesEffectiveWindow() public { + vm.expectEmit(false, false, false, true, address(dm)); + emit DealManager.SettlementWindowSet(45 days, owner); + vm.prank(owner); + dm.setSettlementWindow(45 days); + assertEq(dm.getSettlementWindow(), 45 days, "configured window overrides the default"); + } + + // Core regression for the truncated-settlement-window finding: a lot accepted moments before the offer + // expires still gets the full settlement window measured from acceptance, not the offer's imminent validUntil. + function test_AcceptOffer_SettlementExpiry_RunsFromAcceptance_NotOfferExpiry() public { + bytes32 offerId = _postSellOffer(); // validUntil = block.timestamp + 1 days + uint256 offerValidUntil = dm.getOffer(offerId).validUntil; + + // Accept one minute before the offer expires. + vm.warp(offerValidUntil - 1 minutes); + bytes32 settlementId = _acceptSellOffer(offerId); + + assertEq( + dm.getSecondaryEscrow(settlementId).expiry, + block.timestamp + 7 days, + "settlement expiry is acceptance time + window, independent of the near offer expiry" + ); + assertGt( + dm.getSecondaryEscrow(settlementId).expiry, + offerValidUntil, + "settlement outlives the offer it was accepted against" + ); + } + + function test_AcceptOffer_SettlementExpiry_UsesConfiguredWindow() public { + vm.prank(owner); + dm.setSettlementWindow(45 days); + + bytes32 offerId = _postSellOffer(); + bytes32 settlementId = _acceptSellOffer(offerId); + assertEq( + dm.getSecondaryEscrow(settlementId).expiry, + block.timestamp + 45 days, + "settlement expiry honors the configured window over the default" + ); + } + + // The compounding case from the finding: an acceptance late in the offer's life used to be + // unfinalizeable once block.timestamp passed offer.validUntil. With the window running from acceptance, + // the lot stays finalizeable well past the original offer expiry. + function test_FinalizeSecondaryTrade_LateAcceptance_StaysFinalizeable() public { + bytes32 offerId = _postSellOffer(); // validUntil = block.timestamp + 1 days + uint256 offerValidUntil = dm.getOffer(offerId).validUntil; + + vm.warp(offerValidUntil - 1 minutes); + bytes32 settlementId = _acceptSellOffer(offerId); + + // Advance past the original offer expiry (but within the settlement window). + vm.warp(offerValidUntil + 1 days); + assertLt(block.timestamp, dm.getSecondaryEscrow(settlementId).expiry, "still inside the settlement window"); + + vm.prank(keeper); + dm.finalizeSecondaryTradeAgreement(settlementId); + assertEq( + uint8(dm.getSecondaryEscrow(settlementId).status), + uint8(SecondaryEscrowStatus.FINALIZED), + "late-accepted lot finalizes past the original offer expiry" + ); + } + // Voiding one of several lots on a FULLY_ACCEPTED offer leaves unitsAccepted > 0, so the offer // reverts to PARTIALLY_ACCEPTED (not LIVE). The SELL lot returns to the offer's free pool and // stays reserved (offer not cancelled); only the voided lot's buyer payment is refunded. @@ -3288,7 +3365,7 @@ contract DealManagerSecondaryTradeTest is Test { } // finalizeSecondaryTradeAgreement past the settlement's expiry reverts. The settlement agreement's - // registry expiry equals secEscrow.expiry (both = offer.validUntil), so the registry + // registry expiry equals secEscrow.expiry (both = acceptance time + settlement window), so the registry // finalizeContract call reverts ContractExpired before the escrow's own SecondaryTradeAgreementExpired guard is // reached — that guard is defensive/unreachable for secondary settlements in normal flow. function test_RevertIf_FinalizeSecondaryTrade_PastSettlementExpiry() public { @@ -3335,7 +3412,7 @@ contract DealManagerSecondaryTradeTest is Test { address(paymentToken), CONSIDERATION, sellerTokenId, - block.timestamp + 1 days, + block.timestamp + 7 days, SELL_ACCEPT_BUYER_NAME, HostingMode.DIRECT, address(0), From a77dac3ec67230fd75c48eefb5e0a8e997ed6f64 Mon Sep 17 00:00:00 2001 From: detoo Date: Fri, 3 Jul 2026 13:44:34 -0700 Subject: [PATCH 4/4] fix: audit: secondary trade seller ownership checks --- src/interfaces/ISecondaryTradeStorage.sol | 3 ++ src/storage/SecondaryTradeStorage.sol | 7 +++ test/DealManagerSecondaryTradeTest.t.sol | 60 +++++++++++++++++++++++ 3 files changed, 70 insertions(+) diff --git a/src/interfaces/ISecondaryTradeStorage.sol b/src/interfaces/ISecondaryTradeStorage.sol index 4403c4e..59c0844 100644 --- a/src/interfaces/ISecondaryTradeStorage.sol +++ b/src/interfaces/ISecondaryTradeStorage.sol @@ -230,6 +230,9 @@ interface ISecondaryTradeStorage { error SecondaryTradeAgreementExpired(); error SecondaryTradeAgreementNotExpired(); error SecondaryTradeAgreementNotVoided(); + /// @notice At finalize, the cert's current legal owner no longer matches the settlement's seller of record + /// (ownership moved after listing/acceptance), so the payee and the party whose units are consumed diverge + error SecondaryTradeSellerOwnershipChanged(); error SecondaryConditionsNotMet(address condition); /// @notice Condition address supplied to a config setter is the zero address error InvalidSecondaryCondition(); diff --git a/src/storage/SecondaryTradeStorage.sol b/src/storage/SecondaryTradeStorage.sol index 90441c4..fc71bf0 100644 --- a/src/storage/SecondaryTradeStorage.sol +++ b/src/storage/SecondaryTradeStorage.sol @@ -400,6 +400,9 @@ library SecondaryTradeStorage { tokenId = offer.tokenId; buyer = acceptor; endorsementSig = offer.openEndorsementSig; + // Re-check the seller still owns the cert + if (ICyberCertPrinter(certPrinter).legalOwnerOf(tokenId) != offer.offeror) + revert ISecondaryTradeStorage.SecondaryTradeSellerOwnershipChanged(); } else { certPrinter = offer.certPrinter; tokenId = params.sellerTokenId; @@ -532,6 +535,10 @@ library SecondaryTradeStorage { offer.status = OfferStatus.FINALIZED; } (address seller, address buyer) = _settlementParties(offer, secEscrow); + // Require seller ownership to remain unchanged; if it was a legitimately-moved + // position it'd still revert here and it could be resolved via the void/expiry path instead of mispaying. + if (ICyberCertPrinter(offer.certPrinter).legalOwnerOf(secEscrow.tokenId) != seller) + revert ISecondaryTradeStorage.SecondaryTradeSellerOwnershipChanged(); // Fee math (mirrors DealManager.computeFee / getPlatformPayable) computed directly from the factory address upgradeFactory = DealManagerStorage.getUpgradeFactory(); uint256 fee = secEscrow.paymentAmount * IDealManagerFactory(upgradeFactory).getDefaultFeeRatio() / DealManagerFactoryStorage.BASIS_POINTS; diff --git a/test/DealManagerSecondaryTradeTest.t.sol b/test/DealManagerSecondaryTradeTest.t.sol index 13dbc91..fd6af34 100644 --- a/test/DealManagerSecondaryTradeTest.t.sol +++ b/test/DealManagerSecondaryTradeTest.t.sol @@ -855,6 +855,31 @@ contract DealManagerSecondaryTradeTest is Test { dm.acceptOffer(p); } + // A SELL offer whose seller loses cert ownership between postOffer and acceptance is rejected at accept + // time (via the IssuanceManager-assignment vector), so no doomed settlement is ever created. + function test_RevertIf_AcceptSellOffer_SellerOwnershipChanged() public { + address newOwner = makeAddr("newLegalOwner"); + bytes32 offerId = _postSellOffer(); + + vm.prank(owner); + im.assignCert(address(certPrinter), seller, sellerTokenId, newOwner, _sellerCertDetails(UNITS)); + + AcceptOfferParams memory p = AcceptOfferParams({ + offerId: offerId, + units: UNITS, + buyerName: SELL_ACCEPT_BUYER_NAME, + buyerHostingMode: HostingMode.DIRECT, + adminMultisig: address(0), + sellerTokenId: 0, + acceptorPartyValues: new string[](0), + acceptorAgreementSig: _acceptorSig(offerId, buyer, buyerKey), + openEndorsementSig: "" + }); + vm.prank(buyer); + vm.expectRevert(ISecondaryTradeStorage.SecondaryTradeSellerOwnershipChanged.selector); + dm.acceptOffer(p); + } + // ───────────────────────────────────────────────────────────────────────── // threshold conditions in an Offer // ───────────────────────────────────────────────────────────────────────── @@ -2060,6 +2085,41 @@ contract DealManagerSecondaryTradeTest is Test { assertEq(paymentToken.balanceOf(company), companyBefore, "company payable untouched on secondary path"); } + // Ownership is snapshotted for payment (SELL: seller = offer.offeror) but secondaryTransfer consumes units + // from the cert's LIVE legalOwnerOf. Re-register the seller's Ledger Entry Token to a new legal owner after + // acceptance (the IssuanceManager-assignment divergence vector — assignCert moves legalOwnerOf without + // moving the NFT or the reservation) and finalize must revert instead of paying the stale seller for units + // now owned by someone else. + function test_RevertIf_FinalizeSecondaryTrade_Sell_SellerOwnershipChanged() public { + address newOwner = makeAddr("newLegalOwner"); + bytes32 offerId = _postSellOffer(); + bytes32 settlementId = _acceptSellOffer(offerId); + + vm.prank(owner); + im.assignCert(address(certPrinter), seller, sellerTokenId, newOwner, _sellerCertDetails(UNITS)); + assertEq(certPrinter.legalOwnerOf(sellerTokenId), newOwner, "legal owner moved off the seller"); + + vm.expectRevert(ISecondaryTradeStorage.SecondaryTradeSellerOwnershipChanged.selector); + vm.prank(keeper); + dm.finalizeSecondaryTradeAgreement(settlementId); + } + + // BUY mirror: seller = the acceptor (secEscrow.counterparty), snapshotted at acceptance. Re-registering the + // acceptor's Ledger Entry Token before finalize must likewise revert. + function test_RevertIf_FinalizeSecondaryTrade_Buy_SellerOwnershipChanged() public { + address newOwner = makeAddr("newLegalOwner"); + bytes32 offerId = _postBid(); + bytes32 settlementId = _acceptBid(offerId); + + vm.prank(owner); + im.assignCert(address(certPrinter), seller, sellerTokenId, newOwner, _sellerCertDetails(UNITS)); + assertEq(certPrinter.legalOwnerOf(sellerTokenId), newOwner, "legal owner moved off the acceptor"); + + vm.expectRevert(ISecondaryTradeStorage.SecondaryTradeSellerOwnershipChanged.selector); + vm.prank(keeper); + dm.finalizeSecondaryTradeAgreement(settlementId); + } + function test_FinalizeSecondaryTrade_Buy() public { // BUY mirror: the bid stays FULLY_ACCEPTED until every lot settles, reaching FINALIZED only // once the last outstanding lot is finalized.