fix: validate msm points on 0 scalar, tests

(cherry picked from commit 7dc54f4)

Author: MirandaWood

avm-transpiler/src/procedures/msm.rs

@@ -29,7 +29,7 @@ OUTER_BODY:     MUL d6, d13, d16; Compute the pointer to the point
                 EQ i17, d8, d19; Check if the scalar lo is zero
                 EQ i18, d8, d20; Check if the scalar hi is zero
                 AND d19, d20, d19; Check if both scalars are zero
-                JUMPI d19, OUTER_INC; If both scalar limbs are zero, continue
+                JUMPI d19, VALIDATE_ZERO_SCALAR_POINT; If both scalar limbs are zero, validate the point then continue
                 ; Decompose the scalars to an array of 254 bits
                 ; Allocate a 254 bit array
                 MOV $1, d19; Move the free memory pointer to d19, where we'll store the bits
@@ -75,6 +75,15 @@ OUTER_INC:      ADD d6, $2, d6; Increment the outer loop variable
                 JUMP OUTER_HEAD
                 ; After the outer loop we have computed the msm. We can return since we wrote the result in i3, i4
 OUTER_END:      INTERNALRETURN
+
+                ; A zero scalar skips the loop where point validation takes place (inside ECADD), but Brillig/native validates /before/ the loop. 
+                ; To mirror this we perform a 'dummy' ECADD (P + O = P) which throws if the point is not on the curve.
+VALIDATE_ZERO_SCALAR_POINT: MOV i16, d22; x
+                ADD d16, $2, d25; pointer to y
+                MOV i25, d23; y
+                ; Note: d8 (=0) used as the (0,0) infinity coords here - if the infinity rep changes, update this.
+                ECADD d22, d23, d8, d8, /*not indirect, so the result (= point) is stored in d22, d23*/ d22; Add the original point to infinity
+                JUMP OUTER_INC; Skip this zero-scalar term
 ";
 
 #[cfg(test)]

noir-projects/noir-contracts/contracts/test/avm_test_contract/src/main.nr

@@ -224,6 +224,23 @@ pub contract AvmTest {
         )
     }
 
+    #[external("public")]
+    fn variable_base_msm_with_point(
+        px: Field,
+        py: Field,
+        scalar_lo: Field,
+        scalar_hi: Field,
+        scalar2_lo: Field,
+        scalar2_hi: Field,
+    ) -> EmbeddedCurvePoint {
+        let p = EmbeddedCurvePoint { x: px, y: py };
+
+        multi_scalar_mul(
+            [p, p],
+            [Scalar { lo: scalar_lo, hi: scalar_hi }, Scalar { lo: scalar2_lo, hi: scalar2_hi }],
+        )
+    }
+
     #[external("public")]
     fn pedersen_commit(x: Field, y: Field) -> EmbeddedCurvePoint {
         std::hash::pedersen_commitment_with_separator([x, y], 20)

yarn-project/bb-prover/src/avm_proving_tests/avm_msm_zero_scalar_invalid_point.test.ts

@@ -0,0 +1,57 @@
+import { AvmTestContractArtifact } from '@aztec/noir-test-contracts.js/AvmTest';
+import { defaultGlobals } from '@aztec/simulator/public/fixtures';
+import { AztecAddress } from '@aztec/stdlib/aztec-address';
+import type { ContractInstanceWithAddress } from '@aztec/stdlib/contract';
+import { NativeWorldStateService } from '@aztec/world-state';
+
+import { AvmProvingTester } from './avm_proving_tester.js';
+
+// Regression for the AVM-transpiled MSM zero-scalar invalid-point gap, exercised through the full bb-prover
+// proving path.
+//
+// The hand-written MSM procedure (avm-transpiler/src/procedures/msm.rs) used to skip a term as soon as both
+// scalar limbs were zero, before validating the corresponding point. Native Brillig/BN254 `multi_scalar_mul`
+// validates every input point regardless of its scalar, and there is a native test that rejects
+// (x=1, y=1, is_infinite=false) even with a zero scalar
+// (noir/noir-repo/acvm-repo/bn254_blackbox_solver/src/embedded_curve_ops.rs). The fix forces an on-curve
+// ECADD check for every input point before the zero-scalar skip, so this off-curve point must now revert in
+// the public AVM proving path instead of silently returning the point at infinity.
+//
+// The AvmTestContract `variable_base_msm_with_point` function takes the point coordinates as arguments, so
+// we can hand it the off-curve point (1, 1) directly without patching bytecode.
+
+describe('AVM MSM zero-scalar invalid-point regression', () => {
+  let tester: AvmProvingTester;
+  let worldStateService: NativeWorldStateService;
+
+  const sender = AztecAddress.fromNumberUnsafe(42);
+  let contract: ContractInstanceWithAddress;
+
+  beforeEach(async () => {
+    worldStateService = await NativeWorldStateService.tmp();
+    // FULL PROVING (not check-circuit) so the gap is exercised through the real bb-prover path.
+    tester = await AvmProvingTester.new(worldStateService, /*checkCircuitOnly=*/ false, /*globals=*/ defaultGlobals());
+    contract = await tester.registerAndDeployContract(
+      /*constructorArgs=*/ [],
+      sender,
+      /*contractArtifact=*/ AvmTestContractArtifact,
+    );
+  });
+
+  afterEach(async () => {
+    await worldStateService.close();
+  });
+
+  it('reverts on an off-curve point even when its scalar is zero', async () => {
+    await tester.simProveVerifyAppLogic(
+      {
+        address: contract.address,
+        fnName: 'variable_base_msm_with_point',
+        // Off-curve point (1, 1) with a zero scalar: the term contributes nothing but must still be validated.
+        args: [/*px=*/ 1, /*py=*/ 1, /*scalar_lo=*/ 0, /*scalar_hi=*/ 0, /*scalar2_lo=*/ 20, /*scalar2_hi=*/ 0],
+      },
+      /*expectRevert=*/ true,
+      /*txLabel=*/ 'msm-zero-scalar-invalid-point',
+    );
+  }, 180_000);
+});