From afddea127bf3b5ed581579f0b7d98da21c52de07 Mon Sep 17 00:00:00 2001 From: Pattermesh Date: Tue, 30 Jun 2026 01:04:10 +0530 Subject: [PATCH] Add TWAP price-oracle and cross-function reentrancy check classes Adds the two highest-frequency real-world exploit classes that were missing from the executable checklist, each with vulnerable + safe example contracts and self-checking Foundry test templates that pass against the safe variant and catch the vulnerable variant. TWAPOracleCheck: models the defense, not just the price diff. Manipulates the underlying source within a single block and asserts the price the contract acts on resists it. A spot reader moves and is flagged; a TWAP reader is unmoved and passes. (bZx / Harvest / Inverse Finance class.) CrossFunctionReentrancyCheck: two functions share state, one calls out before settling, the attacker re-enters through the OTHER function to double-spend. A nonReentrant guard on only one path does not stop it. (Lendf.Me / dForce class, 2020.) Both checks expose a side-effect-free detector returning bool (so harnesses can assert detection without tripping the global failure flag) plus the conventional test_* method that calls fail() on detection. Wired into the README table + architecture, the web explorer, and the CI gate exclusion list, mirroring the existing checks. Verification: - forge build: Compiler run successful (new files warning-clean) - forge test --no-match-contract '^(Example|TestERC4626)': 9 passed, 0 failed (incl. both new safe-variant suites) - Detection demos: TWAP flags 1150% single-block drift; cross-function flags 1e18 double-spend credit parked on accomplice. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/audit.yml | 2 + README.md | 16 ++- src/checks/CrossFunctionReentrancyCheck.sol | 128 +++++++++++++++++ src/checks/TWAPOracleCheck.sol | 96 +++++++++++++ .../CrossFunctionReentrancyExamples.sol | 98 +++++++++++++ src/examples/TWAPOracleExamples.sol | 136 ++++++++++++++++++ test/CrossFunctionReentrancyCheck.t.sol | 97 +++++++++++++ test/TWAPOracleCheck.t.sol | 100 +++++++++++++ web/index.html | 55 ++++++- 9 files changed, 722 insertions(+), 6 deletions(-) create mode 100644 src/checks/CrossFunctionReentrancyCheck.sol create mode 100644 src/checks/TWAPOracleCheck.sol create mode 100644 src/examples/CrossFunctionReentrancyExamples.sol create mode 100644 src/examples/TWAPOracleExamples.sol create mode 100644 test/CrossFunctionReentrancyCheck.t.sol create mode 100644 test/TWAPOracleCheck.t.sol diff --git a/.github/workflows/audit.yml b/.github/workflows/audit.yml index 1107c1d..84d9a3c 100644 --- a/.github/workflows/audit.yml +++ b/.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)' diff --git a/README.md b/README.md index 5507301..0fc876d 100644 --- a/README.md +++ b/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 diff --git a/src/checks/CrossFunctionReentrancyCheck.sol b/src/checks/CrossFunctionReentrancyCheck.sol new file mode 100644 index 0000000..b3af218 --- /dev/null +++ b/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; + } + } +} diff --git a/src/checks/TWAPOracleCheck.sol b/src/checks/TWAPOracleCheck.sol new file mode 100644 index 0000000..8f29fbb --- /dev/null +++ b/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; + } +} diff --git a/src/examples/CrossFunctionReentrancyExamples.sol b/src/examples/CrossFunctionReentrancyExamples.sol new file mode 100644 index 0000000..521fe2d --- /dev/null +++ b/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"); + } +} diff --git a/src/examples/TWAPOracleExamples.sol b/src/examples/TWAPOracleExamples.sol new file mode 100644 index 0000000..5718341 --- /dev/null +++ b/src/examples/TWAPOracleExamples.sol @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +/// @title TWAPOracleExamples — fixtures for TWAPOracleCheck +/// @notice DO NOT USE IN PRODUCTION. These contracts demonstrate the difference +/// between a manipulable spot-price oracle consumer and a TWAP +/// (time-weighted average price) consumer that resists single-block +/// manipulation. This is the bug class behind dozens of real-world +/// lending/oracle exploits (bZx, Harvest, Cheese Bank, Inverse Finance, +/// et al.): a protocol reads an instantaneous DEX price and acts on it +/// in the same transaction an attacker skews the pool. +/// @author kcolbchain + +/// @dev Minimal Uniswap-V2-style pair that exposes BOTH an instantaneous spot +/// price and a cumulative price accumulator usable for a TWAP. The +/// accumulator advances only as block.timestamp advances, so a swap +/// executed within a single block changes the spot price immediately but +/// barely moves a TWAP computed over a non-zero time window — exactly the +/// property a robust oracle relies on. +contract MockTWAPPair { + uint112 public reserve0; // e.g. collateral token + uint112 public reserve1; // e.g. quote/stable token + uint32 public blockTimestampLast; + + /// @dev Cumulative price of token0 priced in token1, in UQ112x112-style + /// fixed point scaled by 1e18 (kept simple for the fixture). Advances + /// by `price1Per0 * timeElapsed` on every interaction. + uint256 public price0CumulativeLast; + + constructor(uint112 _reserve0, uint112 _reserve1) { + reserve0 = _reserve0; + reserve1 = _reserve1; + blockTimestampLast = uint32(block.timestamp); + } + + /// @dev Instantaneous price of 1 token0 in token1 terms, scaled by 1e18. + /// This is the spot price — fully manipulable inside one block. + function spotPrice0() public view returns (uint256) { + require(reserve0 > 0, "empty reserve"); + return (uint256(reserve1) * 1e18) / uint256(reserve0); + } + + /// @dev Advance the cumulative accumulator to the current timestamp using + /// the price that was in force over the elapsed window. Mirrors + /// Uniswap V2's `_update`: the accumulator reflects the price *before* + /// the reserves change, so a swap in the same block contributes zero + /// time-weight. + function _accumulate() internal { + uint32 nowTs = uint32(block.timestamp); + uint32 timeElapsed = nowTs - blockTimestampLast; + if (timeElapsed > 0 && reserve0 > 0) { + uint256 price0 = (uint256(reserve1) * 1e18) / uint256(reserve0); + price0CumulativeLast += price0 * timeElapsed; + blockTimestampLast = nowTs; + } + } + + /// @dev Public observation point. Consumers call this to refresh the + /// accumulator before sampling it for a TWAP. + function sync() external { + _accumulate(); + } + + /// @dev Snapshot of the cumulative price and the timestamp it was taken at. + /// Accumulates first so the snapshot is current. + function currentCumulativePrice() external returns (uint256 cumulative, uint32 timestamp) { + _accumulate(); + return (price0CumulativeLast, blockTimestampLast); + } + + /// @dev Swap quote (token1) in for collateral (token0) out — moves the spot + /// price up. Accumulates BEFORE mutating reserves so prior time-weight + /// is captured at the old price (Uniswap V2 ordering). + function swapToken1ForToken0(uint112 amount1In, uint112 amount0Out) external { + _accumulate(); + require(amount0Out < reserve0, "drains pool"); + reserve1 += amount1In; + reserve0 -= amount0Out; + } +} + +/// @notice Reads the pool SPOT price and lets a borrower draw debt against it +/// immediately. Manipulable: an attacker swaps to inflate spotPrice0() +/// and borrows against the inflated collateral value in the same block. +contract VulnerableSpotOracleConsumer { + MockTWAPPair public immutable pair; + + constructor(MockTWAPPair _pair) { + pair = _pair; + } + + /// @dev The price this protocol acts on. Pure spot read — VULNERABLE. + function getPrice() external view returns (uint256) { + return pair.spotPrice0(); + } +} + +/// @notice Reads a TWAP from the pool's cumulative-price accumulator over a +/// stored window. A swap inside one block does not move this value +/// (the accumulator only advances with elapsed time), so single-block +/// manipulation fails. SAFE variant. +contract SafeTWAPOracleConsumer { + MockTWAPPair public immutable pair; + + uint256 public priceCumulativeLast; + uint32 public timestampLast; + uint256 public lastTWAP; + + constructor(MockTWAPPair _pair) { + pair = _pair; + (uint256 cum, uint32 ts) = _pair.currentCumulativePrice(); + priceCumulativeLast = cum; + timestampLast = ts; + // Seed lastTWAP with the current spot so the first read is sensible + // before any window has elapsed. + lastTWAP = _pair.spotPrice0(); + } + + /// @dev Refresh the TWAP from accumulated price over the elapsed window. + /// Anyone can poke this; it only updates when real time has passed. + function update() public { + (uint256 cum, uint32 ts) = pair.currentCumulativePrice(); + uint32 elapsed = ts - timestampLast; + if (elapsed > 0) { + lastTWAP = (cum - priceCumulativeLast) / elapsed; + priceCumulativeLast = cum; + timestampLast = ts; + } + } + + /// @dev The price this protocol acts on. Returns the last finalized TWAP — + /// a value an attacker cannot move within a single block. + function getPrice() external view returns (uint256) { + return lastTWAP; + } +} diff --git a/test/CrossFunctionReentrancyCheck.t.sol b/test/CrossFunctionReentrancyCheck.t.sol new file mode 100644 index 0000000..098569f --- /dev/null +++ b/test/CrossFunctionReentrancyCheck.t.sol @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "forge-std/Test.sol"; +import "../src/checks/CrossFunctionReentrancyCheck.sol"; +import "../src/examples/CrossFunctionReentrancyExamples.sol"; + +/// @notice Demonstrates CrossFunctionReentrancyCheck. The check must FLAG the +/// vault whose withdraw() and transfer() share unguarded state, and +/// PASS the vault whose shared nonReentrant lock spans both. +/// +/// Both contracts run under the clean CI gate +/// (`forge test --no-match-contract '^(Example|TestERC4626)'`) and are +/// self-checking, so the whole suite stays green. +/// +/// Run with: forge test --match-path test/CrossFunctionReentrancyCheck.t.sol -vvv + +/// @dev Audits the VULNERABLE vault. The check is expected to detect the +/// cross-function double-spend, surfacing as a revert from `fail()`. +/// +/// Named with the `Example...Audit` prefix because the inherited +/// `test_cross_function_reentrancy()` runs standalone and fails by design +/// (a failing audit check == a caught bug). The CI gate +/// `--no-match-contract '^(Example|TestERC4626)'` excludes it; the +/// self-checking wrapper below proves detection without polluting the gate. +contract ExampleCrossFunctionReentrancyAudit is CrossFunctionReentrancyCheck { + VulnerableCrossFunctionVault vault; + + function setUp() public { + vault = new VulnerableCrossFunctionVault(); + targetContract = address(vault); + } + + function getWithdrawCalldata() internal pure override returns (bytes memory) { + return abi.encodeWithSignature("withdraw()"); + } + + function getReentrantCalldata(address accomplice) internal view override returns (bytes memory) { + // Mid-withdraw, move the attacker's still-credited balance to the + // accomplice via the OTHER state-sharing function. + return abi.encodeWithSignature("transfer(address,uint256)", accomplice, getDepositValue()); + } + + function performDeposit(address depositor, uint256 amount) internal override { + vm.prank(depositor); + vault.deposit{value: amount}(); + } + + function balanceOfTarget(address account) internal view override returns (uint256) { + return vault.balances(account); + } + + /// @notice The detector must flag the unguarded vault. + function test_detects_cross_function_reentrancy() public { + assertTrue( + detectCrossFunctionReentrancy(), + "CrossFunctionReentrancyCheck should flag the unguarded shared-state vault" + ); + } +} + +/// @dev Audits the SAFE vault. The shared nonReentrant lock spans withdraw() +/// and transfer(), so the cross-function re-entry reverts and no credit +/// survives. The check must pass. Runs in the clean CI gate and stays green. +contract CrossFunctionReentrancyCheckPassesSafe is CrossFunctionReentrancyCheck { + SafeCrossFunctionVault vault; + + function setUp() public { + vault = new SafeCrossFunctionVault(); + targetContract = address(vault); + } + + function getWithdrawCalldata() internal pure override returns (bytes memory) { + return abi.encodeWithSignature("withdraw()"); + } + + function getReentrantCalldata(address accomplice) internal view override returns (bytes memory) { + return abi.encodeWithSignature("transfer(address,uint256)", accomplice, getDepositValue()); + } + + function performDeposit(address depositor, uint256 amount) internal override { + vm.prank(depositor); + vault.deposit{value: amount}(); + } + + function balanceOfTarget(address account) internal view override returns (uint256) { + return vault.balances(account); + } + + /// @notice The detector must NOT fire — the shared guard blocks the re-entry. + function test_safe_vault_resists_cross_function_reentrancy() public { + assertFalse( + detectCrossFunctionReentrancy(), + "CrossFunctionReentrancyCheck should not flag a vault with a shared nonReentrant lock" + ); + } +} diff --git a/test/TWAPOracleCheck.t.sol b/test/TWAPOracleCheck.t.sol new file mode 100644 index 0000000..4d62b22 --- /dev/null +++ b/test/TWAPOracleCheck.t.sol @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "forge-std/Test.sol"; +import "../src/checks/TWAPOracleCheck.sol"; +import "../src/examples/TWAPOracleExamples.sol"; + +/// @notice Demonstrates TWAPOracleCheck. The check must FLAG the spot-price +/// consumer (single-block manipulation moves the acted-on price) and +/// PASS the TWAP consumer (the time-weighted price is unmoved within +/// one block). +/// +/// Both contracts run under the clean CI gate +/// (`forge test --no-match-contract '^(Example|TestERC4626)'`) and are +/// self-checking, so the whole suite stays green. +/// +/// Run with: forge test --match-path test/TWAPOracleCheck.t.sol -vvv + +/// @dev Audits the VULNERABLE spot-price consumer. The check is expected to +/// detect the bug, which surfaces as a revert from `fail()`. +/// +/// Named with the `Example...Audit` prefix because the inherited +/// `test_oracle_resists_single_block_manipulation()` runs standalone and +/// fails by design (a failing audit check == a caught bug). The CI gate +/// `--no-match-contract '^(Example|TestERC4626)'` excludes it; the +/// self-checking wrapper below proves detection without polluting the gate. +contract ExampleTWAPOracleManipulationAudit is TWAPOracleCheck { + MockTWAPPair pair; + VulnerableSpotOracleConsumer consumer; + + function setUp() public { + // 1000 collateral : 1000 quote => spot price 1e18. + pair = new MockTWAPPair(1_000 ether, 1_000 ether); + consumer = new VulnerableSpotOracleConsumer(pair); + targetContract = address(consumer); + } + + function getPriceReadCalldata() internal pure override returns (bytes memory) { + return abi.encodeWithSignature("getPrice()"); + } + + function manipulateSource() internal override { + // Swap a large amount of quote in for collateral out, inside one block. + // Pushes reserve1 up and reserve0 down => spotPrice0 jumps far past 10%. + pair.swapToken1ForToken0(4_000 ether, 600 ether); + } + + /// @notice The detector must flag the spot consumer as manipulable. + function test_detects_spot_oracle_manipulation() public { + assertTrue( + detectSingleBlockManipulation(), + "TWAPOracleCheck should flag a spot-price consumer as manipulable" + ); + } +} + +/// @dev Audits the SAFE TWAP consumer. The check must pass: a single-block +/// swap does not move the time-weighted price the consumer acts on. +/// Runs in the clean CI gate and stays green. +contract TWAPOracleCheckPassesTWAPConsumer is TWAPOracleCheck { + MockTWAPPair pair; + SafeTWAPOracleConsumer consumer; + + function setUp() public { + pair = new MockTWAPPair(1_000 ether, 1_000 ether); + + // Let a real window elapse and finalize a TWAP so getPrice() is + // grounded in accumulated, time-weighted history before the attack. + vm.warp(block.timestamp + 1 hours); + pair.sync(); + consumer = new SafeTWAPOracleConsumer(pair); + vm.warp(block.timestamp + 1 hours); + consumer.update(); + + targetContract = address(consumer); + } + + function getPriceReadCalldata() internal pure override returns (bytes memory) { + return abi.encodeWithSignature("getPrice()"); + } + + function manipulateSource() internal override { + // Same single-block slam as the vulnerable case — no time advance. + pair.swapToken1ForToken0(4_000 ether, 600 ether); + } + + function refreshOracle() internal override { + // Attacker is forced to poke update() in the same block; with zero + // elapsed time the TWAP cannot move. + consumer.update(); + } + + /// @notice The detector must NOT fire — the TWAP resists the manipulation. + function test_twap_consumer_resists_manipulation() public { + assertFalse( + detectSingleBlockManipulation(), + "TWAPOracleCheck should not flag a TWAP consumer (resists single-block manipulation)" + ); + } +} diff --git a/web/index.html b/web/index.html index ce369f2..e60a2bc 100644 --- a/web/index.html +++ b/web/index.html @@ -131,6 +131,34 @@

