Add TWAP price-oracle and cross-function reentrancy check classes

.github/workflows/audit.yml

@@ -31,6 +31,8 @@ jobs:
         #   - ExampleReentrancyAudit, ExampleAccessControlAudit (Example.t.sol)
         #   - ExampleGovernanceAudit                            (GovernanceExample.t.sol)
         #   - TestERC4626AdvancedCheck                          (ERC4626AdvancedCheck.t.sol)
+        #   - ExampleTWAPOracleManipulationAudit                (TWAPOracleCheck.t.sol)
+        #   - ExampleCrossFunctionReentrancyAudit               (CrossFunctionReentrancyCheck.t.sol)
         # See tracking issue for packaging these as forge scripts / a separate
         # `forge test --match-path test/demo/*` suite.
         run: forge test -vvv --no-match-contract '^(Example|TestERC4626)'

README.md

@@ -189,10 +189,12 @@ or open [`web/index.html`](web/index.html) directly.
 
 | Check | What it detects |
 |-------|----------------|
-| `ReentrancyCheck` | Checks-effects-interactions violations, cross-function reentrancy via callbacks |
+| `ReentrancyCheck` | Single-function checks-effects-interactions violations via the withdraw callback |
+| `CrossFunctionReentrancyCheck` | Cross-function reentrancy — two functions share state, one calls out before settling, the attacker re-enters through the other (Lendf.Me / dForce class, 2020) |
 | `ERC777ReentrancyCheck` | Reentrancy via `tokensReceived` recipient hook (imBTC / Uniswap V1 class, 2020) |
 | `AccessControlCheck` | Unprotected admin functions, unguarded initializers, missing role checks |
 | `OracleCheck` | Spot price reads (manipulable), missing TWAP, single-source oracles |
+| `TWAPOracleCheck` | Price oracle manipulable within a single block — verifies the acted-on price resists single-block source manipulation (TWAP passes, spot fails); bZx / Harvest / Inverse Finance class |
 | `UpgradeCheck` | Storage layout collisions in proxies, uninitialized implementation contracts |
 | `FlashLoanCheck` | Functions vulnerable to flash-loan-powered price/state manipulation |
 | `UncheckedLowLevelCallCheck` | Discarded `address.call()` success booleans that silently trap funds or hide failures |
@@ -203,16 +205,22 @@ or open [`web/index.html`](web/index.html) directly.
 src/
 ├── ChecklistBase.sol        — Base contract with shared test helpers
 ├── checks/
-│   ├── ReentrancyCheck.sol  — Reentrancy detection tests
+│   ├── ReentrancyCheck.sol  — Single-function reentrancy detection tests
+│   ├── CrossFunctionReentrancyCheck.sol — Cross-function reentrancy (shared state, two entry points)
 │   ├── ERC777ReentrancyCheck.sol — ERC-777 tokensReceived reentrancy
 │   ├── AccessControlCheck.sol — Access control verification
 │   ├── OracleCheck.sol      — Oracle manipulation checks
+│   ├── TWAPOracleCheck.sol  — Single-block price-oracle manipulation (TWAP vs spot)
 │   ├── UpgradeCheck.sol     — Proxy upgrade safety
 │   └── FlashLoanCheck.sol   — Flash loan resistance
 ├── examples/
-│   └── VulnerableVault.sol  — Intentionally vulnerable demo contract
+│   ├── VulnerableVault.sol  — Intentionally vulnerable demo contract
+│   ├── TWAPOracleExamples.sol — Spot-price (vulnerable) vs TWAP (safe) oracle consumers
+│   └── CrossFunctionReentrancyExamples.sol — Unguarded vs shared-lock vaults
 test/
