diff --git a/contracts/PoseidonHasher.sol b/contracts/PoseidonHasher.sol index 9b95466..ded0daa 100644 --- a/contracts/PoseidonHasher.sol +++ b/contracts/PoseidonHasher.sol @@ -11,7 +11,7 @@ interface IPoseidonHasher { } contract PoseidonHasher is IPoseidonHasher { - uint256 constant Q = 21888242871839275222246405745257275088548364400416034343698204186575808495617; + uint256 public constant Q = 21888242871839275222246405745257275088548364400416034343698204186575808495617; uint256 constant C0 = 4417881134626180770308697923359573201005643519861877412381846989312604493735; uint256 constant C1 = 5433650512959517612316327474713065966758808864213826738576266661723522780033; uint256 constant C2 = 13641176377184356099764086973022553863760045607496549923679278773208775739952; diff --git a/contracts/RlnBase.sol b/contracts/RlnBase.sol index cff9a39..f729258 100644 --- a/contracts/RlnBase.sol +++ b/contracts/RlnBase.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.15; -import {IPoseidonHasher} from "./PoseidonHasher.sol"; +import {PoseidonHasher} from "./PoseidonHasher.sol"; import {IVerifier} from "./IVerifier.sol"; /// The tree is full @@ -19,6 +19,9 @@ error DuplicateIdCommitment(); /// Failed validation on registration/slashing error FailedValidation(); +/// Invalid idCommitment +error InvalidIdCommitment(uint256 idCommitment); + /// Invalid receiver address, when the receiver is the contract itself or 0x0 error InvalidReceiverAddress(address to); @@ -62,7 +65,7 @@ abstract contract RlnBase { mapping(address => uint256) public withdrawalBalance; /// @notice The Poseidon hasher contract - IPoseidonHasher public immutable poseidonHasher; + PoseidonHasher public immutable poseidonHasher; /// @notice The groth16 verifier contract IVerifier public immutable verifier; @@ -77,17 +80,22 @@ abstract contract RlnBase { /// @param index The index of the member in the set event MemberWithdrawn(uint256 idCommitment, uint256 index); + modifier onlyValidIdCommitment(uint256 idCommitment) { + if (!isValidCommitment(idCommitment)) revert InvalidIdCommitment(idCommitment); + _; + } + constructor(uint256 membershipDeposit, uint256 depth, address _poseidonHasher, address _verifier) { MEMBERSHIP_DEPOSIT = membershipDeposit; DEPTH = depth; SET_SIZE = 1 << depth; - poseidonHasher = IPoseidonHasher(_poseidonHasher); + poseidonHasher = PoseidonHasher(_poseidonHasher); verifier = IVerifier(_verifier); } /// Allows a user to register as a member /// @param idCommitment The idCommitment of the member - function register(uint256 idCommitment) external payable virtual { + function register(uint256 idCommitment) external payable virtual onlyValidIdCommitment(idCommitment) { if (msg.value != MEMBERSHIP_DEPOSIT) { revert InsufficientDeposit(MEMBERSHIP_DEPOSIT, msg.value); } @@ -114,7 +122,11 @@ abstract contract RlnBase { /// @dev Allows a user to slash a member /// @param idCommitment The idCommitment of the member - function slash(uint256 idCommitment, address payable receiver, uint256[8] calldata proof) external virtual { + function slash(uint256 idCommitment, address payable receiver, uint256[8] calldata proof) + external + virtual + onlyValidIdCommitment(idCommitment) + { _validateSlash(idCommitment, receiver, proof); _slash(idCommitment, receiver, proof); } @@ -177,6 +189,10 @@ abstract contract RlnBase { return poseidonHasher.hash(input); } + function isValidCommitment(uint256 idCommitment) public view returns (bool) { + return idCommitment != 0 && idCommitment < poseidonHasher.Q(); + } + /// @dev Groth16 proof verification function _verifyProof(uint256 idCommitment, address receiver, uint256[8] calldata proof) internal diff --git a/docs/index.md b/docs/index.md index b17362c..be17df5 100644 --- a/docs/index.md +++ b/docs/index.md @@ -905,6 +905,14 @@ error FailedValidation() Failed validation on registration/slashing +## InvalidIdCommitment + +```solidity +error InvalidIdCommitment(uint256 idCommitment) +``` + +Invalid idCommitment + ## InvalidReceiverAddress ```solidity @@ -1016,7 +1024,7 @@ The balance of each user that can be withdrawn ### poseidonHasher ```solidity -contract IPoseidonHasher poseidonHasher +contract PoseidonHasher poseidonHasher ``` The Poseidon hasher contract @@ -1059,6 +1067,12 @@ Emitted when a member is removed from the set | idCommitment | uint256 | The idCommitment of the member | | index | uint256 | The index of the member in the set | +### onlyValidIdCommitment + +```solidity +modifier onlyValidIdCommitment(uint256 idCommitment) +``` + ### constructor ```solidity @@ -1164,6 +1178,12 @@ NOTE: The variant of Poseidon we use accepts only 1 input, assume n=2, and the s | ----- | ------- | ----------------- | | input | uint256 | The value to hash | +### isValidCommitment + +```solidity +function isValidCommitment(uint256 idCommitment) public view returns (bool) +``` + ### \_verifyProof ```solidity diff --git a/test/RLNApp.t.sol b/test/RLNApp.t.sol index 7c2d2f4..b6293ef 100644 --- a/test/RLNApp.t.sol +++ b/test/RLNApp.t.sol @@ -10,7 +10,7 @@ import "forge-std/console.sol"; contract RlnApp is RlnBase { uint256 public constant allowedIdCommitment = - 21888242871839275222246405745257275088548364400416034343698204186575808495617; + 19014214495641488759237505126948346942972912379615652741039992445865937985820; uint256 private membershipDeposit = 1000000000000000; uint256 private depth = 20; @@ -57,6 +57,7 @@ contract RLNAppTest is Test { function test__InvalidRegistration(uint256 idCommitment) public { vm.assume(idCommitment != rlnApp.allowedIdCommitment()); + vm.assume(rlnApp.isValidCommitment(idCommitment)); vm.expectRevert(FailedValidation.selector); rlnApp.register{value: MEMBERSHIP_DEPOSIT}(idCommitment); } diff --git a/test/Rln.t.sol b/test/Rln.t.sol index 511f22f..07df987 100644 --- a/test/Rln.t.sol +++ b/test/Rln.t.sol @@ -37,12 +37,14 @@ contract RlnTest is Test { } function test__ValidRegistration(uint256 idCommitment) public { + vm.assume(rln.isValidCommitment(idCommitment)); rln.register{value: MEMBERSHIP_DEPOSIT}(idCommitment); assertEq(rln.stakedAmounts(idCommitment), MEMBERSHIP_DEPOSIT); assertEq(rln.members(idCommitment), 1); } function test__InvalidRegistration__DuplicateCommitment(uint256 idCommitment) public { + vm.assume(rln.isValidCommitment(idCommitment)); rln.register{value: MEMBERSHIP_DEPOSIT}(idCommitment); assertEq(rln.stakedAmounts(idCommitment), MEMBERSHIP_DEPOSIT); assertEq(rln.members(idCommitment), 1); @@ -51,13 +53,13 @@ contract RlnTest is Test { } function test__InvalidRegistration__InsufficientDeposit(uint256 idCommitment) public { + vm.assume(rln.isValidCommitment(idCommitment)); uint256 badDepositAmount = MEMBERSHIP_DEPOSIT - 1; vm.expectRevert(abi.encodeWithSelector(InsufficientDeposit.selector, MEMBERSHIP_DEPOSIT, badDepositAmount)); rln.register{value: badDepositAmount}(idCommitment); } - function test__InvalidRegistration__FullSet(uint256 idCommitmentSeed) public { - vm.assume(idCommitmentSeed < 2 ** 255 - SET_SIZE); + function test__InvalidRegistration__FullSet() public { Rln tempRln = new Rln( MEMBERSHIP_DEPOSIT, 2, @@ -65,12 +67,12 @@ contract RlnTest is Test { address(rln.verifier()) ); uint256 setSize = tempRln.SET_SIZE() - 1; - for (uint256 i = 0; i < setSize; i++) { - tempRln.register{value: MEMBERSHIP_DEPOSIT}(idCommitmentSeed + i); + for (uint256 i = 1; i <= setSize; i++) { + tempRln.register{value: MEMBERSHIP_DEPOSIT}(i); } assertEq(tempRln.idCommitmentIndex(), 4); vm.expectRevert(FullTree.selector); - tempRln.register{value: MEMBERSHIP_DEPOSIT}(idCommitmentSeed + setSize); + tempRln.register{value: MEMBERSHIP_DEPOSIT}(setSize + 1); } function test__ValidSlash(uint256 idCommitment, address payable to) public { @@ -79,6 +81,7 @@ contract RlnTest is Test { assumePayable(to); assumeNotPrecompile(to); vm.assume(to != address(0)); + vm.assume(rln.isValidCommitment(idCommitment)); rln.register{value: MEMBERSHIP_DEPOSIT}(idCommitment); assertEq(rln.stakedAmounts(idCommitment), MEMBERSHIP_DEPOSIT); @@ -110,6 +113,7 @@ contract RlnTest is Test { } function test__InvalidSlash__InvalidIdCommitment(uint256 idCommitment) public { + vm.assume(rln.isValidCommitment(idCommitment)); vm.expectRevert(abi.encodeWithSelector(MemberNotRegistered.selector, idCommitment)); rln.slash(idCommitment, payable(address(this)), zeroedProof); } @@ -120,6 +124,7 @@ contract RlnTest is Test { assumePayable(to); assumeNotPrecompile(to); vm.assume(to != address(0)); + vm.assume(rln.isValidCommitment(idCommitment)); rln.register{value: MEMBERSHIP_DEPOSIT}(idCommitment); assertEq(rln.stakedAmounts(idCommitment), MEMBERSHIP_DEPOSIT);