Skip to content

Commit 632095c

Browse files
authored
fix(port): validate AVM MSM points on zero scalar (forward-port onto next) (#24729)
Forward-port of the AVM MSM zero-scalar invalid-point fix onto `next`. ## Why this is needed The fix landed on the **`v5`/`v5-next`** line (commit `7dc54f4c0cb`, *"fix: validate msm points on 0 scalar, tests"*, MirandaWood) but was **never forward-ported onto `next`**. Public `next`/`v6-next` still carry the vulnerable code — `avm-transpiler/src/procedures/msm.rs` jumps straight to `OUTER_INC` on a zero scalar, skipping point validation. This is the public analogue of the same missed forward-port that stranded the private-repo fix (see AztecProtocol/aztec-packages-private#622). ## The change (cherry-pick of `7dc54f4c0cb`) `avm-transpiler/src/procedures/msm.rs`: a zero-scalar MSM term used to skip the ECADD loop where on-curve point validation happens, so an off-curve point paired with a zero scalar was silently accepted (returned infinity) instead of reverting. The fix adds a `VALIDATE_ZERO_SCALAR_POINT` branch performing a dummy `P + O` ECADD, forcing the on-curve check — matching native/Brillig `multi_scalar_mul` semantics. Includes the `variable_base_msm_with_point` helper in `avm_test_contract` and the `avm_msm_zero_scalar_invalid_point` bb-prover proving test. ## Port adjustments - **API drift fixed:** the ported test used `AztecAddress.fromNumber(42)`, which does not exist on `next`; changed to `fromNumberUnsafe(42)` to match every sibling `avm_proving_tests` file (this is exactly what broke private #622's CI). - **Dropped file:** `yarn-project/simulator/src/public/avm/avm_simulator.test.ts` was deleted on `next` (AVM simulator tests relocated). Its two *"reverts off-curve points"* assertions are **not carried**; the bb-prover proving test covers the zero-scalar off-curve case end-to-end. Re-homing those assertions into next's relocated suite is a follow-up. - **Possible snapshot regen:** adding `variable_base_msm_with_point` to `avm_test_contract` may require regenerating its expanded snapshot on `next`. The upstream `v5` commit didn't touch a snapshot; if next's toolchain expands it differently the snapshot test will flag it and I'll regenerate/babysit. Draft until CI is green. `ci-no-fail-fast` applied so the full matrix reports. Refs AztecProtocol/aztec-packages-private#622 --- *Created by [claudebox](https://claudebox.work/v2/sessions/3b80f12215e0c82d) · group: `slackbot`*
2 parents 0df3ee2 + f99dfc1 commit 632095c

3 files changed

Lines changed: 84 additions & 1 deletion

File tree

avm-transpiler/src/procedures/msm.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ OUTER_BODY: MUL d6, d13, d16; Compute the pointer to the point
2929
EQ i17, d8, d19; Check if the scalar lo is zero
3030
EQ i18, d8, d20; Check if the scalar hi is zero
3131
AND d19, d20, d19; Check if both scalars are zero
32-
JUMPI d19, OUTER_INC; If both scalar limbs are zero, continue
32+
JUMPI d19, VALIDATE_ZERO_SCALAR_POINT; If both scalar limbs are zero, validate the point then continue
3333
; Decompose the scalars to an array of 254 bits
3434
; Allocate a 254 bit array
3535
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
7575
JUMP OUTER_HEAD
7676
; After the outer loop we have computed the msm. We can return since we wrote the result in i3, i4
7777
OUTER_END: INTERNALRETURN
78+
79+
; A zero scalar skips the loop where point validation takes place (inside ECADD), but Brillig/native validates /before/ the loop.
80+
; To mirror this we perform a 'dummy' ECADD (P + O = P) which throws if the point is not on the curve.
81+
VALIDATE_ZERO_SCALAR_POINT: MOV i16, d22; x
82+
ADD d16, $2, d25; pointer to y
83+
MOV i25, d23; y
84+
; Note: d8 (=0) used as the (0,0) infinity coords here - if the infinity rep changes, update this.
85+
ECADD d22, d23, d8, d8, /*not indirect, so the result (= point) is stored in d22, d23*/ d22; Add the original point to infinity
86+
JUMP OUTER_INC; Skip this zero-scalar term
7887
";
7988

8089
#[cfg(test)]

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,23 @@ pub contract AvmTest {
224224
)
225225
}
226226

