From af598cf105845259ef4cedf08ee84dcedf555765 Mon Sep 17 00:00:00 2001 From: "Isaiah \"KyngKai\" Litt" Date: Tue, 16 Jun 2026 13:50:39 -0700 Subject: [PATCH 1/5] security: block meta-transaction self-call privilege escalation (resolver hijack) Fixes the meta-transaction self-call vector exploited against ShapeShift FOX Colony (Arbitrum, 2026-05-13, ~$182K). - BasicMetaTransaction: forbid meta-relaying the EtherRouter/DSAuth proxy-admin selectors (setResolver/setOwner/setAuthority). - CommonStorage: override `auth` to authorize via msgSender() and drop DSAuth's address(this) self-trust, closing the same class on ColonyNetwork's admin functions. - Add regression test (test/contracts-network/metatx-admin-selfcall-guard.js) and a security review document (audits/). Co-Authored-By: Claude Opus 4.8 --- .../2026-06-16-metatx-selfcall-remediation.md | 211 ++++++++++++++++++ contracts/common/BasicMetaTransaction.sol | 37 +++ contracts/common/CommonStorage.sol | 26 ++- .../metatx-admin-selfcall-guard.js | 105 +++++++++ 4 files changed, 378 insertions(+), 1 deletion(-) create mode 100644 audits/2026-06-16-metatx-selfcall-remediation.md create mode 100644 test/contracts-network/metatx-admin-selfcall-guard.js diff --git a/audits/2026-06-16-metatx-selfcall-remediation.md b/audits/2026-06-16-metatx-selfcall-remediation.md new file mode 100644 index 00000000..66209a61 --- /dev/null +++ b/audits/2026-06-16-metatx-selfcall-remediation.md @@ -0,0 +1,211 @@ +# Security Review & Remediation — Meta‑Transaction Self‑Call Privilege Escalation + +**Subject:** Colony Network smart contracts (fork maintained by Deed3Labs) +**Component:** Meta‑transaction dispatch (`executeMetaTransaction`) over the `EtherRouter` / DSAuth proxy +**Branch reviewed / remediated:** `security/metatx-proxy-admin-guard` (off `develop`) +**Prepared for:** Deed3Labs — ClearHQ deployment (Base, Base Sepolia) +**Date:** 2026‑06‑16 +**Classification of lead issue:** **CRITICAL** (CVSS 3.1 ≈ 9.3 — `AV:N/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H`) +**Status:** Remediated in code; **machine‑verification (compile + test) pending** — see §7. + +--- + +## 1. Executive summary + +On **2026‑05‑13** an attacker drained ShapeShift's *FOX Colony* on Arbitrum (~**$182,700** across two transactions) by abusing Colony's native meta‑transaction facility to **hijack the colony's proxy resolver** and then execute arbitrary code in the colony's storage context. Blockaid publicly noted that *every* Colony deployment exposing `executeMetaTransaction` on top of `EtherRouter`, on any chain, shares the vector. + +This review (a) confirms the root cause against the source, (b) **identifies a second, previously‑undisclosed instance of the same vulnerability class on `ColonyNetwork`**, (c) verifies that colony logic, extension logic, and the meta‑transaction token are *not* affected, and (d) supplies and documents a minimal two‑part remediation plus a regression test. + +| # | Finding | Severity | Status | +|---|---------|----------|--------| +| **F‑1** | Meta‑tx self‑call → `EtherRouter`/DSAuth proxy‑admin takeover (the exploited bug) | Critical | Fixed (part 1) | +| **F‑2** | Meta‑tx self‑call → `ColonyNetwork` admin functions (found in this review) | Critical | Fixed (part 2) | +| **F‑3** | Vestigial `address(this)` self‑trust in `DSAuthMeta.isAuthorized` | Informational | Hardening recommended | +| **A‑1** | Extension contracts (CoinMachine, OneTxPayment, VotingReputation, …) | No finding | Reviewed | + +--- + +## 2. Background — the incident + +A colony's on‑chain address is an **`EtherRouter`** (an upgradeable proxy). It `delegatecall`s into a `Resolver`, which maps a 4‑byte selector to the live logic contract. Changing the resolver therefore changes *all* of the colony's logic. `EtherRouter.setResolver(address)` is the legitimate upgrade hook and is gated by dappsys **`DSAuth`**. + +Colony also offers gasless UX via native meta‑transactions: a relayer submits `executeMetaTransaction(user, payload, sig)` and the contract executes `payload` "as" `user`. + +The attacker combined the two: they meta‑relayed a `setResolver` call that repointed the colony at an attacker‑controlled contract, then drained the colony through the proxy's `delegatecall` fallback. + +--- + +## 3. Scope & methodology + +**In scope:** `contracts/common/BasicMetaTransaction.sol`, `contracts/common/MetaTransactionMsgSender.sol`, `contracts/EtherRouter.sol` (and `contracts/common/EtherRouter.sol`), `lib/dappsys/auth.sol` (DSAuth), `contracts/colony/ColonyStorage.sol`, `contracts/common/CommonStorage.sol`, `contracts/colonyNetwork/*`, `contracts/extensions/*`, `contracts/metaTxToken/*`. + +**Method:** manual source review of the authorization and proxy‑dispatch paths; enumeration of every contract that (a) inherits `BasicMetaTransaction` (i.e. exposes the self‑call) and (b) exposes any function authorized by raw `msg.sender` under DSAuth's self‑trust rule. Dynamic testing was performed in code (regression suite authored) but **not executed in the review environment** (no Solidity toolchain / restricted egress) — see §7. + +--- + +## 4. Root‑cause analysis + +### 4.1 The self‑call +`executeMetaTransaction` dispatches the user's payload by calling the contract **on itself**: + +```solidity +(success, returnData) = address(this).call( + abi.encodePacked(_payload, METATRANSACTION_FLAG, _user) +); +``` + +Because this is an **external call to `address(this)`**, the *inner* call runs with **`msg.sender == address(this)`**. The signer `_user` is appended to calldata so that meta‑aware functions can recover the real sender via `msgSender()`. + +### 4.2 The self‑trust +`executeMetaTransaction` does **not** require the signer to hold any permission — it only checks that `_user` signed the payload. Anyone can therefore sign and relay their *own* meta‑transaction. + +dappsys `DSAuth` (`lib/dappsys/auth.sol`) authorizes calls as follows: + +```solidity +modifier auth virtual { require(isAuthorized(msg.sender, msg.sig), "ds-auth-unauthorized"); _; } + +function isAuthorized(address src, bytes4 sig) internal view returns (bool) { + if (src == address(this)) { return true; } // ← unconditional self‑trust + else if (src == owner) { return true; } + ... +} +``` + +The critical interaction: any contract whose `auth` modifier checks **raw `msg.sender`** (DSAuth's default) treats the meta‑transaction self‑call (`msg.sender == address(this)`) as fully authorized — **bypassing the meta‑aware `msgSender()` entirely**. + +### 4.3 Why some contracts are safe and others are not + +| Contract | `auth` resolves caller via | `address(this)` self‑trust reachable? | Exploitable via meta‑tx | +|---|---|---|---| +| Colony logic (`ColonyStorage`) | `msgSender()` (override) | No (override drops it) | **No** | +| `EtherRouter` proxy (`setResolver`/`setOwner`/`setAuthority`) | raw `msg.sender` (DSAuth) | **Yes** | **YES → F‑1** | +| `ColonyNetwork` logic (`CommonStorage`) | raw `msg.sender` (DSAuth, *not* overridden) | **Yes** | **YES → F‑2** | +| Extension logic | colony roles via `msgSender()` | n/a (no DSAuth `auth` used) | No (A‑1) | +| `MetaTxToken` (`DSAuthMeta`) | `msgSender()` (modifier) | Unreachable (modifier passes `msgSender()`) | No (F‑3 is dead code) | + +--- + +## 5. Findings + +### F‑1 — Meta‑transaction self‑call → proxy‑admin takeover *(Critical — exploited 2026‑05‑13)* + +**Affected:** `EtherRouter.setResolver(address)` and the inherited DSAuth `setOwner(address)` / `setAuthority(address)` on every `EtherRouter`‑fronted contract (colonies, the network, and each installed extension). + +**Attack (resolver hijack):** +1. Attacker crafts `payload = setResolver(EVIL)` and signs a meta‑transaction over it (no permission required). +2. `colony.executeMetaTransaction(ATTACKER, payload, sig)` → `address(this).call(payload …)`. +3. Inner call hits `EtherRouter.setResolver` with `msg.sender == address(this)`; DSAuth self‑trust authorizes it. +4. Resolver now points at `EVIL`; every subsequent call `delegatecall`s attacker code in the colony's storage → **funds drained**. + +`setOwner`/`setAuthority` give equivalent takeover (attacker becomes owner / installs a permissive authority). + +**Remediation (part 1):** reject the proxy‑admin selectors inside `executeMetaTransaction`, *before* the self‑call. Because the legitimate upgrade path is never a meta‑transaction, this has no functional impact. See §6.1. + +--- + +### F‑2 — Meta‑transaction self‑call → `ColonyNetwork` admin functions *(Critical — identified in this review)* + +**Affected:** `ColonyNetwork` (and any other `CommonStorage`‑based contract that exposes `executeMetaTransaction`). `CommonStorage` is `is DSAuth, MetaTransactionMsgSender` and **does not override `auth`** — so the network's `auth`‑guarded admin functions (`setTokenLocking`, `initialise`, `addColonyVersion`, `addNetworkColonyVersion`, `startTokenAuction`, reputation‑mining setters, bridge setters, …) inherit the same raw‑`msg.sender` self‑trust. + +**Attack:** `colonyNetwork.executeMetaTransaction(ATTACKER, setTokenLocking(EVIL), sig)` → self‑call → `msg.sender == address(this)` → DSAuth self‑trust → executes. The colony‑level fix (`ColonyStorage`'s override) does **not** cover the network; the part‑1 selector blocklist does **not** cover these (non‑proxy) selectors. + +**Remediation (part 2):** give `CommonStorage` the same meta‑safe `auth` override that `ColonyStorage` already uses — authorize via `msgSender()` and **drop** the `address(this)` self‑trust. See §6.2. + +--- + +### F‑3 — Vestigial self‑trust in `DSAuthMeta.isAuthorized` *(Informational)* + +`DSAuthMeta.isAuthorized` retains `if (src == address(this)) return true;`. It is currently **unreachable for exploitation** because `DSAuthMeta.auth` passes `msgSender()` (which resolves to the appended `_user` during a meta‑tx self‑call, never `address(this)`). It is nonetheless a latent footgun; recommend deleting the branch so the property is structural, not incidental. + +--- + +### A‑1 — Extension contracts *(No finding)* + +All contracts under `contracts/extensions/` that inherit `BasicMetaTransaction` (CoinMachine, OneTxPayment, FundingQueue, TokenSupplier, Whitelist, EvaluatedExpenditure, VotingReputation, and the `ColonyExtensionMeta` base) were reviewed. **None uses the bare DSAuth `auth` modifier on any function**; they authorize via the colony's role system through `msgSender()` (e.g. `colony.hasInheritedUserRole(msgSender(), …)`). Their only DSAuth self‑trust surface is the proxy‑admin trio on their own `EtherRouter`, which is covered by part‑1. **No additional fix required.** + +--- + +## 6. Remediation (applied on `security/metatx-proxy-admin-guard`) + +### 6.1 Part 1 — `contracts/common/BasicMetaTransaction.sol` +Block the EtherRouter/DSAuth proxy‑admin selectors from ever being dispatched through a meta‑transaction: + +```solidity +interface IProxyAdminSelectors { + function setResolver(address) external; + function setOwner(address) external; + function setAuthority(address) external; +} +// … +bytes4 private constant PROXY_ADMIN_SET_RESOLVER = IProxyAdminSelectors.setResolver.selector; +bytes4 private constant PROXY_ADMIN_SET_OWNER = IProxyAdminSelectors.setOwner.selector; +bytes4 private constant PROXY_ADMIN_SET_AUTHORITY = IProxyAdminSelectors.setAuthority.selector; +// … inside executeMetaTransaction, after signature verification: +require(_payload.length >= 4, "colony-metatx-payload-too-short"); +bytes4 targetSig; +assembly { targetSig := mload(add(_payload, 0x20)) } +require( + targetSig != PROXY_ADMIN_SET_RESOLVER && + targetSig != PROXY_ADMIN_SET_OWNER && + targetSig != PROXY_ADMIN_SET_AUTHORITY, + "colony-metatx-admin-selector-forbidden" +); +``` + +### 6.2 Part 2 — `contracts/common/CommonStorage.sol` +Make the network's authorization meta‑aware and self‑trust‑free (mirrors `ColonyStorage`): + +```solidity +modifier auth() virtual override { + require(authorizedSender(msgSender(), msg.sig), "ds-auth-unauthorized"); + _; +} +function authorizedSender(address src, bytes4 sig) internal view returns (bool) { + if (src == owner) { return true; } + else if (authority == DSAuthority(0)) { return false; } + else { return authority.canCall(src, address(this), sig); } +} +``` + +### 6.3 Regression test — `test/contracts-network/metatx-admin-selfcall-guard.js` +Asserts: meta‑relayed `setResolver`/`setOwner`/`setAuthority` revert (`colony-metatx-admin-selector-forbidden`) and the resolver is unchanged; a legitimate non‑admin meta‑transaction from a permitted signer still succeeds; a meta‑relayed `ColonyNetwork.setTokenLocking` reverts (`ds-auth-unauthorized`). + +--- + +## 7. Verification status (read this before deploying) + +This report **does not constitute a passing test run**. The review environment had no Solidity toolchain and restricted network egress, so the contracts were **not compiled or executed** here. Before any deployment the maintainer MUST: + +1. `npx hardhat compile` on the branch. *Watch:* the `CommonStorage.auth` override sits in the `DSAuth → CommonStorage → ColonyStorage` chain; if solc requires `ColonyStorage`'s existing `modifier auth() override` to become `override(CommonStorage)`, apply that one‑line change. +2. Run the new regression test **and the full suite**, with particular attention to network **`initialise` / upgrade / cross‑chain** flows (part 2 changes authorization on the network's critical path). +3. Deploy to **Base Sepolia** and **replay the exploit** (sign an attacker meta‑tx calling `setResolver` against a live colony) to confirm an on‑chain revert. +4. Commission an **independent third‑party audit** of the meta‑tx / auth / proxy surface. This review found a second critical instance (F‑2) by manual inspection; that is evidence the area warrants external eyes, not a clean bill. + +--- + +## 8. Residual risk & recommendations + +- **Selector blocklist is exact, not categorical.** Part 1 enumerates the three known self‑trusting proxy selectors. Part 2 (categorical, via `msgSender()`) is the durable defense for logic functions. New `EtherRouter`/DSAuth admin functions, if ever added, must be added to the blocklist. +- **Defense in depth (optional):** removing `if (src == address(this)) return true;` from `lib/dappsys/auth.sol` would neutralize the entire class at the root, but it touches the shared proxy/upgrade path and must be validated against every legitimate self‑call — treat as a separate, fully‑tested change. +- **F‑3:** delete the dead self‑trust branch in `DSAuthMeta`. +- **Deployment posture:** deploy fresh, fixed contracts (do not build on the abandoned, unpatched live deployment); maintainer retains upgrade keys, so future issues can be resolved via resolver upgrade. + +--- + +## 9. Conclusion + +The vulnerability exploited against FOX Colony is **fully root‑caused** and a **minimal, targeted remediation** is in place for both the disclosed vector (F‑1) and a second critical instance discovered during this review (F‑2). Colony logic, extension logic, and the meta‑transaction token were verified **not** to be affected. Subject to the verification gate in §7 — compilation, full test suite, a Base Sepolia exploit‑replay, and an independent audit — the fork is sound to deploy. This document should be read **together with** the passing CI run that fulfills §7; on its own it certifies the analysis and the fix, not their execution. + +--- + +### Appendix A — changed files +``` +contracts/common/BasicMetaTransaction.sol | +37 +contracts/common/CommonStorage.sol | +26 −1 +test/contracts-network/metatx-admin-selfcall-guard.js | (new) +``` + +### Appendix B — error signatures introduced +- `colony-metatx-admin-selector-forbidden` — meta‑tx targeted a proxy‑admin selector (F‑1). +- `colony-metatx-payload-too-short` — meta‑tx payload shorter than a 4‑byte selector. +- `ds-auth-unauthorized` — (existing) now also raised for self‑call attempts on `CommonStorage` admin functions (F‑2). diff --git a/contracts/common/BasicMetaTransaction.sol b/contracts/common/BasicMetaTransaction.sol index acb30b0b..1c13d1cb 100644 --- a/contracts/common/BasicMetaTransaction.sol +++ b/contracts/common/BasicMetaTransaction.sol @@ -6,12 +6,26 @@ import { MetaTransactionMsgSender } from "./MetaTransactionMsgSender.sol"; import { MultiChain } from "./MultiChain.sol"; import { IBasicMetaTransaction } from "./IBasicMetaTransaction.sol"; +/// @notice Selectors of the EtherRouter / dappsys-DSAuth proxy-admin functions. These are +/// authorized purely on `msg.sender == address(this)` (they do NOT use the meta-aware +/// `msgSender()`), so they must never be reachable via a meta-transaction's self-call. +interface IProxyAdminSelectors { + function setResolver(address) external; + function setOwner(address) external; + function setAuthority(address) external; +} + abstract contract BasicMetaTransaction is IBasicMetaTransaction, DSMath, MetaTransactionMsgSender, MultiChain { + // Proxy / DSAuth admin selectors that must never be invoked via a meta-transaction (see executeMetaTransaction). + bytes4 private constant PROXY_ADMIN_SET_RESOLVER = IProxyAdminSelectors.setResolver.selector; + bytes4 private constant PROXY_ADMIN_SET_OWNER = IProxyAdminSelectors.setOwner.selector; + bytes4 private constant PROXY_ADMIN_SET_AUTHORITY = IProxyAdminSelectors.setAuthority.selector; + function getMetatransactionNonce(address _user) public view virtual returns (uint256 nonce); // NB if implementing this functionality in a contract with recovery mode, @@ -40,6 +54,29 @@ abstract contract BasicMetaTransaction is verify(_user, getMetatransactionNonce(_user), block.chainid, _payload, _sigR, _sigS, _sigV), "metatransaction-signer-signature-mismatch" ); + + // SECURITY (resolver-hijack / proxy takeover): the payload below is dispatched via a + // self-call (address(this).call), so the inner call runs with msg.sender == address(this). + // The EtherRouter proxy and the dappsys DSAuth it inherits authorize ANY caller where + // msg.sender == address(this) and do NOT route through the meta-aware msgSender(). Without + // this guard, anyone can meta-relay setResolver/setOwner/setAuthority and take over the + // contract's proxy (repoint the resolver -> attacker-controlled delegatecall -> full drain). + // Colony's own `auth` uses msgSender() and is unaffected; these three admin selectors are the + // only self-trusting surface, so they must never be reachable through a meta-transaction. + // Ref: ShapeShift FOX Colony exploit, Arbitrum, 2026-05-13. + require(_payload.length >= 4, "colony-metatx-payload-too-short"); + bytes4 targetSig; + // solhint-disable-next-line no-inline-assembly + assembly { + targetSig := mload(add(_payload, 0x20)) + } + require( + targetSig != PROXY_ADMIN_SET_RESOLVER && + targetSig != PROXY_ADMIN_SET_OWNER && + targetSig != PROXY_ADMIN_SET_AUTHORITY, + "colony-metatx-admin-selector-forbidden" + ); + incrementMetatransactionNonce(_user); // Append _user at the end to extract it from calling context diff --git a/contracts/common/CommonStorage.sol b/contracts/common/CommonStorage.sol index b263764b..216423f6 100644 --- a/contracts/common/CommonStorage.sol +++ b/contracts/common/CommonStorage.sol @@ -18,7 +18,7 @@ pragma solidity 0.8.28; -import { DSAuth } from "./../../lib/dappsys/auth.sol"; +import { DSAuth, DSAuthority } from "./../../lib/dappsys/auth.sol"; import { MetaTransactionMsgSender } from "./../common/MetaTransactionMsgSender.sol"; // ignore-file-swc-131 @@ -64,4 +64,28 @@ abstract contract CommonStorage is DSAuth, MetaTransactionMsgSender { modifier always() { _; } + + // SECURITY: override DSAuth's `auth` so authorization uses the meta-aware msgSender() and does + // NOT honour DSAuth's `msg.sender == address(this)` self-trust. executeMetaTransaction dispatches + // its payload via a self-call (address(this).call), so without this override anyone could + // meta-relay an `auth`-guarded admin function on a CommonStorage-based contract (e.g. + // ColonyNetwork: setTokenLocking / initialise / addColonyVersion ...) because the inner call + // runs with msg.sender == address(this). Colonies already override `auth` in ColonyStorage; this + // closes the same hole for ColonyNetwork and any other CommonStorage-based contract. + // Ref: ShapeShift FOX Colony exploit, Arbitrum, 2026-05-13. + modifier auth() virtual override { + require(authorizedSender(msgSender(), msg.sig), "ds-auth-unauthorized"); + _; + } + + // DSAuth.isAuthorized, minus the `src == address(this)` self-trust (see the `auth` override above). + function authorizedSender(address src, bytes4 sig) internal view returns (bool) { + if (src == owner) { + return true; + } else if (authority == DSAuthority(0)) { + return false; + } else { + return authority.canCall(src, address(this), sig); + } + } } diff --git a/test/contracts-network/metatx-admin-selfcall-guard.js b/test/contracts-network/metatx-admin-selfcall-guard.js new file mode 100644 index 00000000..f84ca60f --- /dev/null +++ b/test/contracts-network/metatx-admin-selfcall-guard.js @@ -0,0 +1,105 @@ +/* globals artifacts */ + +// Regression tests for the meta-transaction self-call privilege-escalation that was exploited +// against ShapeShift's FOX Colony on Arbitrum (2026-05-13, ~$182K). +// +// Root cause: executeMetaTransaction dispatches its payload via address(this).call(...), so the +// inner call runs with msg.sender == address(this). DSAuth (used by the EtherRouter proxy and, +// for the network, via CommonStorage) authorizes any caller where msg.sender == address(this). +// Without the fixes, anyone could meta-relay setResolver/setOwner/setAuthority (proxy takeover -> +// resolver hijack -> full drain) or, on the ColonyNetwork, its admin functions. +// +// Fixes under test: +// 1. BasicMetaTransaction.executeMetaTransaction blocks the EtherRouter/DSAuth admin selectors. +// 2. CommonStorage overrides `auth` to use msgSender() and drop the address(this) self-trust, +// protecting ColonyNetwork's admin functions. + +const chai = require("chai"); +const bnChai = require("bn-chai"); + +const { checkErrorRevert, expectEvent } = require("../../helpers/test-helper"); +const { getMetaTransactionParameters, setupRandomColony } = require("../../helpers/test-data-generator"); + +const { expect } = chai; +chai.use(bnChai(web3.utils.BN)); + +const EtherRouter = artifacts.require("EtherRouter"); +const IColonyNetwork = artifacts.require("IColonyNetwork"); +const Resolver = artifacts.require("Resolver"); + +contract("Meta-transaction security: admin self-call guard", (accounts) => { + const ROOT = accounts[0]; + const ATTACKER = accounts[5]; + const RELAYER = accounts[6]; + + let colonyNetwork; + let colony; + let colonyRouter; + let originalResolver; + + before(async () => { + const cnAddress = (await EtherRouter.deployed()).address; + colonyNetwork = await IColonyNetwork.at(cnAddress); + }); + + beforeEach(async () => { + ({ colony } = await setupRandomColony(colonyNetwork)); + // A colony's address IS an EtherRouter proxy. + colonyRouter = await EtherRouter.at(colony.address); + originalResolver = await colonyRouter.resolver(); + }); + + // ATTACKER signs and relays an arbitrary payload against an EtherRouter-fronted contract. + async function relayAsAttacker(target, txData) { + const { r, s, v } = await getMetaTransactionParameters(txData, ATTACKER, target.address); + return target.executeMetaTransaction(ATTACKER, txData, r, s, v, { from: RELAYER }); + } + + describe("colony — EtherRouter / DSAuth proxy-admin trio", () => { + it("blocks a meta-relayed setResolver (the resolver-hijack drain vector)", async () => { + const evilResolver = await Resolver.new(); + const txData = colonyRouter.contract.methods.setResolver(evilResolver.address).encodeABI(); + + await checkErrorRevert(relayAsAttacker(colony, txData), "colony-metatx-admin-selector-forbidden"); + + // The resolver must be untouched. + expect(await colonyRouter.resolver()).to.equal(originalResolver); + }); + + it("blocks a meta-relayed setOwner (proxy takeover)", async () => { + const txData = colonyRouter.contract.methods.setOwner(ATTACKER).encodeABI(); + await checkErrorRevert(relayAsAttacker(colony, txData), "colony-metatx-admin-selector-forbidden"); + expect(await colonyRouter.owner()).to.not.equal(ATTACKER); + }); + + it("blocks a meta-relayed setAuthority (proxy takeover)", async () => { + const txData = colonyRouter.contract.methods.setAuthority(ATTACKER).encodeABI(); + await checkErrorRevert(relayAsAttacker(colony, txData), "colony-metatx-admin-selector-forbidden"); + }); + + it("still allows a legitimate (non-admin) meta-transaction from a permitted signer", async () => { + // ROOT created the colony in setupRandomColony and holds the Root role. + const txData = colony.contract.methods.setRewardInverse(50).encodeABI(); + const { r, s, v } = await getMetaTransactionParameters(txData, ROOT, colony.address); + + await expectEvent( + colony.executeMetaTransaction(ROOT, txData, r, s, v, { from: RELAYER }), + "ColonyRewardInverseSet", + [ROOT, 50], + ); + }); + }); + + describe("ColonyNetwork — CommonStorage auth (no address(this) self-trust)", () => { + it("blocks a meta-relayed network admin function (setTokenLocking) by an unauthorized signer", async () => { + const tokenLockingBefore = await colonyNetwork.getTokenLocking(); + const txData = colonyNetwork.contract.methods.setTokenLocking(ATTACKER).encodeABI(); + + // CommonStorage's overridden `auth` resolves the signer via msgSender() (== ATTACKER, no + // permission) and no longer self-trusts address(this), so the self-call is rejected. + await checkErrorRevert(relayAsAttacker(colonyNetwork, txData), "ds-auth-unauthorized"); + + expect(await colonyNetwork.getTokenLocking()).to.equal(tokenLockingBefore); + }); + }); +}); From ad558e429cbdeea22f0f162576b9409071b888ad Mon Sep 17 00:00:00 2001 From: "Isaiah \"KyngKai\" Litt" Date: Tue, 16 Jun 2026 14:00:46 -0700 Subject: [PATCH 2/5] ci: add GitHub Actions workflow to compile + run the meta-tx security test Upstream CI is CircleCI tied to private DockerHub creds; this runs on GitHub-hosted runners so it works on the fork with no external setup. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/security-check.yml | 48 ++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 .github/workflows/security-check.yml diff --git a/.github/workflows/security-check.yml b/.github/workflows/security-check.yml new file mode 100644 index 00000000..299699e7 --- /dev/null +++ b/.github/workflows/security-check.yml @@ -0,0 +1,48 @@ +name: security-check + +# Lightweight CI for the Deed3Labs fork: compile the contracts and run the +# meta-transaction self-call security regression test. Runs on GitHub-hosted +# runners (no external CI account / DockerHub secrets required, unlike the +# upstream CircleCI config). + +on: + push: + branches: + - "security/**" + pull_request: + workflow_dispatch: {} + +permissions: + contents: read + +jobs: + compile-and-test: + runs-on: ubuntu-latest + timeout-minutes: 60 + env: + HUSKY: 0 + NODE_OPTIONS: --max-old-space-size=6144 + steps: + - name: Checkout (with submodules) + uses: actions/checkout@v4 + with: + submodules: recursive + + - name: Set up Node 20.11.0 + uses: actions/setup-node@v4 + with: + node-version: "20.11.0" + + - name: Set up pnpm 8.14.1 + uses: pnpm/action-setup@v4 + with: + version: "8.14.1" + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - name: Compile contracts + run: npx hardhat compile + + - name: Meta-transaction security regression test + run: npx hardhat test ./test/contracts-network/metatx-admin-selfcall-guard.js From 9bbbe82e2be3d1f85360cf27d18fcdf5df86886e Mon Sep 17 00:00:00 2001 From: "Isaiah \"KyngKai\" Litt" Date: Tue, 16 Jun 2026 14:09:12 -0700 Subject: [PATCH 3/5] fix: DSAuthority(address(0)) cast in CommonStorage auth override (solc 0.8) Co-Authored-By: Claude Opus 4.8 --- contracts/common/CommonStorage.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/common/CommonStorage.sol b/contracts/common/CommonStorage.sol index 216423f6..68adf8ff 100644 --- a/contracts/common/CommonStorage.sol +++ b/contracts/common/CommonStorage.sol @@ -82,7 +82,7 @@ abstract contract CommonStorage is DSAuth, MetaTransactionMsgSender { function authorizedSender(address src, bytes4 sig) internal view returns (bool) { if (src == owner) { return true; - } else if (authority == DSAuthority(0)) { + } else if (authority == DSAuthority(address(0))) { return false; } else { return authority.canCall(src, address(this), sig); From 1a3362cdf6581b9bfe10953c58c6ef9a38886375 Mon Sep 17 00:00:00 2001 From: "Isaiah \"KyngKai\" Litt" Date: Tue, 16 Jun 2026 14:16:07 -0700 Subject: [PATCH 4/5] test: expect executeMetaTransaction wrapper revert for the network admin case Co-Authored-By: Claude Opus 4.8 --- test/contracts-network/metatx-admin-selfcall-guard.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/contracts-network/metatx-admin-selfcall-guard.js b/test/contracts-network/metatx-admin-selfcall-guard.js index f84ca60f..6a94ce19 100644 --- a/test/contracts-network/metatx-admin-selfcall-guard.js +++ b/test/contracts-network/metatx-admin-selfcall-guard.js @@ -96,8 +96,11 @@ contract("Meta-transaction security: admin self-call guard", (accounts) => { const txData = colonyNetwork.contract.methods.setTokenLocking(ATTACKER).encodeABI(); // CommonStorage's overridden `auth` resolves the signer via msgSender() (== ATTACKER, no - // permission) and no longer self-trusts address(this), so the self-call is rejected. - await checkErrorRevert(relayAsAttacker(colonyNetwork, txData), "ds-auth-unauthorized"); + // permission) and no longer self-trusts address(this), so the inner self-call reverts with + // `ds-auth-unauthorized`. executeMetaTransaction surfaces any failed self-call as its generic + // wrapper, so that is the revert the caller observes; the state-unchanged assertion below + // confirms the admin function did NOT execute (without the fix it would succeed). + await checkErrorRevert(relayAsAttacker(colonyNetwork, txData), "colony-metatx-function-call-unsuccessful"); expect(await colonyNetwork.getTokenLocking()).to.equal(tokenLockingBefore); }); From dbf74c947a0ed2819a8d55fa5f130254c9fb07b7 Mon Sep 17 00:00:00 2001 From: "Isaiah \"KyngKai\" Litt" Date: Tue, 16 Jun 2026 14:21:32 -0700 Subject: [PATCH 5/5] docs: mark compile + regression test green in CI in the audit report Co-Authored-By: Claude Opus 4.8 --- audits/2026-06-16-metatx-selfcall-remediation.md | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/audits/2026-06-16-metatx-selfcall-remediation.md b/audits/2026-06-16-metatx-selfcall-remediation.md index 66209a61..9cb31535 100644 --- a/audits/2026-06-16-metatx-selfcall-remediation.md +++ b/audits/2026-06-16-metatx-selfcall-remediation.md @@ -6,7 +6,7 @@ **Prepared for:** Deed3Labs — ClearHQ deployment (Base, Base Sepolia) **Date:** 2026‑06‑16 **Classification of lead issue:** **CRITICAL** (CVSS 3.1 ≈ 9.3 — `AV:N/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H`) -**Status:** Remediated in code; **machine‑verification (compile + test) pending** — see §7. +**Status:** Remediated; full‑tree **compile + targeted regression test are green in CI** (GitHub Actions `security-check`). Full suite / Base Sepolia replay / independent audit still required — see §7. --- @@ -174,12 +174,11 @@ Asserts: meta‑relayed `setResolver`/`setOwner`/`setAuthority` revert (`colony- ## 7. Verification status (read this before deploying) -This report **does not constitute a passing test run**. The review environment had no Solidity toolchain and restricted network egress, so the contracts were **not compiled or executed** here. Before any deployment the maintainer MUST: +The fix has been **compiled against the full contract tree, and the regression test passes**, in CI (GitHub Actions `security-check` on `Deed3Labs/colonyNetwork` — green): the meta‑relayed `setResolver`/`setOwner`/`setAuthority` revert and the resolver is unchanged; the network `setTokenLocking` self‑call is rejected and its state is unchanged; and a legitimate non‑admin meta‑transaction still succeeds. The override‑chain compiled without needing an `override(CommonStorage)` change. (CI caught and the maintainer fixed two issues en route: a `DSAuthority(address(0))` cast and a test assertion string.) Before any deployment the maintainer MUST still: -1. `npx hardhat compile` on the branch. *Watch:* the `CommonStorage.auth` override sits in the `DSAuth → CommonStorage → ColonyStorage` chain; if solc requires `ColonyStorage`'s existing `modifier auth() override` to become `override(CommonStorage)`, apply that one‑line change. -2. Run the new regression test **and the full suite**, with particular attention to network **`initialise` / upgrade / cross‑chain** flows (part 2 changes authorization on the network's critical path). -3. Deploy to **Base Sepolia** and **replay the exploit** (sign an attacker meta‑tx calling `setResolver` against a live colony) to confirm an on‑chain revert. -4. Commission an **independent third‑party audit** of the meta‑tx / auth / proxy surface. This review found a second critical instance (F‑2) by manual inspection; that is evidence the area warrants external eyes, not a clean bill. +1. Run the **full test suite** — the CI above runs only compilation plus the targeted regression test — with particular attention to network **`initialise` / upgrade / cross‑chain** flows (part 2 changes authorization on the network's critical path). +2. Deploy to **Base Sepolia** and **replay the exploit** (sign an attacker meta‑tx calling `setResolver` against a live colony) to confirm an on‑chain revert. +3. Commission an **independent third‑party audit** of the meta‑tx / auth / proxy surface. This review found a second critical instance (F‑2) by manual inspection; that is evidence the area warrants external eyes, not a clean bill. ---