diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a896cba..8659b42 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,12 +2,20 @@ name: Quality checks on: push: + branches: + - main pull_request: + branches: + - main workflow_dispatch: permissions: contents: read +concurrency: + group: quality-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + env: FOUNDRY_PROFILE: ci @@ -18,10 +26,10 @@ jobs: timeout-minutes: 15 steps: - name: Checkout - uses: actions/checkout@v7 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install Foundry - uses: foundry-rs/foundry-toolchain@v1 + uses: foundry-rs/foundry-toolchain@908c540300062bd5a7e473851cdb4282204cee09 # v1.9.1 with: version: v1.7.1 cache: true @@ -29,7 +37,7 @@ jobs: - name: Install dependencies run: | forge install foundry-rs/forge-std@v1.16.2 --no-git - forge install OpenZeppelin/openzeppelin-contracts@v5.4.0 --no-git + forge install OpenZeppelin/openzeppelin-contracts@v5.7.0 --no-git - name: Formatting run: forge fmt --check @@ -51,15 +59,15 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 15 steps: - - uses: actions/checkout@v7 - - uses: foundry-rs/foundry-toolchain@v1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: foundry-rs/foundry-toolchain@908c540300062bd5a7e473851cdb4282204cee09 # v1.9.1 with: version: v1.7.1 - name: Install dependencies run: | forge install foundry-rs/forge-std@v1.16.2 --no-git - forge install OpenZeppelin/openzeppelin-contracts@v5.4.0 --no-git + forge install OpenZeppelin/openzeppelin-contracts@v5.7.0 --no-git - name: Install Slither - run: pipx install slither-analyzer==0.11.5 + run: pipx install slither-analyzer==0.11.6 - name: Run Slither - run: slither . --config-file slither.config.json --exclude arbitrary-send-erc20,timestamp + run: slither . --config-file slither.config.json diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cc3febf..7fe2ae1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -2,10 +2,49 @@ This repository is primarily a portfolio/reference implementation, but focused improvements are welcome. -1. Open an issue for protocol-level behavior changes. -2. Keep settlement logic minimal and avoid adding privileged admin surfaces without a clear threat-model update. -3. Add unit tests for expected/revert paths, fuzz tests for input boundaries and invariant tests for stateful accounting changes. -4. Run `make check` before opening a pull request. -5. Update `docs/THREAT-MODEL.md` when assumptions or trust boundaries change. +## Prerequisites and setup + +- Git; +- Foundry 1.7.1 or a compatible newer stable release; +- Python and Slither 0.11.6 for the full static-analysis gate. + +```bash +git clone https://github.com/alsaecas/orderforge.git +cd orderforge +make install +pipx install slither-analyzer==0.11.6 +``` + +## Development checks + +Run individual layers while iterating: + +```bash +make fmt +make lint +make build +make test-unit +make test-fuzz +make test-invariant +make coverage +make slither +``` + +Before opening a pull request, run `make ci`. This uses the `ci` Foundry profile: 4,096 fuzz runs and 512 invariant runs at depth 128. + +## Protocol and security expectations + +1. Open an issue before changing signed fields, domain semantics, nonce/cancellation behavior, settlement arithmetic or trust assumptions. +2. Keep settlement minimal; do not add privileged admin, custody, fee, governance or upgrade surfaces without an explicit protocol-design decision. +3. Add focused unit regressions, property-oriented fuzz tests and stateful invariants appropriate to the change. +4. Preserve checks-effects-interactions, direct settlement and standard ERC-20 assumptions unless the threat model is deliberately revised. +5. Update `docs/THREAT-MODEL.md`, architecture and ABI documentation when behavior or assumptions change. + +## Pull requests and commits + +- Keep PRs reviewable and explain security implications, test results and coverage effects. +- Do not commit `lib/`, `out/`, `cache/`, broadcast artifacts, environment files, RPC URLs or private keys. +- Use concise Conventional Commit-style subjects where practical, such as `test:`, `fix:`, `docs:`, `ci:` or `chore:`. +- Ensure every required GitHub check is green and address review feedback before merge. Dependencies are installed with `make install`; generated Foundry output and dependency directories are intentionally not committed. diff --git a/Makefile b/Makefile index 1177ead..19c5637 100644 --- a/Makefile +++ b/Makefile @@ -1,10 +1,10 @@ SHELL := /bin/bash -.PHONY: install fmt lint build test test-unit test-fuzz test-invariant coverage slither check clean +.PHONY: install fmt lint build test test-unit test-fuzz test-invariant coverage slither check ci clean install: forge install foundry-rs/forge-std@v1.16.2 --no-git - forge install OpenZeppelin/openzeppelin-contracts@v5.4.0 --no-git + forge install OpenZeppelin/openzeppelin-contracts@v5.7.0 --no-git fmt: forge fmt --check @@ -33,7 +33,10 @@ coverage: slither: slither . --config-file slither.config.json -check: fmt lint build test +check: fmt lint build test coverage slither + +ci: + FOUNDRY_PROFILE=ci $(MAKE) check clean: forge clean diff --git a/README.md b/README.md index 3dff28d..f627b38 100644 --- a/README.md +++ b/README.md @@ -106,9 +106,12 @@ The invariant handler randomly fills, cancels and invalidates the same signed or - token totals are conserved; - OrderForge does not retain settlement tokens; - cumulative payment never exceeds the signed price and equals it at completion. +- cancellation, nonce invalidation and completion prevent every later fill attempt. Read [Testing strategy](docs/TESTING.md). +The current suite contains 35 unit tests, 4 fuzz properties and 5 stateful invariants. A local Foundry 1.5.1 coverage run against Solidity 0.8.30 reports 100% lines, statements, branches and functions across every contract under `src/`. The repository-wide aggregate is lower because it honestly includes the deployment script and test helpers; exact figures are recorded in the testing guide. + ## Repository structure ```text @@ -149,6 +152,7 @@ make test-invariant make coverage make lint make slither +make ci # full suite with CI-strength fuzz/invariant settings ``` ## Deployment diff --git a/SECURITY.md b/SECURITY.md index baf53eb..0d662b2 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -2,9 +2,23 @@ OrderForge is **educational portfolio software**. It has not been audited and should not be used with meaningful funds. +## Supported versions + +Security fixes are provided for the latest commit on `main`. No released version is currently supported because the project has not yet published a stable release. Historical commits and forks are outside the maintainer's support scope. + ## Reporting a vulnerability -Please use GitHub private security advisories rather than opening a public issue for suspected vulnerabilities. Include a minimal reproduction, affected commit, impact and any suggested mitigation. +Please use [GitHub private security advisories](https://github.com/alsaecas/orderforge/security/advisories/new) rather than opening a public issue for suspected vulnerabilities. Do not disclose the issue publicly before a fix and coordinated disclosure timeline are agreed. + +A useful report includes: + +- the affected commit or version and execution path; +- a concise impact assessment and realistic attack prerequisites; +- a minimal Foundry reproduction or transaction sequence; +- relevant order fields, state transitions and token assumptions; +- a suggested mitigation, if available. + +The maintainer will acknowledge the report, validate its scope and coordinate remediation and disclosure through the private advisory. No bug-bounty payment is promised. ## Security assumptions @@ -25,3 +39,5 @@ Please use GitHub private security advisories rather than opening a public issue - Cumulative floor rounding so accepted partial fills never transfer sell tokens for zero payment and the full order still settles to the exact signed price. - No intended token custody by the settlement contract. - Stateful invariant tests assert no overfill, token conservation and zero protocol custody. + +Automated tests, coverage and static analysis are engineering controls, not substitutes for a professional audit. OrderForge has **not** undergone a professional smart-contract security audit. diff --git a/docs/STATIC-ANALYSIS.md b/docs/STATIC-ANALYSIS.md index 0fec608..5139439 100644 --- a/docs/STATIC-ANALYSIS.md +++ b/docs/STATIC-ANALYSIS.md @@ -1,6 +1,6 @@ # Static analysis -OrderForge runs Slither in CI and treats unexpected findings as build failures. Two detector classes are explicitly excluded because they flag intentional protocol mechanics rather than defects. +OrderForge pins Slither 0.11.6 in CI and treats unexpected findings as build failures. Two detector classes are explicitly excluded in `slither.config.json` because they flag intentional protocol mechanics rather than defects. ## `arbitrary-send-erc20` @@ -11,3 +11,7 @@ OrderForge runs Slither in CI and treats unexpected findings as build failures. `block.timestamp` is used only to enforce the maker-signed order expiry. Small miner/validator timestamp latitude cannot alter the signed exchange rate, recipient, token pair or fill limits; it only affects the exact boundary at which an order becomes expired. These exclusions are narrow and documented. Other Slither findings continue to fail CI. + +The configuration filters dependencies, tests and deployment scripts so findings are scoped to production contracts. Run the same gate locally with `make slither`; CI does not add hidden command-line suppressions. + +The final local Slither 0.11.6 run analyzed 27 contracts with 100 active detectors and produced zero results after the two exclusions above. diff --git a/docs/TESTING.md b/docs/TESTING.md index 0fc16bd..a72d0d1 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -10,10 +10,12 @@ The unit suite covers: - cumulative floor rounding across partial fills, including minimum-resolution rejection; - taker restrictions and expiry; - invalid signatures; +- malformed, mutated and cross-chain-domain signatures; - maker cancellation and nonce invalidation; - first-fill nonce binding; - EIP-712 verifying-contract domain separation; - ERC-1271 contract-wallet signatures; +- ERC-1271 rejection and false-returning ERC-20 rollback behavior; - `abi.encode` / `abi.decode` round trips; - dynamic execution-payload encoding; - `abi.encodeCall` selectors; @@ -22,16 +24,35 @@ The unit suite covers: ## Fuzz tests -Fuzz tests vary order sizes and fill sizes, then assert that balances and recorded fills agree. Codec fuzzing checks round-trip preservation over arbitrary typed values. Market-ID fuzzing checks token-order symmetry. +Fuzz tests vary order sizes and fill sizes, then assert that balances and recorded fills agree. A two-partition property proves cumulative settlement reaches the exact signed buy amount across arbitrary valid ratios. Codec fuzzing checks round-trip preservation over arbitrary typed values. Market-ID fuzzing checks token-order symmetry. ## Stateful invariants -A handler randomly alternates between partial fills, order cancellation and nonce invalidation. Across arbitrary call sequences the suite requires: +A handler randomly alternates between partial fills, order cancellation and nonce invalidation. It continues attempting fills after terminal state transitions and records any unexpected success. Across arbitrary call sequences the suite requires: 1. `filledSellAmount <= sellAmount`; 2. handler ghost accounting equals protocol accounting; 3. total sell-token and buy-token balances are conserved; 4. OrderForge never retains either settlement token; 5. cumulative payment never exceeds `buyAmount` and equals it on a full fill. +6. cancellation, nonce invalidation and completion permanently block further fills. CI increases the fuzz and invariant run counts through the `ci` Foundry profile. + +## Current verified baseline + +The final hardening suite contains: + +- 35 deterministic unit tests; +- 4 fuzz properties; +- 5 stateful invariants; +- 44 total Forge test functions. + +With Solidity 0.8.30 and local Foundry 1.5.1, `forge coverage --report summary` reports: + +| Scope | Lines | Statements | Branches | Functions | +| --- | ---: | ---: | ---: | ---: | +| Production contracts under `src/` | 100% (104/104) | 100% (127/127) | 100% (22/22) | 100% (27/27) | +| Repository aggregate | 94.23% (147/156) | 93.96% (171/182) | 86.21% (25/29) | 94.87% (37/39) | + +The aggregate includes the unexecuted deployment script and partially instrumented invariant/mock helpers. It is not presented as production-contract coverage. diff --git a/foundry.toml b/foundry.toml index 672b393..57ca64c 100644 --- a/foundry.toml +++ b/foundry.toml @@ -19,6 +19,9 @@ runs = 256 depth = 64 fail_on_revert = true +[profile.ci] +# Declaring the parent table preserves default project paths on older Foundry releases. + [profile.ci.fuzz] runs = 4096 diff --git a/slither.config.json b/slither.config.json index 741af8f..394f7c5 100644 --- a/slither.config.json +++ b/slither.config.json @@ -1,5 +1,6 @@ { "filter_paths": "lib|test|script", + "detectors_to_exclude": "arbitrary-send-erc20,timestamp", "exclude_dependencies": true, "solc_remaps": [ "forge-std/=lib/forge-std/src/", diff --git a/test/fuzz/OrderForgeFuzz.t.sol b/test/fuzz/OrderForgeFuzz.t.sol index f32f615..444b4af 100644 --- a/test/fuzz/OrderForgeFuzz.t.sol +++ b/test/fuzz/OrderForgeFuzz.t.sol @@ -86,6 +86,29 @@ contract OrderForgeFuzzTest is Test { assertEq(keccak256(abi.encode(decoded)), keccak256(abi.encode(order))); } + function testFuzz_multiplePartialFillsSettleExactSignedBuyAmount( + uint128 rawSellAmount, + uint128 rawBuyAmount, + uint128 rawFirstFill + ) external { + uint128 sellAmount = uint128(bound(rawSellAmount, 2, type(uint96).max / 2)); + uint128 buyAmount = uint128(bound(rawBuyAmount, sellAmount, type(uint96).max)); + uint128 firstFill = uint128(bound(rawFirstFill, 1, sellAmount - 1)); + + Order memory order = _order(sellAmount, buyAmount, 1002); + bytes memory signature = _sign(order); + + vm.startPrank(taker); + uint256 firstPaid = forge.fillOrder(order, signature, firstFill); + uint256 secondPaid = forge.fillOrder(order, signature, sellAmount - firstFill); + vm.stopPrank(); + + assertEq(firstPaid + secondPaid, buyAmount); + assertEq(forge.filledSellAmount(forge.hashOrder(order)), sellAmount); + assertEq(sellToken.balanceOf(address(forge)), 0); + assertEq(buyToken.balanceOf(address(forge)), 0); + } + function testFuzz_marketIdIsSymmetric(address tokenA, address tokenB, uint24 feeBps) external view { assertEq(codec.marketId(tokenA, tokenB, feeBps), codec.marketId(tokenB, tokenA, feeBps)); } diff --git a/test/invariant/OrderForgeInvariant.t.sol b/test/invariant/OrderForgeInvariant.t.sol index df61ec3..944fffe 100644 --- a/test/invariant/OrderForgeInvariant.t.sol +++ b/test/invariant/OrderForgeInvariant.t.sol @@ -19,6 +19,9 @@ contract SettlementHandler is Test { uint256 public ghostSellFilled; uint256 public ghostBuyPaid; + bool public fillSucceededAfterCancellation; + bool public fillSucceededAfterNonceInvalidation; + bool public fillSucceededAfterCompletion; constructor( OrderForge forge_, @@ -41,14 +44,19 @@ contract SettlementHandler is Test { function fill(uint256 rawAmount) external { bytes32 digest = forge.hashOrder(_order); uint256 filled = forge.filledSellAmount(digest); - if (filled >= _order.sellAmount) return; - if (forge.cancelled(digest) || forge.nonceInvalidated(maker, _order.nonce)) return; + bool wasCompleted = filled == _order.sellAmount; + bool wasCancelled = forge.cancelled(digest); + bool wasNonceInvalidated = forge.nonceInvalidated(maker, _order.nonce); + uint128 amount = wasCompleted ? 1 : uint128(bound(rawAmount, 1, uint256(_order.sellAmount) - filled)); - uint128 amount = uint128(bound(rawAmount, 1, uint256(_order.sellAmount) - filled)); vm.prank(taker); - uint256 buyPaid = forge.fillOrder(_order, _signature, amount); - ghostSellFilled += amount; - ghostBuyPaid += buyPaid; + try forge.fillOrder(_order, _signature, amount) returns (uint256 buyPaid) { + if (wasCompleted) fillSucceededAfterCompletion = true; + if (wasCancelled) fillSucceededAfterCancellation = true; + if (wasNonceInvalidated && !wasCompleted) fillSucceededAfterNonceInvalidation = true; + ghostSellFilled += amount; + ghostBuyPaid += buyPaid; + } catch {} } function cancel() external { @@ -141,4 +149,10 @@ contract OrderForgeInvariantTest is StdInvariant, Test { assertEq(handler.ghostBuyPaid(), order.buyAmount); } } + + function invariant_terminalStatesAlwaysBlockFurtherFills() external view { + assertFalse(handler.fillSucceededAfterCancellation()); + assertFalse(handler.fillSucceededAfterNonceInvalidation()); + assertFalse(handler.fillSucceededAfterCompletion()); + } } diff --git a/test/mocks/MockFalseReturnERC20.sol b/test/mocks/MockFalseReturnERC20.sol new file mode 100644 index 0000000..3811963 --- /dev/null +++ b/test/mocks/MockFalseReturnERC20.sol @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; + +/// @notice ERC-20 test double that signals transfer failure by returning false. +contract MockFalseReturnERC20 is ERC20 { + constructor(string memory name_, string memory symbol_) ERC20(name_, symbol_) {} + + function mint(address to, uint256 amount) external { + _mint(to, amount); + } + + function transferFrom(address, address, uint256) public pure override returns (bool) { + return false; + } +} diff --git a/test/unit/OrderForge.t.sol b/test/unit/OrderForge.t.sol index 310e9a0..ec0b228 100644 --- a/test/unit/OrderForge.t.sol +++ b/test/unit/OrderForge.t.sol @@ -3,12 +3,15 @@ pragma solidity ^0.8.24; import {Test} from "forge-std/Test.sol"; +import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; + import {CodecHarness} from "../../src/CodecHarness.sol"; import {IOrderForge} from "../../src/interfaces/IOrderForge.sol"; import {OrderForge} from "../../src/OrderForge.sol"; import {ExecutionPayload, Order, YieldPosition} from "../../src/types/OrderTypes.sol"; import {Mock1271Wallet} from "../mocks/Mock1271Wallet.sol"; import {MockERC20} from "../mocks/MockERC20.sol"; +import {MockFalseReturnERC20} from "../mocks/MockFalseReturnERC20.sol"; contract OrderForgeTest is Test { uint256 internal constant MAKER_PK = 0xA11CE; @@ -122,6 +125,34 @@ contract OrderForgeTest is Test { forge.fillOrder(order, signature, 10 ether); } + function test_malformedSignatureFailsSafely() external { + Order memory order = _order(100 ether, 200 ether, address(0), 28, keccak256("malformed-sig")); + + vm.expectRevert(OrderForge.InvalidSignature.selector); + vm.prank(taker); + forge.fillOrder(order, hex"deadbeef", 10 ether); + } + + function test_mutatingSignedOrderInvalidatesSignature() external { + Order memory order = _order(100 ether, 200 ether, address(0), 29, keccak256("mutated-order")); + bytes memory signature = _sign(order, MAKER_PK); + order.buyAmount += 1; + + vm.expectRevert(OrderForge.InvalidSignature.selector); + vm.prank(taker); + forge.fillOrder(order, signature, 10 ether); + } + + function test_signatureCannotReplayAcrossChainIds() external { + Order memory order = _order(100 ether, 200 ether, address(0), 30, keccak256("chain-domain")); + bytes memory signature = _sign(order, MAKER_PK); + vm.chainId(block.chainid + 1); + + vm.expectRevert(OrderForge.InvalidSignature.selector); + vm.prank(taker); + forge.fillOrder(order, signature, 10 ether); + } + function test_makerCanCancelRemainingOrder() external { Order memory order = _order(100 ether, 200 ether, address(0), 6, keccak256("cancel")); bytes memory signature = _sign(order, MAKER_PK); @@ -193,6 +224,61 @@ contract OrderForgeTest is Test { assertEq(buyToken.balanceOf(address(wallet)), 200 ether); } + function test_erc1271WalletRejectsSignatureFromNonOwner() external { + address walletOwner = vm.addr(0xD00D); + Mock1271Wallet wallet = new Mock1271Wallet(walletOwner); + + Order memory order = _order(100 ether, 200 ether, address(0), 31, keccak256("1271-invalid")); + order.maker = address(wallet); + bytes memory signature = _sign(order, OTHER_PK); + + vm.expectRevert(OrderForge.InvalidSignature.selector); + vm.prank(taker); + forge.fillOrder(order, signature, 100 ether); + } + + function test_falseReturningBuyTokenRevertsAndRollsBackFillState() external { + MockFalseReturnERC20 falseToken = new MockFalseReturnERC20("False", "FALSE"); + falseToken.mint(taker, 200 ether); + vm.prank(taker); + falseToken.approve(address(forge), type(uint256).max); + + Order memory order = _order(100 ether, 200 ether, address(0), 32, keccak256("false-buy")); + order.buyToken = address(falseToken); + bytes memory signature = _sign(order, MAKER_PK); + bytes32 digest = forge.hashOrder(order); + + vm.expectRevert(abi.encodeWithSelector(SafeERC20.SafeERC20FailedOperation.selector, address(falseToken))); + vm.prank(taker); + forge.fillOrder(order, signature, order.sellAmount); + + assertEq(forge.filledSellAmount(digest), 0); + assertFalse(forge.nonceInvalidated(maker, order.nonce)); + assertEq(forge.boundOrderHash(maker, order.nonce), bytes32(0)); + } + + function test_falseReturningSellTokenRevertsBothTransfersAtomically() external { + MockFalseReturnERC20 falseToken = new MockFalseReturnERC20("False", "FALSE"); + falseToken.mint(maker, 100 ether); + vm.prank(maker); + falseToken.approve(address(forge), type(uint256).max); + + Order memory order = _order(100 ether, 200 ether, address(0), 33, keccak256("false-sell")); + order.sellToken = address(falseToken); + bytes memory signature = _sign(order, MAKER_PK); + uint256 makerBuyBefore = buyToken.balanceOf(maker); + uint256 takerBuyBefore = buyToken.balanceOf(taker); + + vm.expectRevert(abi.encodeWithSelector(SafeERC20.SafeERC20FailedOperation.selector, address(falseToken))); + vm.prank(taker); + forge.fillOrder(order, signature, order.sellAmount); + + assertEq(buyToken.balanceOf(maker), makerBuyBefore); + assertEq(buyToken.balanceOf(taker), takerBuyBefore); + assertEq(forge.filledSellAmount(forge.hashOrder(order)), 0); + assertFalse(forge.nonceInvalidated(maker, order.nonce)); + } + function test_standardAbiOrderRoundTrip() external view { Order memory original = _order(123, 456, taker, 11, keccak256("codec")); bytes memory encoded = codec.encodeOrder(original);