diff --git a/.gitmodules b/.gitmodules index bfd60fa..742bc39 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,3 +1,6 @@ [submodule "src/contracts/lib/forge-std"] path = src/contracts/lib/forge-std url = https://github.com/foundry-rs/forge-std +[submodule "src/contracts/lib/openzeppelin-contracts"] + path = src/contracts/lib/openzeppelin-contracts + url = https://github.com/OpenZeppelin/openzeppelin-contracts diff --git a/SECURITY-HARDENING.md b/SECURITY-HARDENING.md new file mode 100644 index 0000000..98ea758 --- /dev/null +++ b/SECURITY-HARDENING.md @@ -0,0 +1,59 @@ +# StartupChain security hardening + +> Author: Lucia (Zenbit) · Branch: `hardening/contract-and-eas-security` · **Sepolia only — no mainnet +> moves.** This PR applies the transferable hardening patterns from the AxoloDAO best-of-4 review to +> StartupChain. The two products stay fully decoupled (no shared code); only the *patterns* transfer. + +Each fix ships with a test that proves the fix. `forge test` = **61 passing** (13 new StartupChain tests, +2 new AttestationModule tests, existing suites unchanged). TS-layer typecheck clean. + +## Contract fixes + +| # | Sev | Fix | Test | +|---|-----|-----|------| +| 1 | HIGH | **`recordCompany` caller auth.** The caller must be an owner of the target Safe (`ISafe.isOwner`), and the Safe must be deployed. Closes the `ensNameToCompanyId` squat / front-run vector (previously anyone could register any name/Safe). | `test_recordCompany_squatByNonOwnerReverts`, `test_recordCompany_nonContractSafeReverts` | +| 2 | HIGH | **Admin → `Ownable2Step` + timelock.** Ownership is 2-step (`transferOwnership` → `acceptOwnership`); deploy the owner as a **2/3 Safe**. `setFeeRecipient` is now `proposeFeeRecipient` → (2-day timelock) → `executeFeeRecipient`. | `test_ownershipIsTwoStep`, `test_setFeeRecipientIsTimelocked`, `test_withdrawOnlyOwner` | +| 3 | HIGH | **CEI + reentrancy in `recordCompany`.** All state is written before the fee `.call`, and the function is `nonReentrant` (the fee transfer previously executed before state writes). A reverting fee recipient reverts the whole tx and persists nothing. | `test_recordCompany_revertingFeeRecipientRevertsWholeTx`, `test_recordCompany_feePaidToTreasury` | +| 4 | HIGH | **Harden `AttestationModule` before it ships.** `onlyCompanyMember` now enforces real membership via the StartupChain registry (`isFounder`); the six schema-config setters are owner-gated (were permissionless one-time, i.e. first-caller-pins). | `testOnlyCompanyMemberCanAttest`, `testSchemaSetterIsOwnerGated` | +| 5 | MED | **Correct `.eth` namehash.** `transferENS` / `createSubdomain` / `revokeSubdomain` now parent labels under `namehash("eth")` instead of the ENS root (the old code treated the label as a TLD). Unblocks the dashboard subdomain / resolution issues (#43/#44/#33). | `test_namehash_isEthParentedNotRoot` | +| 6 | MED | **Bounded founder loops + reverse index.** `MAX_FOUNDERS = 50` caps every founder loop; a `founder → companyId[]` index (`getCompaniesByFounder`) replaces the off-chain O(n) full-registry scan; `isFounder` backs the #4 membership check. | `test_maxFoundersEnforced`, `test_founderReverseIndex` | + +## Off-chain fixes + +| # | Sev | Fix | File | +|---|-----|-----|------| +| 7 | MED | **Bind payment to the registration.** `checkPaymentStatusAction` now requires the payment tx to originate from a founder wallet of the registration (`allowedFrom`), closing the cross-user / cross-registration replay by non-participants, plus an optional per-registration `expectedCommitment` (tx `input`) hook that binds one payment to one (user, ENS) once the client sends it. | `payment-actions.ts`, `actions.ts` | +| 8 | MED | **De-risk the hot relayer key.** The treasury is no longer the signer address by default: `STARTUPCHAIN_TREASURY_ADDRESS` sets a distinct treasury (ideally a Safe), separating the least-privilege gas payer from fee custody. Falls back to the signer with a loud warning for dev. | `startupchain-client.ts` | + +**#7 residual (flagged, not fully closed):** true single-use (one payment ⇒ at most one registration) +needs either a persisted consumed-tx set or moving the fee on-chain into `recordCompany`'s `msg.value`. +This repo has no server-side store yet; the founder binding closes the replay-by-non-participant vector +and the commitment hook closes cross-ENS reuse when the client adopts it. Tracked as follow-up. + +## Tradeoffs surfaced + +- **Solo companies get a 1/1 Safe.** `recordCompany` allows `threshold == founders.length == 1`. A 1/1 + Safe has no multisig protection, but enforcing a 2-of-N floor would block legitimate solo founders. We + **allow it and document it** rather than enforce a floor. `test_soloFounderOneOfOneSafeAllowed`. +- **Subdomain ENS ownership model.** The correct namehash (#5) is necessary but not sufficient: the Safe + owns the 2LD, so `createSubdomain`/`revokeSubdomain` require the StartupChain contract to be an approved + operator of the company node (`ensRegistry.setApprovalForAll(startupChain, true)` from the Safe) or the + subdomain logic to move into a Safe module. This PR fixes the node math and documents the operator + requirement; the module refactor is out of scope. +- **Founder reverse-index is append-only.** `getCompaniesByFounder` may list a company a founder has since + left (after `updateFounders`); callers confirm current membership with `isFounder`. This avoids O(n²) + removal bookkeeping. + +## Not in this PR (item #9 — separate, larger PR) + +Moving the cap-table detail off contract storage to a permanent Arweave snapshot + an EAS +`CompanyFormed`/`CapTableAttested` anchor (per the AxoloDAO ADR-001 storage-tier rule) is a larger +refactor with its own parity + squat/replay regression tests. Left as a follow-up so this PR stays a +focused, reviewable hardening set. + +## Deploy notes (Sepolia) + +Constructors changed: `StartupChain(ensRegistry, ensResolver, feeRecipient, initialOwner)` and +`AttestationModule(eas, startupChainRegistry, initialOwner)`. Set `STARTUPCHAIN_OWNER` (the 2/3 Safe), +`FEE_RECIPIENT` / `STARTUPCHAIN_TREASURY_ADDRESS` (a treasury distinct from the signer). External audit +precedes any mainnet deploy. diff --git a/src/app/(app)/dashboard/setup/actions.ts b/src/app/(app)/dashboard/setup/actions.ts index eeec84b..df323e6 100644 --- a/src/app/(app)/dashboard/setup/actions.ts +++ b/src/app/(app)/dashboard/setup/actions.ts @@ -200,9 +200,11 @@ export async function commitEnsRegistrationAction({ const { totalWei } = await getEnsRegistrationCostAction(ensName, verificationYears, founders.length) console.log(LOG_PREFIX, 'Expected payment amount:', totalWei) + // SECURITY (#7): bind the payment to a founder wallet of this registration. const paymentStatus = await checkPaymentStatusAction({ paymentTxHash, minValueWei: totalWei, + allowedFrom: founderStructs.map((f) => f.wallet), }) console.log(LOG_PREFIX, 'Payment verification result:', paymentStatus) diff --git a/src/app/(app)/dashboard/setup/payment-actions.ts b/src/app/(app)/dashboard/setup/payment-actions.ts index 81adf8a..d836403 100644 --- a/src/app/(app)/dashboard/setup/payment-actions.ts +++ b/src/app/(app)/dashboard/setup/payment-actions.ts @@ -53,20 +53,36 @@ export async function verifyPrepaymentAction({ } /** - * Check if user has already sent a payment transaction to treasury - * by looking at recent transactions (simplified approach) + * Verify a treasury prepayment transaction. * - * SECURITY: This function verifies: - * 1. Transaction exists and was successful - * 2. Transaction was sent TO the treasury address - * 3. Transaction value is >= minimum required (if specified) + * SECURITY: verifies that the transaction + * 1. exists and was successful, + * 2. was sent TO the treasury address, + * 3. has value >= the minimum required (if specified), + * 4. (#7 — payment binding) was sent FROM one of the registration's founder wallets, so a stranger's + * payment can't be cited and a payment can't be replayed across registrations whose founder set does + * not include the original payer, and + * 5. (optional) carries the expected per-registration commitment in its calldata (`expectedCommitment`), + * which — once the client sends it — binds one payment to exactly one (user, ENS) registration. + * + * NOTE (#7 residual): true single-use (one payment ⇒ at most one registration) additionally requires + * either a persisted consumed-tx set or moving the fee on-chain into `recordCompany`'s msg.value. This + * repo has no server-side store yet; the founder binding above closes the cross-user/cross-registration + * replay vector, and `expectedCommitment` closes cross-ENS reuse when the client adopts it. Tracked as a + * follow-up. */ export async function checkPaymentStatusAction({ paymentTxHash, minValueWei, + allowedFrom, + expectedCommitment, }: { paymentTxHash: string minValueWei?: string + /** Founder wallet addresses; the payment must originate from one of them. */ + allowedFrom?: string[] + /** 0x-hex the payment tx `input` must equal (per-registration commitment). Enforced only if set. */ + expectedCommitment?: string }) { if (!paymentTxHash || !paymentTxHash.startsWith("0x")) { return { confirmed: false, error: "Invalid transaction hash" } @@ -95,7 +111,29 @@ export async function checkPaymentStatusAction({ if (tx.value < minValue) { return { confirmed: false, - error: `Insufficient payment: received ${tx.value.toString()}, required ${minValueWei}` + error: `Insufficient payment: received ${tx.value.toString()}, required ${minValueWei}`, + } + } + } + + // SECURITY (#7): bind the payment to a founder of this registration. + if (allowedFrom && allowedFrom.length > 0) { + const from = tx.from?.toLowerCase() + const isFromFounder = allowedFrom.some((a) => a.toLowerCase() === from) + if (!isFromFounder) { + return { + confirmed: false, + error: "Payment must be sent from a founder wallet of this registration", + } + } + } + + // SECURITY (#7, optional): bind the payment to this specific registration via a commitment. + if (expectedCommitment) { + if ((tx.input ?? "0x").toLowerCase() !== expectedCommitment.toLowerCase()) { + return { + confirmed: false, + error: "Payment commitment does not match this registration", } } } diff --git a/src/contracts/foundry.lock b/src/contracts/foundry.lock index 8f51b75..f613b12 100644 --- a/src/contracts/foundry.lock +++ b/src/contracts/foundry.lock @@ -1,5 +1,11 @@ { "lib/forge-std": { "rev": "8bbcf6e3f8f62f419e5429a0bd89331c85c37824" + }, + "lib/openzeppelin-contracts": { + "tag": { + "name": "v5.1.0", + "rev": "69c8def5f222ff96f2b5beff05dfba996368aa79" + } } } \ No newline at end of file diff --git a/src/contracts/foundry.toml b/src/contracts/foundry.toml index 961a663..2621d1e 100644 --- a/src/contracts/foundry.toml +++ b/src/contracts/foundry.toml @@ -2,6 +2,7 @@ src = "src" out = "out" libs = ["lib"] +solc = "0.8.28" optimizer = true optimizer_runs = 200 via_ir = true diff --git a/src/contracts/lib/openzeppelin-contracts b/src/contracts/lib/openzeppelin-contracts new file mode 160000 index 0000000..69c8def --- /dev/null +++ b/src/contracts/lib/openzeppelin-contracts @@ -0,0 +1 @@ +Subproject commit 69c8def5f222ff96f2b5beff05dfba996368aa79 diff --git a/src/contracts/remappings.txt b/src/contracts/remappings.txt new file mode 100644 index 0000000..779c67d --- /dev/null +++ b/src/contracts/remappings.txt @@ -0,0 +1,2 @@ +@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/ +forge-std/=lib/forge-std/src/ diff --git a/src/contracts/script/DeployAttestationModule.s.sol b/src/contracts/script/DeployAttestationModule.s.sol index 5c7c445..17acdd9 100644 --- a/src/contracts/script/DeployAttestationModule.s.sol +++ b/src/contracts/script/DeployAttestationModule.s.sol @@ -32,7 +32,11 @@ contract DeployAttestationModule is Script { uint256 deployerPrivateKey = vm.envUint("DEPLOYER_KEY"); address deployer = vm.addr(deployerPrivateKey); + // #4 — schema config is owner-gated; deploy the owner as a 2/3 Safe. + address initialOwner = vm.envOr("STARTUPCHAIN_OWNER", deployer); + console.log("Deploying AttestationModule with deployer:", deployer); + console.log("Owner (should be a 2/3 Safe):", initialOwner); console.log("EAS address:", easAddress); console.log("StartupChain registry:", startupChainRegistry); console.log("Chain ID:", chainId); @@ -41,7 +45,8 @@ contract DeployAttestationModule is Script { AttestationModule attestationModule = new AttestationModule( easAddress, - startupChainRegistry + startupChainRegistry, + initialOwner ); console.log("AttestationModule deployed at:", address(attestationModule)); diff --git a/src/contracts/script/DeployStartupChain.s.sol b/src/contracts/script/DeployStartupChain.s.sol index 7cdc05f..ec46413 100644 --- a/src/contracts/script/DeployStartupChain.s.sol +++ b/src/contracts/script/DeployStartupChain.s.sol @@ -14,11 +14,14 @@ contract DeployStartupChain is Script { uint256 deployerPrivateKey = vm.envUint("DEPLOYER_KEY"); address deployer = vm.addr(deployerPrivateKey); - // Fee recipient is the deployer (treasury) - address feeRecipient = deployer; + // #2/#8 — owner should be a 2/3 Safe; fee recipient (treasury) should be SEPARATE from the hot + // signer. Both default to the deployer only if not provided (loudly, for local dev). + address initialOwner = vm.envOr("STARTUPCHAIN_OWNER", deployer); + address feeRecipient = vm.envOr("FEE_RECIPIENT", deployer); console.log("Deploying StartupChain with deployer:", deployer); - console.log("Fee recipient:", feeRecipient); + console.log("Owner (should be a 2/3 Safe):", initialOwner); + console.log("Fee recipient (treasury):", feeRecipient); console.log("Chain ID:", block.chainid); vm.startBroadcast(deployerPrivateKey); @@ -26,7 +29,8 @@ contract DeployStartupChain is Script { StartupChain startupChain = new StartupChain( ensRegistry, ensResolver, - feeRecipient + feeRecipient, + initialOwner ); console.log("StartupChain deployed at:", address(startupChain)); diff --git a/src/contracts/src/AttestationModule.sol b/src/contracts/src/AttestationModule.sol index d726d53..d5e21ce 100644 --- a/src/contracts/src/AttestationModule.sol +++ b/src/contracts/src/AttestationModule.sol @@ -1,5 +1,12 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.13; +pragma solidity 0.8.28; + +import {Ownable, Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol"; + +/// @notice The StartupChain registry membership check (#4 — replaces the no-op onlyCompanyMember). +interface IStartupChainRegistry { + function isFounder(uint256 companyId, address account) external view returns (bool); +} interface IEAS { struct AttestationRequest { @@ -34,7 +41,11 @@ interface IEAS { function getAttestation(bytes32 uid) external view returns (Attestation memory); } -contract AttestationModule { +/// @title AttestationModule +/// @notice EAS wrapper for company attestations. Hardened (#4): `onlyCompanyMember` now enforces real +/// StartupChain-registry membership, and the schema-config setters are owner-gated (not permissionless), +/// so the schema UIDs can no longer be front-run/pinned by the first caller. Admin = a 2/3 Safe. +contract AttestationModule is Ownable2Step { enum AttestationType { CompanyFormation, GovernanceDecision, @@ -90,48 +101,52 @@ contract AttestationModule { ); modifier onlyCompanyMember(uint256 _companyId) { - // In production, this would check against StartupChain registry - // For now, we'll allow any address to attest (can be restricted later) + // #4 — real membership check against the StartupChain registry. + require( + IStartupChainRegistry(startupChainRegistry).isFounder(_companyId, msg.sender), "Not a company member" + ); _; } - constructor(address _eas, address _startupChainRegistry) { + /// @param _initialOwner Admin for schema configuration — deploy as a 2/3 Safe multisig. + constructor(address _eas, address _startupChainRegistry, address _initialOwner) Ownable(_initialOwner) { + require(_startupChainRegistry != address(0), "Invalid registry"); eas = IEAS(_eas); startupChainRegistry = _startupChainRegistry; } - // Schema management functions - function setCompanyFormationSchema(bytes32 _schema) external { + // Schema management functions — #4 owner-gated (was permissionless one-time). + function setCompanyFormationSchema(bytes32 _schema) external onlyOwner { require(companyFormationSchema == bytes32(0), "Schema already set"); companyFormationSchema = _schema; emit SchemaRegistered(AttestationType.CompanyFormation, _schema); } - function setGovernanceDecisionSchema(bytes32 _schema) external { + function setGovernanceDecisionSchema(bytes32 _schema) external onlyOwner { require(governanceDecisionSchema == bytes32(0), "Schema already set"); governanceDecisionSchema = _schema; emit SchemaRegistered(AttestationType.GovernanceDecision, _schema); } - function setFinancialTransactionSchema(bytes32 _schema) external { + function setFinancialTransactionSchema(bytes32 _schema) external onlyOwner { require(financialTransactionSchema == bytes32(0), "Schema already set"); financialTransactionSchema = _schema; emit SchemaRegistered(AttestationType.FinancialTransaction, _schema); } - function setMilestoneAchievementSchema(bytes32 _schema) external { + function setMilestoneAchievementSchema(bytes32 _schema) external onlyOwner { require(milestoneAchievementSchema == bytes32(0), "Schema already set"); milestoneAchievementSchema = _schema; emit SchemaRegistered(AttestationType.MilestoneAchievement, _schema); } - function setMembershipChangeSchema(bytes32 _schema) external { + function setMembershipChangeSchema(bytes32 _schema) external onlyOwner { require(membershipChangeSchema == bytes32(0), "Schema already set"); membershipChangeSchema = _schema; emit SchemaRegistered(AttestationType.MembershipChange, _schema); } - function setContractDeploymentSchema(bytes32 _schema) external { + function setContractDeploymentSchema(bytes32 _schema) external onlyOwner { require(contractDeploymentSchema == bytes32(0), "Schema already set"); contractDeploymentSchema = _schema; emit SchemaRegistered(AttestationType.ContractDeployment, _schema); diff --git a/src/contracts/src/StartupChain.sol b/src/contracts/src/StartupChain.sol index f853ffb..fe43cd2 100644 --- a/src/contracts/src/StartupChain.sol +++ b/src/contracts/src/StartupChain.sol @@ -1,14 +1,36 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.13; +pragma solidity 0.8.28; import "./interfaces/IENS.sol"; +import {Ownable, Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol"; +import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol"; -contract StartupChain { - // Founder with equity allocation (basis points, 10000 = 100%) +/// @notice Minimal Gnosis Safe interface — used to authorize `recordCompany` against real Safe control. +interface ISafe { + function isOwner(address owner) external view returns (bool); +} + +/// @title StartupChain +/// @notice Company (microDAO) registry. Hardened by the Zenbit security pass (mirrors the AxoloDAO +/// best-of-4 hardening patterns; this repo stays fully decoupled from axolodao-system): +/// - **#1** `recordCompany` now requires the caller to be an owner of the target Safe — closes the +/// `ensNameToCompanyId` squat / front-run vector (anyone could previously register any name/Safe). +/// - **#2** admin is `Ownable2Step` (2-step ownership; deploy the owner as a 2/3 Safe) and +/// `setFeeRecipient` is behind a timelock. +/// - **#3** `recordCompany` follows checks-effects-interactions and is `nonReentrant` (the fee `.call` +/// was previously executed before state writes). +/// - **#5** the `.eth` namehash is computed correctly (`namehash("eth")` as the parent, not the ENS +/// root) in `transferENS` / `createSubdomain` / `revokeSubdomain`. +/// - **#6** founder loops are bounded (`MAX_FOUNDERS`) and a `founder → companyId[]` index replaces the +/// off-chain O(n) full-registry scan; `isFounder` backs the AttestationModule membership check. +/// +/// **Sepolia only.** No mainnet moves. Cap-table detail still lives in contract storage; moving it to a +/// permanent Arweave snapshot + EAS anchor is a separate, larger PR (see SECURITY notes, item #9). +contract StartupChain is Ownable2Step, ReentrancyGuard { struct Founder { address wallet; - uint256 equityBps; // Equity in basis points (e.g., 5000 = 50%) - string role; // Optional role: "CEO", "CTO", etc. + uint256 equityBps; // basis points (10000 = 100%) + string role; } struct Company { @@ -16,9 +38,9 @@ contract StartupChain { address companyAddress; // Safe address (ENS owner) string ensName; uint256 creationDate; - address safeAddress; // Gnosis Safe multisig address - address governanceAddress; // GovernanceWrapper contract address - uint256 threshold; // Safe signing threshold + address safeAddress; + address governanceAddress; + uint256 threshold; } struct Subdomain { @@ -28,161 +50,101 @@ contract StartupChain { bool active; } - // Fee configuration (25% = 2500 basis points) uint256 public constant SERVICE_FEE_BPS = 2500; uint256 public constant BPS_DENOMINATOR = 10000; + + /// @notice Bound on founders per company (#6 — prevents unbounded-loop gas DoS). + uint256 public constant MAX_FOUNDERS = 50; + + /// @notice Timelock delay for the sensitive `setFeeRecipient` setter (#2). + uint256 public constant FEE_RECIPIENT_TIMELOCK = 2 days; + + /// @notice namehash("eth") — the correct parent node for `