Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: constraint on bn254 values #26

Merged
merged 3 commits into from
Aug 16, 2023
Merged

fix: constraint on bn254 values #26

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cca45ef
fix: constraint on bn254 values
rymnc Aug 9, 2023
4cc2a27
fix: test sig
rymnc Aug 16, 2023
077be32
fix: test invalid id commitment
rymnc Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contracts/PoseidonHasher.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
26 changes: 21 additions & 5 deletions contracts/RlnBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);

Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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
Expand Down
22 changes: 21 additions & 1 deletion docs/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,14 @@ error FailedValidation()

Failed validation on registration/slashing

## InvalidIdCommitment

```solidity
error InvalidIdCommitment(uint256 idCommitment)
```

Invalid idCommitment

## InvalidReceiverAddress

```solidity
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion test/RLNApp.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import "forge-std/console.sol";

contract RlnApp is RlnBase {
uint256 public constant allowedIdCommitment =
21888242871839275222246405745257275088548364400416034343698204186575808495617;
19014214495641488759237505126948346942972912379615652741039992445865937985820;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why exactly has this been changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it was earlier the order of the field, i.e p = 21888242871839275222246405745257275088548364400416034343698204186575808495617, and no number in the field can exceed it. switched it out to make sure people dont get confused about it


uint256 private membershipDeposit = 1000000000000000;
uint256 private depth = 20;
Expand Down Expand Up @@ -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);
}
Expand Down
15 changes: 10 additions & 5 deletions test/Rln.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -51,26 +53,26 @@ 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for me to understand: why did we move away from a fuzz test here? Has anything changed with regards to the seed that we can't/aren't relying on it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically we are not testing any logic here, we're just checking if an error is thrown based on the size of the tree - thats why we dont need to fuzz id commitments here

Rln tempRln = new Rln(
MEMBERSHIP_DEPOSIT,
2,
address(rln.poseidonHasher()),
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 {
Expand All @@ -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);
Expand Down Expand Up @@ -110,6 +113,7 @@ contract RlnTest is Test {
}

function test__InvalidSlash__InvalidIdCommitment(uint256 idCommitment) public {
vm.assume(rln.isValidCommitment(idCommitment));
0x-r4bbit marked this conversation as resolved.
Show resolved Hide resolved
vm.expectRevert(abi.encodeWithSelector(MemberNotRegistered.selector, idCommitment));
rln.slash(idCommitment, payable(address(this)), zeroedProof);
}
Expand All @@ -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);
Expand Down