diff --git a/README.md b/README.md index df78c6c..ff52659 100644 --- a/README.md +++ b/README.md @@ -11,10 +11,10 @@ > [Hardhat](https://github.com/worldcoin/world-id-starter-hardhat) starter kits. This repository contains the underlying contracts that make World ID work, powered by the -[Semaphore library](https://semaphore.pse.dev/). These contracts are responsible for -performing identity operations on chain, and attestation of identities for the purposes of semaphore -proofs. Check out [user-flows.md](./docs/user-flows.md) for more information on how these contracts -relate to the rest of the World ID system. +[Semaphore library](https://semaphore.pse.dev/). These contracts are responsible for performing +identity operations on chain, and attestation of identities for the purposes of semaphore proofs. +Check out [user-flows.md](./docs/user-flows.md) for more information on how these contracts relate +to the rest of the World ID system. ## World ID Logo About World ID diff --git a/src/WorldIDIdentityManagerImplV1.sol b/src/WorldIDIdentityManagerImplV1.sol index 280a058..824e45c 100644 --- a/src/WorldIDIdentityManagerImplV1.sol +++ b/src/WorldIDIdentityManagerImplV1.sol @@ -327,22 +327,18 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { /// coordinate for `bs`, and elements 4 and 5 are the `y` coordinate for `bs`. Elements 6 /// and 7 are the `x` and `y` coordinates for `krs`. /// @param preRoot The value for the root of the tree before the `identityCommitments` have been - //// inserted. Must be an element of the field `Kr`. + //// inserted. Must be an element of the field `Kr`. (already in reduced form) /// @param startIndex The position in the tree at which the insertions were made. /// @param identityCommitments The identities that were inserted into the tree starting at /// `startIndex` and `preRoot` to give `postRoot`. All of the commitments must be /// elements of the field `Kr`. /// @param postRoot The root obtained after inserting all of `identityCommitments` into the tree - /// described by `preRoot`. Must be an element of the field `Kr`. + /// described by `preRoot`. Must be an element of the field `Kr`. (alread in reduced form) /// /// @custom:reverts Unauthorized If the message sender is not authorised to add identities. - /// @custom:reverts InvalidCommitment If one or more of the provided commitments is invalid. /// @custom:reverts NotLatestRoot If the provided `preRoot` is not the latest root. /// @custom:reverts ProofValidationFailure If `insertionProof` cannot be verified using the /// provided inputs. - /// @custom:reverts UnreducedElement If any of the `preRoot`, `postRoot` and - /// `identityCommitments` is not an element of the field `Kr`. It describes the - /// type and value of the unreduced element. /// @custom:reverts VerifierLookupTable.NoSuchVerifier If the batch sizes doesn't match a known /// verifier. /// @custom:reverts VerifierLookupTable.BatchTooLarge If the batch size exceeds the maximum @@ -354,27 +350,10 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { uint256[] calldata identityCommitments, uint256 postRoot ) public virtual onlyProxy onlyInitialized onlyIdentityOperator { - // We can only operate on the latest root in reduced form. - if (preRoot >= SNARK_SCALAR_FIELD) { - revert UnreducedElement(UnreducedElementType.PreRoot, preRoot); - } if (preRoot != _latestRoot) { revert NotLatestRoot(preRoot, _latestRoot); } - // As the `startIndex` is restricted to a uint32, where - // `type(uint32).max <<< SNARK_SCALAR_FIELD`, we are safe not to check this. As verified in - // the tests, a revert happens if you pass a value larger than `type(uint32).max` when - // calling outside the type-checker's protection. - - // We need the post root to be in reduced form. - if (postRoot >= SNARK_SCALAR_FIELD) { - revert UnreducedElement(UnreducedElementType.PostRoot, postRoot); - } - - // We can only operate on identities that are valid and in reduced form. - validateIdentityCommitmentsForRegistration(identityCommitments); - // Having validated the preconditions we can now check the proof itself. bytes32 inputHash = calculateIdentityRegistrationInputHash( startIndex, preRoot, postRoot, identityCommitments @@ -382,7 +361,7 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { // No matter what, the inputs can result in a hash that is not an element of the scalar // field in which we're operating. We reduce it into the field before handing it to the - // verifier. + // verifier. All other elements are reduced in the circuit. uint256 reducedElement = uint256(inputHash) % SNARK_SCALAR_FIELD; // We need to look up the correct verifier before we can verify. @@ -464,19 +443,10 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { uint256[] calldata newIdentities, uint256 postRoot ) public virtual onlyProxy onlyInitialized onlyIdentityOperator { - // We can only operate on the latest root in reduced form. - if (preRoot >= SNARK_SCALAR_FIELD) { - revert UnreducedElement(UnreducedElementType.PreRoot, preRoot); - } if (preRoot != _latestRoot) { revert NotLatestRoot(preRoot, _latestRoot); } - // We also need the post root to be in reduced form. - if (postRoot >= SNARK_SCALAR_FIELD) { - revert UnreducedElement(UnreducedElementType.PostRoot, postRoot); - } - // We also need the arrays to be of the same length. if ( leafIndices.length != oldIdentities.length || leafIndices.length != newIdentities.length @@ -484,16 +454,14 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { revert MismatchedInputLengths(); } - // We only operate on identities that are in reduced form. - validateIdentitiesForUpdate(oldIdentities, newIdentities); - // With valid preconditions we can calculate the input to the proof. bytes32 inputHash = calculateIdentityUpdateInputHash( preRoot, postRoot, leafIndices, oldIdentities, newIdentities ); - // No matter what, the input hashing process can result in a hash that is not an element of - // the field Fr. We reduce it into the field to give it safely to the verifier. + // No matter what, the inputs can result in a hash that is not an element of the scalar + // field in which we're operating. We reduce it into the field before handing it to the + // verifier. All other elements are reduced in the circuit. uint256 reducedInputHash = uint256(inputHash) % SNARK_SCALAR_FIELD; // We have to look up the correct verifier before we can verify. @@ -674,126 +642,6 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { } } - /// @notice Validates an array of identity commitments, reverting if it finds one that is - /// invalid or has not been reduced. - /// @dev Identities are not valid if an identity is a non-zero element that occurs after a zero - /// element in the array. - /// - /// @param identityCommitments The array of identity commitments to be validated. - /// - /// @custom:reverts Reverts with `InvalidCommitment` if one or more of the provided commitments - /// is invalid. - /// @custom:reverts Reverts with `UnreducedElement` if one or more of the provided commitments - /// is not in reduced form. - function validateIdentityCommitmentsForRegistration(uint256[] calldata identityCommitments) - internal - view - virtual - { - uint256 offset; - uint256 max; - assembly ("memory-safe") { - // offset of the first element in the array - offset := identityCommitments.offset - // offset one byte after the end of argument section - max := add(offset, shl(5, identityCommitments.length)) - - // increment offset until either of the following happens: - // - offset is equal to max, meaning we've reached the end of the array - // - the element at offset is zero - // - the element at offset is greater than or equal to SNARK_SCALAR_FIELD - // The latter two conditions are combined into a single check of the form - // `element - 1 < SNARK_SCALAR_FIELD - 1`. This abuses unsigned comparisons: - // if `element` is zero, it will underflow and be greater than SNARK_SCALAR_FIELD - 1. - // If `element` is non-zero, we can cancel the -1 terms and the comparison becomes what - // we want: `element < SNARK_SCALAR_FIELD`. - for {} and( - lt(offset, max), lt(sub(calldataload(offset), 1), SNARK_SCALAR_FIELD_MIN_ONE) - ) { offset := add(offset, 32) } {} - } - - // check if the loop terminated before end of array and handle the remaining elements - if (offset < max) { - uint256 index = identityCommitments.length - ((max - offset) >> 5); - uint256 element = identityCommitments[index]; - - // Check if the loop terminated because it found a zero - // this means we need to finish looping and make sure all remaining elements are zero. - if (element == 0) { - assembly ("memory-safe") { - // we just confirmed this element is zero - offset := add(offset, 32) - - // increment offset until either of the following happens: - // - offset is equal to max, meaning we've reached the end of the array - // - the element at offset is non-zero - for {} and(lt(offset, max), iszero(calldataload(offset))) { - offset := add(offset, 32) - } {} - } - - // check if the loop terminated because it found a non-zero element - // if yes, revert. - if (offset < max) { - index = identityCommitments.length - ((max - offset) >> 5); - revert InvalidCommitment(index); - } - } - // otherwise check if the loop terminated because it found an unreduced element - // if so, revert - else if (element >= SNARK_SCALAR_FIELD) { - revert UnreducedElement(UnreducedElementType.IdentityCommitment, element); - } - } - } - - /// @notice Validates that an array of identity commitments is within bounds of the SNARK_SCALAR_FIELD - /// @param identityCommitments The array of identity commitments to be validated. - function validateArrayIsInReducedForm(uint256[] calldata identityCommitments) - internal - view - virtual - { - uint256 offset; - uint256 max; - assembly ("memory-safe") { - // offset of the first element in the array - offset := identityCommitments.offset - // offset one byte after the end of argument section - max := add(offset, shl(5, identityCommitments.length)) - - // increment offset until either of the following happens: - // - offset is equal to max, meaning we've reached the end of the array - // - the element at offset is greater than or equal to SNARK_SCALAR_FIELD - for {} and(lt(offset, max), lt(calldataload(offset), SNARK_SCALAR_FIELD)) { - offset := add(offset, 32) - } {} - } - // check if the loop terminated before end of array and revert if so - if (offset < max) { - uint256 index = identityCommitments.length - ((max - offset) >> 5); - uint256 element = identityCommitments[index]; - revert UnreducedElement(UnreducedElementType.IdentityCommitment, element); - } - } - - /// @notice Validates the array of identities for each of the old and new commitments being in - /// reduced form. - /// @dev Must be called with arrays of the same length. - /// - /// @param oldIdentities The array of old values for the identities. - /// @param newIdentities The array of new values for the identities. - /// - /// @custom:reverts UnreducedElement If one or more of the provided commitments is not in - //// reduced form. - function validateIdentitiesForUpdate( - uint256[] calldata oldIdentities, - uint256[] calldata newIdentities - ) internal view virtual { - validateArrayIsInReducedForm(oldIdentities); - validateArrayIsInReducedForm(newIdentities); - } - /// @notice Reverts if the provided root value is not valid. /// @dev A root is valid if it is either the latest root, or not the latest root but has not /// expired. diff --git a/src/WorldIDIdentityManagerImplV2.sol b/src/WorldIDIdentityManagerImplV2.sol index e39ac10..c8cc0c5 100644 --- a/src/WorldIDIdentityManagerImplV2.sol +++ b/src/WorldIDIdentityManagerImplV2.sol @@ -104,26 +104,17 @@ contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { uint256 preRoot, uint256 postRoot ) public virtual onlyProxy onlyInitialized onlyIdentityOperator { - // We can only operate on the latest root in reduced form. - if (preRoot >= SNARK_SCALAR_FIELD) { - revert UnreducedElement(UnreducedElementType.PreRoot, preRoot); - } if (preRoot != _latestRoot) { revert NotLatestRoot(preRoot, _latestRoot); } - // We need the post root to be in reduced form. - if (postRoot >= SNARK_SCALAR_FIELD) { - revert UnreducedElement(UnreducedElementType.PostRoot, postRoot); - } - // Having validated the preconditions we can now check the proof itself. bytes32 inputHash = calculateIdentityDeletionInputHash(packedDeletionIndices, preRoot, postRoot, batchSize); // No matter what, the inputs can result in a hash that is not an element of the scalar // field in which we're operating. We reduce it into the field before handing it to the - // verifier. + // verifier. All other elements are reduced in the circuit. uint256 reducedElement = uint256(inputHash) % SNARK_SCALAR_FIELD; // We need to look up the correct verifier before we can verify. diff --git a/src/test/identity-manager/WorldIDIdentityManagerIdentityDeletion.t.sol b/src/test/identity-manager/WorldIDIdentityManagerIdentityDeletion.t.sol index 984695d..be44353 100644 --- a/src/test/identity-manager/WorldIDIdentityManagerIdentityDeletion.t.sol +++ b/src/test/identity-manager/WorldIDIdentityManagerIdentityDeletion.t.sol @@ -334,44 +334,6 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { assertCallFailsOn(identityManagerAddress, callData, expectedError); } - /// @notice Tests that it reverts if an attempt is made to delete identities with a pre - /// root that is not in reduced form. - function testCannotDeleteIdentitiesWithUnreducedPreRoot(uint128 i) public { - // Setup - uint256 newPreRoot = SNARK_SCALAR_FIELD + i; - bytes memory callData = abi.encodeCall( - ManagerImpl.deleteIdentities, - (deletionProof, deletionBatchSize, packedDeletionIndices, newPreRoot, deletionPostRoot) - ); - bytes memory expectedError = abi.encodeWithSelector( - ManagerImplV1.UnreducedElement.selector, - ManagerImplV1.UnreducedElementType.PreRoot, - newPreRoot - ); - - // Test - assertCallFailsOn(identityManagerAddress, callData, expectedError); - } - - /// @notice Tests that it reverts if an attempt is made to delete identities with a deletionPostRoot - /// that is not in reduced form. - function testCannotDeleteIdentitiesWithUnreducedPostRoot(uint128 i) public { - // Setup - uint256 newPostRoot = SNARK_SCALAR_FIELD + i; - bytes memory callData = abi.encodeCall( - ManagerImpl.deleteIdentities, - (deletionProof, deletionBatchSize, packedDeletionIndices, initialRoot, newPostRoot) - ); - bytes memory expectedError = abi.encodeWithSelector( - ManagerImplV1.UnreducedElement.selector, - ManagerImplV1.UnreducedElementType.PostRoot, - newPostRoot - ); - - // Test - assertCallFailsOn(identityManagerAddress, callData, expectedError); - } - /// @notice Tests that identities can only be deleted through the proxy. function testCannotDelteIdentitiesIfNotViaProxy() public { // Setup diff --git a/src/test/identity-manager/WorldIDIdentityManagerIdentityRegistration.t.sol b/src/test/identity-manager/WorldIDIdentityManagerIdentityRegistration.t.sol index 679d769..ce7cfe4 100644 --- a/src/test/identity-manager/WorldIDIdentityManagerIdentityRegistration.t.sol +++ b/src/test/identity-manager/WorldIDIdentityManagerIdentityRegistration.t.sol @@ -405,34 +405,6 @@ contract WorldIDIdentityManagerIdentityRegistration is WorldIDIdentityManagerTes assertCallFailsOn(identityManagerAddress, callData, expectedError); } - /// @notice Tests that it reverts if an attempt is made to register identity commitments - /// containing an invalid identity. - function testCannotRegisterIdentitiesWithInvalidIdentities( - uint8 identitiesLength, - uint8 invalidPosition - ) public { - // Setup - vm.assume(identitiesLength != 0); - vm.assume(invalidPosition < (identitiesLength - 1)); - uint256[] memory invalidCommitments = new uint256[](identitiesLength); - - for (uint256 i = 0; i < identitiesLength; ++i) { - invalidCommitments[i] = i + 1; - } - invalidCommitments[invalidPosition] = 0x0; - - bytes memory callData = abi.encodeCall( - ManagerImplV1.registerIdentities, - (insertionProof, initialRoot, startIndex, invalidCommitments, insertionPostRoot) - ); - bytes memory expectedError = abi.encodeWithSelector( - ManagerImplV1.InvalidCommitment.selector, uint256(invalidPosition + 1) - ); - - // Test - assertCallFailsOn(identityManagerAddress, callData, expectedError); - } - /// @notice Tests that runs of zeroes are accepted by the `registerIdentities` function as valid /// arrays of identity commitments. function testRegisterIdentitiesWithRunsOfZeroes(uint8 identitiesLength, uint8 zeroPosition) @@ -478,86 +450,6 @@ contract WorldIDIdentityManagerIdentityRegistration is WorldIDIdentityManagerTes assertCallSucceedsOn(identityManagerAddress, callData, new bytes(0)); } - /// @notice Tests that it reverts if an attempt is made to register identity commitments that - /// are not in reduced form. - function testCannotRegisterIdentitiesWithUnreducedIdentities(uint128 i) public { - // Setup - uint256 position = rotateSlot(); - uint256[] memory unreducedCommitments = new uint256[](identityCommitments.length); - unreducedCommitments[position] = SNARK_SCALAR_FIELD + i; - bytes memory callData = abi.encodeCall( - ManagerImplV1.registerIdentities, - (insertionProof, initialRoot, startIndex, unreducedCommitments, insertionPostRoot) - ); - bytes memory expectedError = abi.encodeWithSelector( - ManagerImplV1.UnreducedElement.selector, - ManagerImplV1.UnreducedElementType.IdentityCommitment, - SNARK_SCALAR_FIELD + i - ); - - // Test - assertCallFailsOn(identityManagerAddress, callData, expectedError); - } - - /// @notice Tests that it reverts if an attempt is made to register new identities with a pre - /// root that is not in reduced form. - function testCannotRegisterIdentitiesWithUnreducedPreRoot(uint128 i) public { - // Setup - uint256 newPreRoot = SNARK_SCALAR_FIELD + i; - bytes memory callData = abi.encodeCall( - ManagerImplV1.registerIdentities, - (insertionProof, newPreRoot, startIndex, identityCommitments, insertionPostRoot) - ); - bytes memory expectedError = abi.encodeWithSelector( - ManagerImplV1.UnreducedElement.selector, - ManagerImplV1.UnreducedElementType.PreRoot, - newPreRoot - ); - - // Test - assertCallFailsOn(identityManagerAddress, callData, expectedError); - } - - /// @notice Tests that it reverts if an attempt is made to register identities with a insertionPostRoot - /// that is not in reduced form. - function testCannotRegisterIdentitiesWithUnreducedPostRoot(uint128 i) public { - // Setup - uint256 newPostRoot = SNARK_SCALAR_FIELD + i; - bytes memory callData = abi.encodeCall( - ManagerImplV1.registerIdentities, - (insertionProof, initialRoot, startIndex, identityCommitments, newPostRoot) - ); - bytes memory expectedError = abi.encodeWithSelector( - ManagerImplV1.UnreducedElement.selector, - ManagerImplV1.UnreducedElementType.PostRoot, - newPostRoot - ); - - // Test - assertCallFailsOn(identityManagerAddress, callData, expectedError); - } - - /// @notice Tests that it reverts if an attempt is made to violate type safety and register with - /// a startIndex that is not type safe within the bounds of `type(uint32).max` and hence - /// within `SNARK_SCALAR_FIELD`. - function testCannotRegisterIdentitiesWithUnreducedStartIndex(uint256 i) public { - // Setup - vm.assume(i > type(uint32).max); - bytes4 functionSelector = ManagerImplV1.registerIdentities.selector; - // Have to encode with selector as otherwise it's typechecked. - bytes memory callData = abi.encodeWithSelector( - functionSelector, - insertionProof, - insertionPreRoot, - i, - identityCommitments, - insertionPostRoot - ); - - // Test - assertCallFailsOn(identityManagerAddress, callData); - } - /// @notice Tests that identities can only be registered through the proxy. function testCannotRegisterIdentitiesIfNotViaProxy() public { // Setup diff --git a/src/test/identity-manager/WorldIDIdentityManagerIdentityUpdate.t.sol b/src/test/identity-manager/WorldIDIdentityManagerIdentityUpdate.t.sol index 2a7c9ca..65b185b 100644 --- a/src/test/identity-manager/WorldIDIdentityManagerIdentityUpdate.t.sol +++ b/src/test/identity-manager/WorldIDIdentityManagerIdentityUpdate.t.sol @@ -340,120 +340,6 @@ contract WorldIDIdentityManagerIdentityUpdate is WorldIDIdentityManagerTest { assertCallFailsOn(identityManagerAddress, callData, expectedError); } - /// @notice Tests that it reverts if an attempt is made to update identity commitments that - /// are not in reduced form. - function testCannotRegisterIdentitiesWithUnreducedIdentities( - uint128 i, - uint256 position, - uint128 newPreRoot, - uint128[] memory identities, - uint128[8] memory prf, - bool changeOld - ) public { - // Setup - vm.assume(position < identities.length); - vm.assume(identities.length <= 1000); - ( - uint32[] memory leafIndices, - uint256[] memory oldIdents, - uint256[] memory newIdents, - uint256[8] memory actualProof - ) = prepareUpdateIdentitiesTestCase(identities, prf); - // scoping to avoid stack too deep errors - { - ( - VerifierLookupTable insertVerifiers, - VerifierLookupTable deletionVerifiers, - VerifierLookupTable updateVerifiers - ) = makeVerifierLookupTables(TC.makeDynArray([identities.length])); - makeNewIdentityManager( - treeDepth, - newPreRoot, - insertVerifiers, - deletionVerifiers, - updateVerifiers, - semaphoreVerifier - ); - } - - if (changeOld) { - oldIdents[position] = SNARK_SCALAR_FIELD + i; - } else { - newIdents[position] = SNARK_SCALAR_FIELD + i; - } - bytes memory callData = abi.encodeCall( - ManagerImpl.updateIdentities, - (actualProof, newPreRoot, leafIndices, oldIdents, newIdents, insertionPostRoot) - ); - - bytes memory expectedError = abi.encodeWithSelector( - ManagerImpl.UnreducedElement.selector, - ManagerImpl.UnreducedElementType.IdentityCommitment, - SNARK_SCALAR_FIELD + i - ); - - // Test - assertCallFailsOn(identityManagerAddress, callData, expectedError); - } - - /// @notice Tests that it reverts if an attempt is made to update identities with a pre root - /// that is not in reduced form. - function testCannotUpdateIdentitiesWithUnreducedPreRoot( - uint128 i, - uint128[] memory identities, - uint128[8] memory prf - ) public { - // Setup - uint256 newPreRoot = SNARK_SCALAR_FIELD + i; - ( - uint32[] memory leafIndices, - uint256[] memory oldIdents, - uint256[] memory newIdents, - uint256[8] memory actualProof - ) = prepareUpdateIdentitiesTestCase(identities, prf); - bytes memory callData = abi.encodeCall( - ManagerImpl.updateIdentities, - (actualProof, newPreRoot, leafIndices, oldIdents, newIdents, insertionPostRoot) - ); - bytes memory expectedError = abi.encodeWithSelector( - ManagerImpl.UnreducedElement.selector, - ManagerImpl.UnreducedElementType.PreRoot, - newPreRoot - ); - - // Test - assertCallFailsOn(identityManagerAddress, callData, expectedError); - } - - /// @notice Tests that it reverts if an attempt is made to update identities with a insertionPostRoot - /// that is not in reduced form. - function testCannotUpdateIdentitiesWithUnreducedPostRoot( - uint128 i, - uint128[] memory identities, - uint128[8] memory prf - ) public { - // Setup - uint256 newPostRoot = SNARK_SCALAR_FIELD + i; - ( - uint32[] memory leafIndices, - uint256[] memory oldIdents, - uint256[] memory newIdents, - uint256[8] memory actualProof - ) = prepareUpdateIdentitiesTestCase(identities, prf); - bytes memory callData = abi.encodeCall( - ManagerImpl.updateIdentities, - (actualProof, initialRoot, leafIndices, oldIdents, newIdents, newPostRoot) - ); - bytes memory expectedError = abi.encodeWithSelector( - ManagerImpl.UnreducedElement.selector, - ManagerImpl.UnreducedElementType.PostRoot, - newPostRoot - ); - - // Test - assertCallFailsOn(identityManagerAddress, callData, expectedError); - } - /// @notice Tests that identities can only be updated through the proxy. function testCannotUpdateIdentitiesIfNotViaProxy( uint128[] memory identities,