227+
#[external("public")]
228+
fn variable_base_msm_with_point(
229+
px: Field,
230+
py: Field,
231+
scalar_lo: Field,
232+
scalar_hi: Field,
233+
scalar2_lo: Field,
234+
scalar2_hi: Field,
235+
) -> EmbeddedCurvePoint {
236+
let p = EmbeddedCurvePoint { x: px, y: py };
237+
238+
multi_scalar_mul(
239+
[p, p],
240+
[Scalar { lo: scalar_lo, hi: scalar_hi }, Scalar { lo: scalar2_lo, hi: scalar2_hi }],
241+
)
242+
}
243+
227244
#[external("public")]
228245
fn pedersen_commit(x: Field, y: Field) -> EmbeddedCurvePoint {
229246
std::hash::pedersen_commitment_with_separator([x, y], 20)
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
import { AvmTestContractArtifact } from '@aztec/noir-test-contracts.js/AvmTest';
2+
import { defaultGlobals } from '@aztec/simulator/public/fixtures';
3+
import { AztecAddress } from '@aztec/stdlib/aztec-address';
4+
import type { ContractInstanceWithAddress } from '@aztec/stdlib/contract';
5+
import { NativeWorldStateService } from '@aztec/world-state';
6+
7+
import { AvmProvingTester } from './avm_proving_tester.js';
8+
9+
// Regression for the AVM-transpiled MSM zero-scalar invalid-point gap, exercised through the full bb-prover
10+
// proving path.
11+
//
12+
// The hand-written MSM procedure (avm-transpiler/src/procedures/msm.rs) used to skip a term as soon as both
13+
// scalar limbs were zero, before validating the corresponding point. Native Brillig/BN254 `multi_scalar_mul`
14+
// validates every input point regardless of its scalar, and there is a native test that rejects
15+
// (x=1, y=1, is_infinite=false) even with a zero scalar
16+
// (noir/noir-repo/acvm-repo/bn254_blackbox_solver/src/embedded_curve_ops.rs). The fix forces an on-curve
17+
// ECADD check for every input point before the zero-scalar skip, so this off-curve point must now revert in
18+
// the public AVM proving path instead of silently returning the point at infinity.
19+
//
20+
// The AvmTestContract `variable_base_msm_with_point` function takes the point coordinates as arguments, so
21+
// we can hand it the off-curve point (1, 1) directly without patching bytecode.
22+
23+
describe('AVM MSM zero-scalar invalid-point regression', () => {
24+
let tester: AvmProvingTester;
25+
let worldStateService: NativeWorldStateService;
26+
27+
const sender = AztecAddress.fromNumberUnsafe(42);
28+
let contract: ContractInstanceWithAddress;
29+
30+
beforeEach(async () => {
31+
worldStateService = await NativeWorldStateService.tmp();
32+
// FULL PROVING (not check-circuit) so the gap is exercised through the real bb-prover path.
33+
tester = await AvmProvingTester.new(worldStateService, /*checkCircuitOnly=*/ false, /*globals=*/ defaultGlobals());
34+
contract = await tester.registerAndDeployContract(
35+
/*constructorArgs=*/ [],
36+
sender,
37+
/*contractArtifact=*/ AvmTestContractArtifact,
38+
);
39+
});
40+
41+
afterEach(async () => {
42+
await worldStateService.close();
43+
});
44+
45+
it('reverts on an off-curve point even when its scalar is zero', async () => {
46+
await tester.simProveVerifyAppLogic(
47+
{
48+
address: contract.address,
49+
fnName: 'variable_base_msm_with_point',
50+
// Off-curve point (1, 1) with a zero scalar: the term contributes nothing but must still be validated.
51+
args: [/*px=*/ 1, /*py=*/ 1, /*scalar_lo=*/ 0, /*scalar_hi=*/ 0, /*scalar2_lo=*/ 20, /*scalar2_hi=*/ 0],
52+
},
53+
/*expectRevert=*/ true,
54+
/*txLabel=*/ 'msm-zero-scalar-invalid-point',
55+
);
56+
}, 180_000);
57+
});

0 commit comments

Comments
 (0)