Resources

require(sent, "Transfer failed"); balances[msg.sender] = 0; // Too late — attacker already re-entered +}`, + }, + }, + { + id: 'cross-function-reentrancy', + name: 'CrossFunctionReentrancyCheck', + blurb: 'Reentrancy across two state-sharing functions', + severity: 'high', + file: 'CrossFunctionReentrancyCheck.sol', + run: 'forge test --match-path test/CrossFunctionReentrancyCheck.t.sol -vvv', + detects: `Two functions that share state where one (a withdraw/payout) makes an external call before settling that state. The attacker re-enters through the OTHER function during the callback — acting on a balance that is still stale — turning one deposit into a double-spend. A nonReentrant guard on only the withdraw path does not stop it; the second entry point must share the lock. This is the Lendf.Me / dForce class (2020) and a recurring staking/AMM bug.`, + how: `Seeds the attacker with a deposit, triggers getWithdrawCalldata(), and the attacker's receive() re-enters getReentrantCalldata(accomplice) to park its still-credited balance on an accomplice. If that parked credit survives state settlement (read via balanceOfTarget), the protocol now owes more than was deposited and the check fails.`, + hooks: [ + ['function getWithdrawCalldata() returns (bytes)', 'The payout function under test (function A).'], + ['function getReentrantCalldata(address accomplice) returns (bytes)', 'The OTHER state-sharing function (B) the attacker re-enters mid-callback to move credit to the accomplice.'], + ['function performDeposit(address, uint256)', 'How to seed a withdrawable balance for the attacker.'], + ['function balanceOfTarget(address) returns (uint256)', "Read the protocol's internal credit for an account, to detect surviving moved credit."], + ], + bug: { + title: 'VulnerableCrossFunctionVault.withdraw() + transfer()', + code: `function withdraw() external { + uint256 bal = balances[msg.sender]; + require(bal > 0, "no balance"); + (bool sent,) = msg.sender.call{value: bal}(""); // external call first + balances[msg.sender] = 0; // too late — receive() re-entered transfer() +} +function transfer(address to, uint256 amount) external { // shares balances, no guard + balances[msg.sender] -= amount; balances[to] += amount; }`, }, }, @@ -174,6 +202,29 @@

