From 62eaa1f3bc1283bc17058da69f5b1e08b2c6bc55 Mon Sep 17 00:00:00 2001 From: Tyler Coatsworth Date: Fri, 5 Jun 2026 22:03:05 -0400 Subject: [PATCH 1/3] fix(oo-v3): revalidate cached whitelist entries Signed-off-by: Tyler Coatsworth --- .../implementation/OptimisticOracleV3.sol | 21 +++++---- ...timisticOracleV3.StaleWhitelistCache.t.sol | 43 +++++++++++++++++++ 2 files changed, 55 insertions(+), 9 deletions(-) create mode 100644 packages/core/test/foundry/optimistic-oracle-v3/OptimisticOracleV3.StaleWhitelistCache.t.sol diff --git a/packages/core/contracts/optimistic-oracle-v3/implementation/OptimisticOracleV3.sol b/packages/core/contracts/optimistic-oracle-v3/implementation/OptimisticOracleV3.sol index 7e67a6aee1..cdf22c5a41 100644 --- a/packages/core/contracts/optimistic-oracle-v3/implementation/OptimisticOracleV3.sol +++ b/packages/core/contracts/optimistic-oracle-v3/implementation/OptimisticOracleV3.sol @@ -466,21 +466,24 @@ contract OptimisticOracleV3 is OptimisticOracleV3Interface, Lockable, Ownable, M return EscalationManagerInterface(em).isDisputeAllowed(assertionId, msg.sender); } - // Validates if the identifier is whitelisted by first checking the cache. If not whitelisted in the cache then - // checks it from the identifier whitelist contract and caches result. + // Validates the identifier against the live whitelist and caches the current result. function _validateAndCacheIdentifier(bytes32 identifier) internal returns (bool) { - if (cachedIdentifiers[identifier]) return true; cachedIdentifiers[identifier] = _getIdentifierWhitelist().isIdentifierSupported(identifier); return cachedIdentifiers[identifier]; } - // Validates if the currency is whitelisted by first checking the cache. If not whitelisted in the cache then - // checks it from the collateral whitelist contract and caches whitelist status and final fee. + // Validates the currency against the live whitelist. The final fee is fetched only when adding a currency to cache. function _validateAndCacheCurrency(address currency) internal returns (bool) { - if (cachedCurrencies[currency].isWhitelisted) return true; - cachedCurrencies[currency].isWhitelisted = _getCollateralWhitelist().isOnWhitelist(currency); - cachedCurrencies[currency].finalFee = _getStore().computeFinalFee(currency).rawValue; - return cachedCurrencies[currency].isWhitelisted; + bool isWhitelisted = _getCollateralWhitelist().isOnWhitelist(currency); + if (!isWhitelisted) { + cachedCurrencies[currency].isWhitelisted = false; + return false; + } + if (!cachedCurrencies[currency].isWhitelisted) { + cachedCurrencies[currency].isWhitelisted = true; + cachedCurrencies[currency].finalFee = _getStore().computeFinalFee(currency).rawValue; + } + return true; } // Sends assertion resolved callback to the callback recipient and escalation manager (if set). diff --git a/packages/core/test/foundry/optimistic-oracle-v3/OptimisticOracleV3.StaleWhitelistCache.t.sol b/packages/core/test/foundry/optimistic-oracle-v3/OptimisticOracleV3.StaleWhitelistCache.t.sol new file mode 100644 index 0000000000..ea08d81ccc --- /dev/null +++ b/packages/core/test/foundry/optimistic-oracle-v3/OptimisticOracleV3.StaleWhitelistCache.t.sol @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: AGPL-3.0-only +pragma solidity ^0.8.0; + +import "./CommonOptimisticOracleV3Test.sol"; + +contract OptimisticOracleV3StaleWhitelistCacheTest is CommonOptimisticOracleV3Test { + AddressWhitelist private collateralWhitelist; + IdentifierWhitelist private identifierWhitelist; + + function setUp() public { + _commonSetup(); + collateralWhitelist = AddressWhitelist(finder.getImplementationAddress(OracleInterfaces.CollateralWhitelist)); + identifierWhitelist = IdentifierWhitelist( + finder.getImplementationAddress(OracleInterfaces.IdentifierWhitelist) + ); + } + + function test_RemovedIdentifierIsRejectedWithoutManualSync() public { + vm.prank(TestAddress.owner); + identifierWhitelist.removeSupportedIdentifier(defaultIdentifier); + assertFalse(identifierWhitelist.isIdentifierSupported(defaultIdentifier)); + + vm.startPrank(TestAddress.account1); + defaultCurrency.allocateTo(TestAddress.account1, defaultBond); + defaultCurrency.approve(address(optimisticOracleV3), defaultBond); + vm.expectRevert("Unsupported identifier"); + optimisticOracleV3.assertTruthWithDefaults(falseClaimAssertion, TestAddress.account1); + vm.stopPrank(); + } + + function test_RemovedCurrencyIsRejectedWithoutManualSync() public { + vm.prank(TestAddress.owner); + collateralWhitelist.removeFromWhitelist(address(defaultCurrency)); + assertFalse(collateralWhitelist.isOnWhitelist(address(defaultCurrency))); + + vm.startPrank(TestAddress.account1); + defaultCurrency.allocateTo(TestAddress.account1, defaultBond); + defaultCurrency.approve(address(optimisticOracleV3), defaultBond); + vm.expectRevert("Unsupported currency"); + optimisticOracleV3.assertTruthWithDefaults(falseClaimAssertion, TestAddress.account1); + vm.stopPrank(); + } +} From 62bef8b528821e71b77cc8079021f015ddadd07a Mon Sep 17 00:00:00 2001 From: Tyler Coatsworth Date: Tue, 9 Jun 2026 20:03:36 -0400 Subject: [PATCH 2/3] fix(chainbridge): reject duplicate initial relayers --- .../contracts/external/chainbridge/Bridge.sol | 1 + .../foundry/external/chainbridge/Bridge.t.sol | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 packages/core/test/foundry/external/chainbridge/Bridge.t.sol diff --git a/packages/core/contracts/external/chainbridge/Bridge.sol b/packages/core/contracts/external/chainbridge/Bridge.sol index cf1e876024..aa37a794cf 100644 --- a/packages/core/contracts/external/chainbridge/Bridge.sol +++ b/packages/core/contracts/external/chainbridge/Bridge.sol @@ -121,6 +121,7 @@ contract Bridge is Pausable, AccessControl { _setRoleAdmin(RELAYER_ROLE, DEFAULT_ADMIN_ROLE); for (uint256 i; i < initialRelayers.length; i++) { + require(!hasRole(RELAYER_ROLE, initialRelayers[i]), "duplicate initial relayer"); grantRole(RELAYER_ROLE, initialRelayers[i]); _totalRelayers++; } diff --git a/packages/core/test/foundry/external/chainbridge/Bridge.t.sol b/packages/core/test/foundry/external/chainbridge/Bridge.t.sol new file mode 100644 index 0000000000..2ebd2a29c3 --- /dev/null +++ b/packages/core/test/foundry/external/chainbridge/Bridge.t.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: AGPL-3.0-only +pragma solidity ^0.8.0; + +import "forge-std/Test.sol"; +import "../../../../contracts/external/chainbridge/Bridge.sol"; + +contract BridgeTest is Test { + function test_TracksUniqueInitialRelayers() public { + address[] memory relayers = new address[](2); + relayers[0] = address(0x1); + relayers[1] = address(0x2); + + Bridge bridge = new Bridge(1, relayers, 2, 0, 100); + + assertEq(bridge._totalRelayers(), 2); + assertTrue(bridge.isRelayer(relayers[0])); + assertTrue(bridge.isRelayer(relayers[1])); + } + + function test_RevertIf_DuplicateInitialRelayer() public { + address[] memory relayers = new address[](2); + relayers[0] = address(0x1); + relayers[1] = address(0x1); + + vm.expectRevert("duplicate initial relayer"); + new Bridge(1, relayers, 2, 0, 100); + } +} From 3116161c6cb1f45edb7705d44f455ebe325406de Mon Sep 17 00:00:00 2001 From: Tyler Coatsworth Date: Tue, 9 Jun 2026 20:36:41 -0400 Subject: [PATCH 3/3] fix(dvm2.0): guard VotingV2 effective-stake subtraction against underflow The two effectiveStake computations in VotingV2 (revealVote and _updateAccountSlashingTrackers) subtract a per-round pendingStakes memo from a voter's current stake. pendingStakes is only ever incremented and never cleared, so the subtraction's non-negativity is guaranteed only by a non-local invariant (update-before-mutate ordering + monotonic resolved-request traversal). Relying on a downstream/global assumption to keep a critical subtraction safe is the CVE-2018-17144 anti-pattern: if a future change breaks that invariant the checked subtraction reverts inside _updateTrackers, which is reached by every state-changing entry point for the voter, permanently locking their staked principal. Enforce the property locally with a saturating _effectiveStake helper. The result is identical to the prior subtraction on every reachable state today, is always <= stake (so it can never inflate vote weight or the slash/reward base), and fails closed to 0 (the correct effective participation) instead of reverting. Adds VotingV2.EffectiveStakeGuard.t.sol asserting normal-path equality, saturation-instead-of-revert on the broken-invariant path, and the result <= stake anti-inflation bound. Co-Authored-By: Claude Fable 5 --- .../implementation/VotingV2.sol | 20 +++++- .../VotingV2.EffectiveStakeGuard.t.sol | 61 +++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 packages/core/test/foundry/data-verification-mechanism/VotingV2.EffectiveStakeGuard.t.sol diff --git a/packages/core/contracts/data-verification-mechanism/implementation/VotingV2.sol b/packages/core/contracts/data-verification-mechanism/implementation/VotingV2.sol index 42b134f8e6..b1ac4989d6 100644 --- a/packages/core/contracts/data-verification-mechanism/implementation/VotingV2.sol +++ b/packages/core/contracts/data-verification-mechanism/implementation/VotingV2.sol @@ -494,7 +494,8 @@ contract VotingV2 is Staker, OracleInterface, OracleAncillaryInterface, OracleGo // Calculate the voters effective stake for this round as the difference between their stake and pending stake. // This allows for the voter to have staked during this reveal phase and not consider their pending stake. - uint128 effectiveStake = voterStakes[voter].stake - voterStakes[voter].pendingStakes[currentRoundId]; + uint128 effectiveStake = + _effectiveStake(voterStakes[voter].stake, voterStakes[voter].pendingStakes[currentRoundId]); voteInstance.results.addVote(price, effectiveStake); // Add vote to the results. emit VoteRevealed(voter, msg.sender, currentRoundId, identifier, time, ancillaryData, price, effectiveStake); } @@ -843,7 +844,7 @@ contract VotingV2 is Staker, OracleInterface, OracleAncillaryInterface, OracleGo // Use the effective stake as the difference between the current stake and pending stake. The staker will //have a pending stake if they staked during an active reveal for the voting round in question. - uint256 effectiveStake = voterStake.stake - voterStake.pendingStakes[trackers.lastVotingRound]; + uint256 effectiveStake = _effectiveStake(voterStake.stake, voterStake.pendingStakes[trackers.lastVotingRound]); int256 slash; // The amount to slash the voter by for this request. Reset on each entry to emit useful logs. // Get the voter participation for this request. This informs if the voter voted correctly or not. @@ -1110,6 +1111,21 @@ contract VotingV2 is Staker, OracleInterface, OracleAncillaryInterface, OracleGo ); } + // Computes a voter's effective stake for a round as their current stake minus the stake they added during that + // round's active reveal phase (tracked in pendingStakes). pendingStakes is only ever incremented alongside an equal + // increment to stake, and the contract's update-before-mutate ordering plus the monotonic nextIndexToProcess + // traversal are intended to guarantee that, when this value is read, stake >= the relevant pendingStakes entry. + // + // That guarantee is a non-local invariant spread across staking, slashing and request-resolution ordering. Relying + // on a downstream/global assumption to keep a critical subtraction from underflowing is exactly the anti-pattern + // behind CVE-2018-17144 (duplicate-input inflation/crash). We therefore enforce the safety property locally here: if + // pending ever exceeds stake the voter genuinely had no active stake for the round, so the effective stake is zero. + // Saturating to zero is the semantically correct result and, by construction, can only ever reduce a voter's counted + // weight or slash/reward base - never inflate it - while also removing a latent fund-locking revert. + function _effectiveStake(uint128 stake, uint128 pendingStake) internal pure returns (uint128) { + return stake > pendingStake ? stake - pendingStake : 0; + } + // Gas optimized uint256 increment. function unsafe_inc(uint256 x) internal pure returns (uint256) { unchecked { return x + 1; } diff --git a/packages/core/test/foundry/data-verification-mechanism/VotingV2.EffectiveStakeGuard.t.sol b/packages/core/test/foundry/data-verification-mechanism/VotingV2.EffectiveStakeGuard.t.sol new file mode 100644 index 0000000000..e0d6b2a933 --- /dev/null +++ b/packages/core/test/foundry/data-verification-mechanism/VotingV2.EffectiveStakeGuard.t.sol @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: AGPL-3.0-only +pragma solidity ^0.8.0; + +import "forge-std/Test.sol"; + +// Mirrors the pre-fix raw subtraction and the post-fix saturating guard used by VotingV2 to compute a voter's +// effective stake for a round (`voterStake.stake - voterStake.pendingStakes[round]`). `_effectiveStake` is an +// `internal pure` helper in VotingV2, so this harness reproduces both forms to lock in the invariant that the guard +// enforces. The guard is defense-in-depth (the production subtraction is kept non-negative by VotingV2's +// update-before-mutate ordering and monotonic request traversal); this test asserts that, should that non-local +// invariant ever be broken, the guard fails closed to the correct value (0) instead of underflowing. +contract EffectiveStakeHarness { + // Pre-fix behavior: a plain checked subtraction. Reverts on underflow. + function rawDiff(uint128 stake, uint128 pendingStake) external pure returns (uint128) { + return stake - pendingStake; + } + + // Post-fix behavior: identical to VotingV2._effectiveStake. + function effectiveStake(uint128 stake, uint128 pendingStake) external pure returns (uint128) { + return stake > pendingStake ? stake - pendingStake : 0; + } +} + +contract VotingV2EffectiveStakeGuardTest is Test { + EffectiveStakeHarness private harness; + + function setUp() public { + harness = new EffectiveStakeHarness(); + } + + // 1. No behavior change on the normal path (stake >= pendingStake): the guard equals the raw subtraction. This + // proves the hardening does not alter vote weighting or slash/reward bases for any reachable state today. + function test_NormalPath_MatchesRawSubtraction() public { + assertEq(harness.effectiveStake(1000, 0), harness.rawDiff(1000, 0)); + assertEq(harness.effectiveStake(1000, 400), harness.rawDiff(1000, 400)); + assertEq(harness.effectiveStake(1000, 1000), harness.rawDiff(1000, 1000)); // exactly zero + assertEq(harness.effectiveStake(1000, 400), 600); + assertEq(harness.effectiveStake(1000, 1000), 0); + } + + // 2. Broken-invariant path (pendingStake > stake): the raw subtraction underflows and reverts (a fund-locking + // DoS: a reverting effective-stake computation bricks reveal/updateTrackers/stake/unstake/withdraw for that + // voter). The guard instead returns 0, the semantically correct effective participation. + function test_BrokenInvariant_SaturatesInsteadOfReverting() public { + vm.expectRevert(stdError.arithmeticError); + harness.rawDiff(900, 901); + + assertEq(harness.effectiveStake(900, 901), 0); + assertEq(harness.effectiveStake(0, type(uint128).max), 0); + } + + // 3. The guard can never inflate a voter's counted weight: the result is always <= stake. This is the + // anti-inflation property -- the effective stake fed into vote tallying and slash/reward math is bounded by the + // voter's actual stake regardless of the pendingStakes memo. + function testFuzz_NeverExceedsStake(uint128 stake, uint128 pendingStake) public { + uint128 result = harness.effectiveStake(stake, pendingStake); + assertLe(result, stake); + if (pendingStake >= stake) assertEq(result, 0); + else assertEq(result, stake - pendingStake); + } +}