fix(IB20Asset): reject zero multiplier in updateMultiplier (BOP-328)

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;
 
     /*//////////////////////////////////////////////////////////////

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);
     }

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");
     }
 }

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)"
         );

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,

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,

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);

test/unit/B20Asset/multiplier/updateMultiplier_revertOrder.t.sol

@@ -2,18 +2,19 @@
 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";
 
 /// @title Sequential revert-order test for `updateMultiplier`.
 ///
 /// @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);
     }
 }