From 3429dbc982eb28c0645a3bd2c3e8f9f9ae24a3a8 Mon Sep 17 00:00:00 2001 From: Eric Shenghsiung Liu Date: Tue, 9 Jun 2026 22:12:51 -0400 Subject: [PATCH] fix(IB20Asset): reject zero multiplier in updateMultiplier (BOP-328) Ports the zero-multiplier validation from base/base#3367 into base-std. Zero is the uninitialized-storage sentinel that reads as WAD on every read path; allowing it through would emit MultiplierUpdated(0) while multiplier() returns 1e18, desynchronizing off-chain indexers. Changes: - IB20Asset: add InvalidMultiplier error, document revert in updateMultiplier NatSpec - MockB20Asset: revert with InvalidMultiplier when newMultiplier is zero - updateMultiplier.t.sol: add revert test for zero, guard success fuzz tests against zero - updateMultiplier_revertOrder.t.sol: extend to two-step order (role, then zero check) - multiplier/toScaledBalance/toRawBalance/scaledBalanceOf tests: replace _updateMultiplier(0) with vm.store to isolate read-path fallback testing --- src/interfaces/IB20Asset.sol | 6 +++++- test/lib/mocks/MockB20Asset.sol | 1 + .../unit/B20Asset/multiplier/multiplier.t.sol | 10 +++++---- .../B20Asset/multiplier/scaledBalanceOf.t.sol | 13 ++++++------ .../B20Asset/multiplier/toRawBalance.t.sol | 13 ++++++------ .../B20Asset/multiplier/toScaledBalance.t.sol | 14 ++++++------- .../multiplier/updateMultiplier.t.sol | 13 ++++++++++++ .../updateMultiplier_revertOrder.t.sol | 21 +++++++++++++------ 8 files changed, 61 insertions(+), 30 deletions(-) diff --git a/src/interfaces/IB20Asset.sol b/src/interfaces/IB20Asset.sol index c8d4900..216c8a8 100644 --- a/src/interfaces/IB20Asset.sol +++ b/src/interfaces/IB20Asset.sol @@ -20,6 +20,9 @@ interface IB20Asset is IB20 { /// @notice `updateExtraMetadata` was called with an empty `key`. error InvalidMetadataKey(); + /// @notice `updateMultiplier` was called with a zero multiplier. + error InvalidMultiplier(); + /// @notice A batched function was called with parallel arrays of differing lengths. /// /// @param leftLen Length of the first array argument. @@ -149,8 +152,9 @@ interface IB20Asset is IB20 { /// derive from the new multiplier at read time. Emits `MultiplierUpdated`. /// /// @dev Reverts with `AccessControlUnauthorizedAccount` when the caller does not hold `OPERATOR_ROLE`. + /// @dev Reverts with `InvalidMultiplier` when `newMultiplier` is zero. /// - /// @param newMultiplier New multiplier scaled to `WAD_PRECISION`. + /// @param newMultiplier New multiplier scaled to `WAD_PRECISION`; must be non-zero. function updateMultiplier(uint256 newMultiplier) external; /*////////////////////////////////////////////////////////////// diff --git a/test/lib/mocks/MockB20Asset.sol b/test/lib/mocks/MockB20Asset.sol index a4c7336..c2befee 100644 --- a/test/lib/mocks/MockB20Asset.sol +++ b/test/lib/mocks/MockB20Asset.sol @@ -133,6 +133,7 @@ contract MockB20Asset is MockB20, IB20Asset { } function updateMultiplier(uint256 newMultiplier) external onlyRole(OPERATOR_ROLE) { + if (newMultiplier == 0) revert InvalidMultiplier(); MockB20AssetStorage.layout().multiplier = newMultiplier; emit MultiplierUpdated(newMultiplier); } diff --git a/test/unit/B20Asset/multiplier/multiplier.t.sol b/test/unit/B20Asset/multiplier/multiplier.t.sol index 937a979..f15d3f8 100644 --- a/test/unit/B20Asset/multiplier/multiplier.t.sol +++ b/test/unit/B20Asset/multiplier/multiplier.t.sol @@ -33,12 +33,14 @@ contract B20AssetMultiplierTest is B20AssetTest { ); } - /// @notice Verifies writing zero re-activates the WAD fallback - /// @dev Operator can revert to the default 1:1 multiplier by writing 0; the read surface's - /// `stored == 0 ? WAD : stored` collapses zero back to WAD. + /// @notice Verifies the WAD fallback applies when the stored slot is zero after a prior write + /// @dev The read surface's `stored == 0 ? WAD : stored` sentinel applies regardless of whether + /// zero was the initial value or was written directly to the slot. `updateMultiplier(0)` + /// now reverts (InvalidMultiplier), so we zero the slot via vm.store to isolate the + /// read-path fallback from the write-path validation. function test_multiplier_success_zeroRestoresWadFallback() public { _updateMultiplier(5e18); - _updateMultiplier(0); + vm.store(address(token), MockB20AssetStorage.multiplierSlot(), bytes32(0)); assertEq(asset().multiplier(), asset().WAD_PRECISION(), "multiplier must collapse to WAD after zero"); } } diff --git a/test/unit/B20Asset/multiplier/scaledBalanceOf.t.sol b/test/unit/B20Asset/multiplier/scaledBalanceOf.t.sol index 0dc3dfc..63add21 100644 --- a/test/unit/B20Asset/multiplier/scaledBalanceOf.t.sol +++ b/test/unit/B20Asset/multiplier/scaledBalanceOf.t.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.20; import {B20AssetTest} from "base-std-test/lib/B20AssetTest.sol"; +import {MockB20AssetStorage} from "base-std-test/lib/mocks/MockB20Storage.sol"; + contract B20AssetScaledBalanceOfTest is B20AssetTest { /// @notice Verifies scaledBalanceOf is zero for an account with no balance /// @dev Property: empty balance => zero scaled balance regardless of the multiplier. @@ -47,17 +49,16 @@ contract B20AssetScaledBalanceOfTest is B20AssetTest { ); } - /// @notice Verifies scaledBalanceOf applies the WAD fallback when the stored multiplier is explicitly zero - /// @dev A stored `multiplier` of zero is documented to resolve as `WAD_PRECISION` on the read - /// surface, both pre-write (fresh slot) and post-explicit-zero-write. Tests the latter at - /// the derived-function level so a refactor that reads the slot directly here would fail. - /// See test_multiplier_success_zeroRestoresWadFallback for the base-level fallback assertion. + /// @notice Verifies scaledBalanceOf applies the WAD fallback when the stored multiplier is zero + /// @dev A stored `multiplier` of zero resolves as `WAD_PRECISION` on the read surface. + /// `updateMultiplier(0)` now reverts (InvalidMultiplier), so we zero the slot via + /// vm.store to isolate the read-path fallback from write-path validation. function test_scaledBalanceOf_success_explicitZeroMultiplierFallsBackToWad(address account, uint256 amount) public { _assumeValidActor(account); amount = bound(amount, 0, type(uint128).max); if (amount > 0) _mint(account, amount); _updateMultiplier(5e18); // seed a non-zero value first - _updateMultiplier(0); // then explicitly clear back to zero + vm.store(address(token), MockB20AssetStorage.multiplierSlot(), bytes32(0)); // zero the slot directly assertEq( asset().scaledBalanceOf(account), amount, "stored zero multiplier must produce identity (WAD fallback)" ); diff --git a/test/unit/B20Asset/multiplier/toRawBalance.t.sol b/test/unit/B20Asset/multiplier/toRawBalance.t.sol index 0afaff3..b1ec972 100644 --- a/test/unit/B20Asset/multiplier/toRawBalance.t.sol +++ b/test/unit/B20Asset/multiplier/toRawBalance.t.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.20; import {B20AssetTest} from "base-std-test/lib/B20AssetTest.sol"; +import {MockB20AssetStorage} from "base-std-test/lib/mocks/MockB20Storage.sol"; + contract B20AssetToRawBalanceTest is B20AssetTest { /// @notice Verifies toRawBalance is the identity on a fresh token (WAD multiplier) /// @dev Default multiplier is WAD, so scaledBalance * WAD / WAD == scaledBalance for every input. @@ -33,15 +35,14 @@ contract B20AssetToRawBalanceTest is B20AssetTest { assertEq(asset().toRawBalance(0), 0, "zero scaled balance must produce zero raw balance"); } - /// @notice Verifies toRawBalance applies the WAD fallback when the stored multiplier is explicitly zero - /// @dev A stored `multiplier` of zero is documented to resolve as `WAD_PRECISION` on the read - /// surface, both pre-write (fresh slot) and post-explicit-zero-write. Tests the latter at - /// the derived-function level so a refactor that reads the slot directly here would fail. - /// See test_multiplier_success_zeroRestoresWadFallback for the base-level fallback assertion. + /// @notice Verifies toRawBalance applies the WAD fallback when the stored multiplier is zero + /// @dev A stored `multiplier` of zero resolves as `WAD_PRECISION` on the read surface. + /// `updateMultiplier(0)` now reverts (InvalidMultiplier), so we zero the slot via + /// vm.store to isolate the read-path fallback from write-path validation. function test_toRawBalance_success_explicitZeroMultiplierFallsBackToWad(uint256 scaledBalance) public { scaledBalance = bound(scaledBalance, 0, type(uint128).max); _updateMultiplier(5e18); // seed a non-zero value first - _updateMultiplier(0); // then explicitly clear back to zero + vm.store(address(token), MockB20AssetStorage.multiplierSlot(), bytes32(0)); // zero the slot directly assertEq( asset().toRawBalance(scaledBalance), scaledBalance, diff --git a/test/unit/B20Asset/multiplier/toScaledBalance.t.sol b/test/unit/B20Asset/multiplier/toScaledBalance.t.sol index d0f8eea..0b595fb 100644 --- a/test/unit/B20Asset/multiplier/toScaledBalance.t.sol +++ b/test/unit/B20Asset/multiplier/toScaledBalance.t.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.20; import {B20AssetTest} from "base-std-test/lib/B20AssetTest.sol"; +import {MockB20AssetStorage} from "base-std-test/lib/mocks/MockB20Storage.sol"; + contract B20AssetToScaledBalanceTest is B20AssetTest { /// @notice Verifies toScaledBalance is the identity on a fresh token (WAD multiplier) /// @dev Default multiplier is WAD, so rawBalance * WAD / WAD == rawBalance for every input. @@ -33,16 +35,14 @@ contract B20AssetToScaledBalanceTest is B20AssetTest { assertEq(asset().toScaledBalance(0), 0, "zero rawBalance must produce zero scaled balance"); } - /// @notice Verifies toScaledBalance applies the WAD fallback when the stored multiplier is explicitly zero - /// @dev A stored `multiplier` of zero is documented to resolve as `WAD_PRECISION` on the read - /// surface, both pre-write (fresh slot) and post-explicit-zero-write. Tests the latter: - /// after writing zero, toScaledBalance must behave as if the multiplier were WAD - /// (identity). Cross-references test_multiplier_success_zeroRestoresWadFallback at the - /// derived-function level so a refactor that reads the slot directly here would fail. + /// @notice Verifies toScaledBalance applies the WAD fallback when the stored multiplier is zero + /// @dev A stored `multiplier` of zero resolves as `WAD_PRECISION` on the read surface. + /// `updateMultiplier(0)` now reverts (InvalidMultiplier), so we zero the slot via + /// vm.store to isolate the read-path fallback from write-path validation. function test_toScaledBalance_success_explicitZeroMultiplierFallsBackToWad(uint256 rawBalance) public { rawBalance = bound(rawBalance, 0, type(uint128).max); _updateMultiplier(5e18); // seed a non-zero value first - _updateMultiplier(0); // then explicitly clear back to zero + vm.store(address(token), MockB20AssetStorage.multiplierSlot(), bytes32(0)); // zero the slot directly assertEq( asset().toScaledBalance(rawBalance), rawBalance, diff --git a/test/unit/B20Asset/multiplier/updateMultiplier.t.sol b/test/unit/B20Asset/multiplier/updateMultiplier.t.sol index 1b3df9b..d24f392 100644 --- a/test/unit/B20Asset/multiplier/updateMultiplier.t.sol +++ b/test/unit/B20Asset/multiplier/updateMultiplier.t.sol @@ -22,11 +22,23 @@ contract B20AssetUpdateMultiplierTest is B20AssetTest { asset().updateMultiplier(newMultiplier); } + /// @notice Verifies updateMultiplier reverts when newMultiplier is zero + /// @dev Input validation: zero is an invalid multiplier because stored zero is the + /// uninitialized-storage sentinel (read path normalizes it to WAD). Passing zero + /// would create an event/read inconsistency for off-chain indexers. + function test_updateMultiplier_revert_zeroMultiplier() public { + _grantOperator(); + vm.prank(operator); + vm.expectRevert(IB20Asset.InvalidMultiplier.selector); + asset().updateMultiplier(0); + } + /// @notice Verifies updateMultiplier writes the new value to the stored slot /// @dev State invariant: the stored slot holds the supplied multiplier verbatim (no clamping, /// no scaling). Paired slot assertion verifies the storage write lands at the /// multiplier slot. function test_updateMultiplier_success_writesSlot(uint256 newMultiplier) public { + vm.assume(newMultiplier != 0); _updateMultiplier(newMultiplier); assertEq( uint256(vm.load(address(token), MockB20AssetStorage.multiplierSlot())), @@ -39,6 +51,7 @@ contract B20AssetUpdateMultiplierTest is B20AssetTest { /// @dev Event integrity for the rotation; subscribers depend on this event to /// re-derive holder scaled balances off-chain. function test_updateMultiplier_success_emitsEvent(uint256 newMultiplier) public { + vm.assume(newMultiplier != 0); _grantOperator(); vm.expectEmit(false, false, false, true, address(token)); emit IB20Asset.MultiplierUpdated(newMultiplier); diff --git a/test/unit/B20Asset/multiplier/updateMultiplier_revertOrder.t.sol b/test/unit/B20Asset/multiplier/updateMultiplier_revertOrder.t.sol index 54abbfb..84e2f8b 100644 --- a/test/unit/B20Asset/multiplier/updateMultiplier_revertOrder.t.sol +++ b/test/unit/B20Asset/multiplier/updateMultiplier_revertOrder.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.20; import {IB20} from "base-std/interfaces/IB20.sol"; +import {IB20Asset} from "base-std/interfaces/IB20Asset.sol"; import {B20AssetTest} from "base-std-test/lib/B20AssetTest.sol"; @@ -9,11 +10,11 @@ import {B20AssetTest} from "base-std-test/lib/B20AssetTest.sol"; /// /// @notice **Canonical order (Solidity reference):** /// 1. ROLE (`onlyRole(OPERATOR_ROLE)` modifier) → `AccessControlUnauthorizedAccount` +/// 2. INVALID-MULTIPLIER (`newMultiplier == 0`) → `InvalidMultiplier` /// -/// A single test verifies the guard fires for an unauthorized caller and then -/// confirms the call succeeds once the role is granted. +/// Walks from all conditions broken to success, fixing one per step. contract B20AssetUpdateMultiplierRevertOrderTest is B20AssetTest { - function test_updateMultiplier_revertOrder(address caller, uint256 newMultiplier) public { + function test_updateMultiplier_revertOrder(address caller) public { // Exclude precompiles (which can distort msg.sender) and admin (needed // internally by _grantRole to approve the role grant). _assumeValidCaller(caller); @@ -24,16 +25,24 @@ contract B20AssetUpdateMultiplierRevertOrderTest is B20AssetTest { // before the state-changing call, sending it as address(this) instead. bytes32 operatorRole = asset().OPERATOR_ROLE(); - // 1. ROLE fires. + // 1. ROLE fires: caller lacks OPERATOR_ROLE AND multiplier is zero. + // The role modifier runs before the body's zero-multiplier check. vm.prank(caller); vm.expectRevert(abi.encodeWithSelector(IB20.AccessControlUnauthorizedAccount.selector, caller, operatorRole)); - asset().updateMultiplier(newMultiplier); + asset().updateMultiplier(0); // Fix: grant OPERATOR_ROLE to caller. _grantRole(operatorRole, caller); + // 2. INVALID-MULTIPLIER fires: caller now holds the role, but multiplier is still zero. + vm.prank(caller); + vm.expectRevert(IB20Asset.InvalidMultiplier.selector); + asset().updateMultiplier(0); + + // Fix: pass a non-zero multiplier. + // Success: all conditions resolved. vm.prank(caller); - asset().updateMultiplier(newMultiplier); + asset().updateMultiplier(1e18); } }