Resources

function getPrice() external view returns (uint256) { // Proxy for a DEX spot read — attacker can push the pool, query, revert. return address(this).balance; +}`, + }, + }, + { + id: 'twap-oracle', + name: 'TWAPOracleCheck', + blurb: 'Price oracle manipulable within a single block (TWAP vs spot)', + severity: 'high', + file: 'TWAPOracleCheck.sol', + run: 'forge test --match-path test/TWAPOracleCheck.t.sol -vvv', + detects: `Protocols that act on an instantaneous DEX price. The stronger sibling of OracleCheck: it models the defense, not just the diff. A spot-price reader moves the moment an attacker slams the pool and is flagged; a TWAP reader barely moves within one block — its accumulator only advances with elapsed time — and passes. This is the bZx (2020), Harvest, Cheese Bank, and Inverse Finance class.`, + how: `Reads the price the contract acts on, runs manipulateSource() within a single block (no time advance), calls refreshOracle() exactly as an attacker would be forced to mid-tx, then reads again. If the acted-on price drifts past getMaxDriftPercent() (default 10%), the oracle is single-block manipulable and the check fails.`, + hooks: [ + ['function getPriceReadCalldata() returns (bytes)', 'The price read the protocol acts on; must return a single uint256.'], + ['function manipulateSource()', 'Skew the underlying source within the current block (e.g. an imbalanced swap). Must NOT advance time.'], + ['function refreshOracle()', 'Optional. Poke the consumer the way an attacker must in the same tx (e.g. call update()). Default: no-op for pure spot readers.'], + ['function getMaxDriftPercent() returns (uint256)', 'Optional. Max tolerated single-block drift. Default: 10%.'], + ], + bug: { + title: 'VulnerableSpotOracleConsumer.getPrice()', + code: `// Pure spot read — attacker swaps the pair, then this jumps in the same block. +function getPrice() external view returns (uint256) { + return pair.spotPrice0(); // VULNERABLE — use a TWAP instead }`, }, }, @@ -267,7 +318,7 @@

Hooks you implement

${c.hooks.map(([sig, desc]) => `
${sig.replace(/&/g,'&').replace(/${desc}
`).join('')} -

Bug it flags in VulnerableVault

+

Bug it flags

⚠ ${c.bug.title}
${c.bug.code}
@@ -275,7 +326,7 @@

Bug it flags in VulnerableVault

Run just this check against the shipped demo: - forge test --match-contract Example${c.name.replace('Check','')}Audit -vvv + ${c.run || `forge test --match-contract Example${c.name.replace('Check','')}Audit -vvv`}

Source