-└── Example.t.sol            — Full example audit against VulnerableVault
+├── Example.t.sol            — Full example audit against VulnerableVault
+├── TWAPOracleCheck.t.sol    — TWAPOracleCheck vs safe/vulnerable oracle consumers
+└── CrossFunctionReentrancyCheck.t.sol — CrossFunctionReentrancyCheck vs safe/vulnerable vaults
 ```
 
 ## License

src/checks/CrossFunctionReentrancyCheck.sol

@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+import "../ChecklistBase.sol";
+
+/// @title CrossFunctionReentrancyCheck — detect cross-function reentrancy
+/// @notice Classic ReentrancyCheck re-enters the SAME function. This check
+///         models *cross-function* reentrancy: function A (a withdraw/payout)
+///         makes an external call before settling shared state, and the
+///         attacker re-enters through a DIFFERENT function B that mutates the
+///         same state while it is still stale.
+///
+///         The tell-tale is that a single deposit becomes withdrawable twice:
+///         during withdraw()'s callback the attacker moves its still-credited
+///         internal balance to an accomplice, so the protocol pays both the
+///         attacker (now) and the accomplice (later). A `nonReentrant` guard
+///         that does not span BOTH entry points fails to stop this — which is
+///         why it deserves its own check.
+///
+///         Override:
+///           - `getWithdrawCalldata()`     — the payout function (A)
+///           - `getReentrantCalldata(addr)` — the OTHER function (B) the
+///             attacker calls mid-callback to move credit to `accomplice`
+///           - `performDeposit(addr,amount)` — seed a balance
+///           - `balanceOfTarget(addr)`      — read the protocol's internal
+///             credit for an account, to detect surviving moved credit
+/// @author kcolbchain
+abstract contract CrossFunctionReentrancyCheck is ChecklistBase {
+    /// @dev Calldata that triggers the payout under test (function A).
+    function getWithdrawCalldata() internal view virtual returns (bytes memory);
+
+    /// @dev Calldata for the OTHER state-sharing function (function B) the
+    ///      attacker re-enters during A's callback. `accomplice` is where the
+    ///      attacker tries to park its still-credited balance.
+    function getReentrantCalldata(address accomplice) internal view virtual returns (bytes memory);
+
+    /// @dev Seed `amount` of withdrawable balance for `depositor`.
+    function performDeposit(address depositor, uint256 amount) internal virtual;
+
+    /// @dev Read the protocol's internal credit for `account` (e.g. the
+    ///      `balances` mapping). Used to detect credit that survived the
+    ///      cross-function move.
+    function balanceOfTarget(address account) internal view virtual returns (uint256);
+
+    /// @dev Deposit amount for the test.
+    function getDepositValue() internal view virtual returns (uint256) {
+        return 1 ether;
+    }
+
+    /// @notice Forge-discovered audit check. Runs the detector and calls
+    ///         `fail()` when cross-function reentrancy succeeds — a failing test
+    ///         means a caught bug, the same convention as every other check here.
+    function test_cross_function_reentrancy() public {
+        if (detectCrossFunctionReentrancy()) {
+            fail();
+        }
+    }
+
+    /// @notice Side-effect-free detector (does not call `fail()`). Returns true
+    ///         if credit moved to an accomplice mid-withdraw survives state
+    ///         settlement — the double-spend signature of cross-function
+    ///         reentrancy. Lets assertion-based harnesses prove detection
+    ///         without tripping the global failure flag.
+    function detectCrossFunctionReentrancy() public returns (bool) {
+        uint256 amount = getDepositValue();
+        address accomplice = makeAddr("xfn_accomplice");
+
+        CrossFunctionReentrantAttacker attacker = new CrossFunctionReentrantAttacker(
+            targetContract,
+            getWithdrawCalldata(),
+            getReentrantCalldata(accomplice)
+        );
+
+        // Fund the protocol so a successful double-spend has ETH to pay out.
+        vm.deal(targetContract, amount * 2);
+
+        // Attacker deposits exactly `amount` of withdrawable credit.
+        vm.deal(address(attacker), amount);
+        performDeposit(address(attacker), amount);
+
+        // Trigger the payout. The attacker's receive() re-enters the OTHER
+        // function to move its credit to the accomplice before withdraw()
+        // zeroes it.
+        attacker.attack();
+
+        // Cross-function reentrancy succeeded if the accomplice still holds
+        // credit that was conjured from the attacker's single deposit — i.e.
+        // the protocol now owes more than was ever deposited.
+        uint256 survivingCredit = balanceOfTarget(accomplice);
+
+        if (survivingCredit > 0) {
+            emit log(unicode"VULNERABILITY: cross-function reentrancy — credit moved to an accomplice mid-withdraw survived state settlement");
+            emit log_named_uint("Deposited", amount);
+            emit log_named_uint("Credit parked on accomplice (double-spend)", survivingCredit);
+            return true;
+        }
+        return false;
+    }
+}
+
+/// @dev Attacker that re-enters a DIFFERENT function on the payout callback.
+contract CrossFunctionReentrantAttacker {
+    address public target;
+    bytes public withdrawPayload;
+    bytes public reentrantPayload;
+    bool private entered;
+
+    constructor(address _target, bytes memory _withdraw, bytes memory _reentrant) {
+        target = _target;
+        withdrawPayload = _withdraw;
+        reentrantPayload = _reentrant;
+    }
+
+    function attack() external {
+        (bool ok,) = target.call(withdrawPayload);
+        ok; // detection is via surviving credit, not call success
+    }
+
+    receive() external payable {
+        if (!entered) {
+            entered = true;
+            // Cross-function re-entry: call the OTHER function while the
+            // withdraw that sent us this ETH has not yet zeroed our balance.
+            (bool ok,) = target.call(reentrantPayload);
+            ok;
+        }
+    }
+}

src/checks/TWAPOracleCheck.sol

@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+import "../ChecklistBase.sol";
+
+/// @title TWAPOracleCheck — detect price oracles manipulable within one block
+/// @notice Stronger sibling of OracleCheck. OracleCheck simply diffs a price
+///         before/after a manipulation. This check models the *defense*: it
+///         manipulates the underlying source within a single block (no time
+///         advance) and asserts the price the contract under audit actually
+///         acts on does NOT move beyond a tolerance.
+///
+///         A spot-price reader moves immediately and fails. A TWAP
+///         (time-weighted average price) reader barely moves inside one block —
+///         its accumulator only advances with elapsed time — and passes.
+///
+///         This is the bug class behind bZx (2020), Harvest, Cheese Bank,
+///         Inverse Finance, and many other oracle-manipulation drains: a single
+///         instantaneous DEX read used for solvency-critical decisions.
+///
+///         Override the hooks to point this check at your protocol:
+///         `getPriceReadCalldata`, `manipulateSource`, `refreshOracle`.
+/// @author kcolbchain
+abstract contract TWAPOracleCheck is ChecklistBase {
+    /// @dev Calldata for the price read the protocol acts on (e.g. getPrice()).
+    ///      Must return a single uint256.
+    function getPriceReadCalldata() internal view virtual returns (bytes memory);
+
+    /// @dev Manipulate the underlying price source within the current block
+    ///      (e.g. swap an imbalanced amount into the DEX pair). Must NOT advance
+    ///      time — single-block manipulation is the threat being modelled.
+    function manipulateSource() internal virtual;
+
+    /// @dev Refresh/poke the oracle the way an attacker would be forced to in
+    ///      the same transaction (e.g. call the consumer's `update()`), then
+    ///      read again. A TWAP consumer's value still won't have moved because
+    ///      no time elapsed. Default: no-op (pure spot readers need nothing).
+    function refreshOracle() internal virtual {}
+
+    /// @dev Maximum tolerated single-block price drift, in percent. A robust
+    ///      oracle should move well under this even when the raw source is
+    ///      slammed. Default 10%. Override to tighten/loosen.
+    function getMaxDriftPercent() internal view virtual returns (uint256) {
+        return 10;
+    }
+
+    function _readPrice() private view returns (uint256) {
+        (bool ok, bytes memory data) = targetContract.staticcall(getPriceReadCalldata());
+        require(ok, "TWAPOracleCheck: price read reverted");
+        return abi.decode(data, (uint256));
+    }
+
+    /// @notice Forge-discovered audit check. Runs the detector and calls
+    ///         `fail()` when the oracle is manipulable — a failing test means a
+    ///         caught bug, the same convention as every other check here.
+    function test_oracle_resists_single_block_manipulation() public {
+        if (detectSingleBlockManipulation()) {
+            fail();
+        }
+    }
+
+    /// @notice Side-effect-free detector. Returns true if the price the
+    ///         contract acts on drifts past the tolerance under single-block
+    ///         manipulation. Useful for assertion-based harnesses that want to
+    ///         prove detection without tripping the global failure flag.
+    function detectSingleBlockManipulation() public returns (bool) {
+        uint256 priceBefore = _readPrice();
+
+        // Slam the underlying source within this single block (no vm.warp).
+        manipulateSource();
+        // Force the consumer to refresh exactly as it would mid-attack.
+        refreshOracle();
+
+        uint256 priceAfter = _readPrice();
+
+        if (priceBefore == 0) {
+            // Cannot reason about drift from a zero baseline; treat as inert.
+            return false;
+        }
+
+        uint256 delta = priceBefore > priceAfter
+            ? priceBefore - priceAfter
+            : priceAfter - priceBefore;
+        uint256 pctChange = (delta * 100) / priceBefore;
+
+        if (pctChange > getMaxDriftPercent()) {
+            emit log(unicode"VULNERABILITY: price oracle is manipulable within a single block (spot read, no TWAP)");
+            emit log_named_uint("Price before", priceBefore);
+            emit log_named_uint("Price after (same block)", priceAfter);
+            emit log_named_uint("Drift %", pctChange);
+            emit log_named_uint("Max tolerated %", getMaxDriftPercent());
+            return true;
+        }
+        return false;
+    }
+}

src/examples/CrossFunctionReentrancyExamples.sol

@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+/// @title CrossFunctionReentrancyExamples — fixtures for CrossFunctionReentrancyCheck
+/// @notice DO NOT USE IN PRODUCTION. Demonstrates *cross-function* reentrancy:
+///         two functions share state, one makes an external call before
+///         settling that state, and the attacker re-enters through the OTHER
+///         function during the callback to act on the still-stale balance.
+///
+///         This differs from classic single-function reentrancy (where the
+///         attacker re-enters the SAME function). A `nonReentrant` guard placed
+///         on only the withdraw path — or naive CEI within one function — does
+///         not stop it; the second entry point must share the lock. This is the
+///         pattern behind several real drains (e.g. the Lendf.Me / dForce
+///         imBTC drain, 2020, and numerous staking/AMM bugs).
+/// @author kcolbchain
+
+/// @dev Minimal OpenZeppelin-style reentrancy guard so the safe variant is
+///      self-contained (no import wiring needed for the fixture).
+abstract contract SimpleReentrancyGuard {
+    uint256 private constant _NOT_ENTERED = 1;
+    uint256 private constant _ENTERED = 2;
+    uint256 private _status = _NOT_ENTERED;
+
+    modifier nonReentrant() {
+        require(_status != _ENTERED, "ReentrancyGuard: reentrant call");
+        _status = _ENTERED;
+        _;
+        _status = _NOT_ENTERED;
+    }
+}
+
+/// @notice VULNERABLE. `withdraw()` sends ETH (external call) before zeroing
+///         `balances[msg.sender]`. The attacker's `receive()` re-enters via the
+///         separate `transfer()` function and moves its still-nonzero internal
+///         balance to an accomplice account. After `withdraw()` finally zeroes
+///         the attacker's slot, the accomplice can withdraw the transferred
+///         balance too — so the protocol pays out twice for one deposit.
+contract VulnerableCrossFunctionVault {
+    mapping(address => uint256) public balances;
+
+    function deposit() external payable {
+        balances[msg.sender] += msg.value;
+    }
+
+    /// @dev Internal transfer of credit between accounts. Shares `balances`
+    ///      with withdraw() but has NO reentrancy protection and can be called
+    ///      mid-withdraw.
+    function transfer(address to, uint256 amount) external {
+        require(balances[msg.sender] >= amount, "insufficient");
+        balances[msg.sender] -= amount;
+        balances[to] += amount;
+    }
+
+    /// @dev BUG: external call before the state write. While the call is in
+    ///      flight `balances[msg.sender]` is still funded, so a re-entrant
+    ///      `transfer()` can siphon that credit to an accomplice. The later
+    ///      `balances[msg.sender] = 0` zeroes an already-emptied slot — the
+    ///      moved credit survives.
+    function withdraw() external {
+        uint256 bal = balances[msg.sender];
+        require(bal > 0, "no balance");
+
+        (bool sent,) = msg.sender.call{value: bal}("");
+        require(sent, "transfer failed");
+
+        balances[msg.sender] = 0; // Too late — credit already moved cross-function.
+    }
+}
+
+/// @notice SAFE. Same two entry points, but a shared `nonReentrant` guard
+///         spans BOTH `withdraw()` and `transfer()`, so the cross-function
+///         re-entry reverts. (CEI alone would also fix withdraw(), but the
+///         shared lock is what defeats the cross-function vector generally.)
+contract SafeCrossFunctionVault is SimpleReentrancyGuard {
+    mapping(address => uint256) public balances;
+
+    function deposit() external payable {
+        balances[msg.sender] += msg.value;
+    }
+
+    function transfer(address to, uint256 amount) external nonReentrant {
+        require(balances[msg.sender] >= amount, "insufficient");
+        balances[msg.sender] -= amount;
+        balances[to] += amount;
+    }
+
+    function withdraw() external nonReentrant {
+        uint256 bal = balances[msg.sender];
+        require(bal > 0, "no balance");
+
+        // Checks-effects-interactions AND a shared lock — belt and suspenders.
+        balances[msg.sender] = 0;
+
+        (bool sent,) = msg.sender.call{value: bal}("");
+        require(sent, "transfer failed");
+    }
+}