From 121cffc7a64308108966328611331b4081a37fc5 Mon Sep 17 00:00:00 2001 From: "dcbuilder.eth" Date: Wed, 11 Oct 2023 16:41:14 +0100 Subject: [PATCH] PRO-343 Implement audit suggestions (#134) * fix: Invalid batchSize can result in incorrect hash calculation * fix natspec documentation mismatch * fix natpsec enum docs * fix: Missing input validation in some functions * fix: Missing input validation in some functions in V2 * fix: Absence of event emission during contract initialization * add sanity check and revert natspec * fix tests * make format --- src/WorldIDIdentityManagerImplV1.sol | 30 +++++++++- src/WorldIDIdentityManagerImplV2.sol | 52 ++++++++++++----- ...rldIDIdentityManagerIdentityDeletion.t.sol | 58 +++++++------------ ...WorldIDIdentityManagerInitialization.t.sol | 5 ++ .../WorldIDIdentityManagerTest.sol | 4 ++ .../WorldIDIdentityManagerUninit.t.sol | 8 +-- 6 files changed, 98 insertions(+), 59 deletions(-) diff --git a/src/WorldIDIdentityManagerImplV1.sol b/src/WorldIDIdentityManagerImplV1.sol index 5c69a23..bf03cc4 100644 --- a/src/WorldIDIdentityManagerImplV1.sol +++ b/src/WorldIDIdentityManagerImplV1.sol @@ -125,7 +125,6 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { } /// @notice Represents the kind of change that is made to the root of the tree. - /// @dev TreeChange.Update preserved for ABI backwards compatibility with V1, no longer used enum TreeChange { Insertion, Deletion @@ -210,6 +209,12 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { /// @dev preserved for ABI backwards compatibility with V1, no longer used error MismatchedInputLengths(); + /// @notice Thrown when a verifier is initialized to be the zero address + error InvalidVerifier(); + + /// @notice Thrown when a verifier lookup table is initialized to be the zero address + error InvalidVerifierLUT(); + /////////////////////////////////////////////////////////////////////////////// /// EVENTS /// /////////////////////////////////////////////////////////////////////////////// @@ -287,6 +292,8 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { /// /// @custom:reverts string If called more than once at the same initialisation number. /// @custom:reverts UnsupportedTreeDepth If passed tree depth is not among defined values. + /// @custom:reverts InvalidVerifierLUT if `_batchInsertionVerifiers` or `_batchUpdateVerifiers` is set to the zero address + /// @custom:reverts InvalidVerifier if `_semaphoreVerifier` is set to the zero address function initialize( uint8 _treeDepth, uint256 initialRoot, @@ -294,6 +301,17 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { VerifierLookupTable _batchUpdateVerifiers, ISemaphoreVerifier _semaphoreVerifier ) public reinitializer(1) { + if (address(_batchInsertionVerifiers) == address(0)) { + revert InvalidVerifierLUT(); + } + + if (address(_batchUpdateVerifiers) == address(0)) { + revert InvalidVerifierLUT(); + } + + if (address(_semaphoreVerifier) == address(0)) { + revert InvalidVerifier(); + } // First, ensure that all of the parent contracts are initialised. __delegateInit(); @@ -515,6 +533,7 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { /// /// @param newTable The new verifier lookup table to be used for verifying identity /// registrations. + /// @custom:reverts InvalidVerifierLUT if `newTable` is set to the zero address function setRegisterIdentitiesVerifierLookupTable(VerifierLookupTable newTable) public virtual @@ -522,6 +541,10 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { onlyInitialized onlyOwner { + if (address(newTable) == address(0)) { + revert InvalidVerifierLUT(); + } + VerifierLookupTable oldTable = batchInsertionVerifiers; batchInsertionVerifiers = newTable; emit DependencyUpdated( @@ -548,6 +571,7 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { /// @dev Only the owner of the contract can call this function. /// /// @param newVerifier The new verifier instance to be used for verifying semaphore proofs. + /// @custom:reverts InvalidVerifier if `newVerifier` is set to the zero address function setSemaphoreVerifier(ISemaphoreVerifier newVerifier) public virtual @@ -555,6 +579,10 @@ contract WorldIDIdentityManagerImplV1 is WorldIDImpl, IWorldID { onlyInitialized onlyOwner { + if (address(newVerifier) == address(0)) { + revert InvalidVerifier(); + } + ISemaphoreVerifier oldVerifier = semaphoreVerifier; semaphoreVerifier = newVerifier; emit DependencyUpdated( diff --git a/src/WorldIDIdentityManagerImplV2.sol b/src/WorldIDIdentityManagerImplV2.sol index 2064b2d..79cd374 100644 --- a/src/WorldIDIdentityManagerImplV2.sol +++ b/src/WorldIDIdentityManagerImplV2.sol @@ -6,7 +6,7 @@ import "./WorldIDIdentityManagerImplV1.sol"; /// @author Worldcoin /// @notice An implementation of a batch-based identity manager for the WorldID protocol. /// @dev The manager is based on the principle of verifying externally-created Zero Knowledge Proofs -/// to perform the insertions. +/// to perform the deletions. /// @dev This is the implementation delegated to by a proxy. contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { /////////////////////////////////////////////////////////////////////////////// @@ -50,8 +50,15 @@ contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { /// @notice The table of verifiers for verifying batch identity deletions. VerifierLookupTable internal batchDeletionVerifiers; + /// @notice Thrown when the WorldIDIdentityManagerImplV2 contract is initalized + event WorldIDIdentityManagerImplV2Initialized(); + + /// @notice Thrown when the bytes calldata packedDeletionIndices array + /// is not a multiple of 4 (to make up 32 bit indices) + error InvalidDeletionIndices(); + /// @notice Initializes the V2 implementation contract. - /// @param _batchUpdateVerifiers The table of verifiers for verifying batch identity deletions. + /// @param _batchDeletionVerifiers The table of verifiers for verifying batch identity deletions. /// @dev Must be called exactly once /// @dev This is marked `reinitializer()` to allow for updated initialisation steps when working /// with upgrades based upon this contract. Be aware that there are only 256 (zero-indexed) @@ -61,8 +68,15 @@ contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { /// upgrading. Create a separate initializer function instead. /// /// - function initializeV2(VerifierLookupTable _batchUpdateVerifiers) public reinitializer(2) { - batchDeletionVerifiers = _batchUpdateVerifiers; + /// @custom:reverts InvalidVerifierLUT if `_batchDeletionVerifiers` is set to the zero address + function initializeV2(VerifierLookupTable _batchDeletionVerifiers) public reinitializer(2) { + if (address(_batchDeletionVerifiers) == address(0)) { + revert InvalidVerifierLUT(); + } + + batchDeletionVerifiers = _batchDeletionVerifiers; + + emit WorldIDIdentityManagerImplV2Initialized(); } /////////////////////////////////////////////////////////////////// @@ -70,7 +84,7 @@ contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { /////////////////////////////////////////////////////////////////// /// @notice Deletes identities from the WorldID system. - /// @dev Can only be called by the owner. + /// @dev Can only be called by the identity operator. /// @dev Deletion is performed off-chain and verified on-chain via the `deletionProof`. /// This saves gas and time over deleting identities one at a time. /// @@ -79,10 +93,10 @@ contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { /// coordinates for `ar` respectively. Elements 2 and 3 are the `x` 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 batchSize The number of identities that are to be deleted in the current batch. - /// @param packedDeletionIndices The indices of the identities that were deleted from the tree. - /// @param preRoot The value for the root of the tree before the `identityCommitments` have been - /// inserted. Must be an element of the field `Kr`. + /// @param packedDeletionIndices The indices of the identities that were deleted from the tree. The batch size is inferred from the length of this + //// array: batchSize = packedDeletionIndices / 4 + /// @param preRoot The value for the root of the tree before the corresponding identity commitments have + /// been deleted. Must be an element of the field `Kr`. /// @param postRoot The root obtained after deleting all of `identityCommitments` into the tree /// described by `preRoot`. Must be an element of the field `Kr`. /// @@ -92,13 +106,20 @@ contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { /// provided inputs. /// @custom:reverts VerifierLookupTable.NoSuchVerifier If the batch sizes doesn't match a known /// verifier. + /// @custom:reverts InvalidDeletionIndices if the length of `packedDeletionIndices` + /// is not a multiple of 4 (8*4 = 32 bits per index) function deleteIdentities( uint256[8] calldata deletionProof, - uint32 batchSize, bytes calldata packedDeletionIndices, uint256 preRoot, uint256 postRoot ) public virtual onlyProxy onlyInitialized onlyIdentityOperator { + if (packedDeletionIndices.length % 4 != 0) { + revert InvalidDeletionIndices(); + } + + uint32 batchSize = uint32(packedDeletionIndices.length / 4); + if (preRoot != _latestRoot) { revert NotLatestRoot(preRoot, _latestRoot); } @@ -118,7 +139,7 @@ contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { // With that, we can properly try and verify. try deletionVerifier.verifyProof(deletionProof, [reducedElement]) { // If it did verify, we need to update the contract's state. We set the currently valid - // root to the root after the insertions. + // root to the root after the deletions. _latestRoot = postRoot; // We also need to add the previous root to the history, and set the timestamp at @@ -158,6 +179,7 @@ contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { /// /// @param newTable The new verifier lookup table to be used for verifying identity /// deletions. + /// @custom:reverts InvalidVerifierLUT if `newTable` is set to the zero address function setDeleteIdentitiesVerifierLookupTable(VerifierLookupTable newTable) public virtual @@ -165,6 +187,10 @@ contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { onlyInitialized onlyOwner { + if (address(newTable) == address(0)) { + revert InvalidVerifierLUT(); + } + VerifierLookupTable oldTable = batchDeletionVerifiers; batchDeletionVerifiers = newTable; emit DependencyUpdated( @@ -180,8 +206,8 @@ contract WorldIDIdentityManagerImplV2 is WorldIDIdentityManagerImplV1 { /// @dev Implements the computation described below. /// /// @param packedDeletionIndices The indices of the identities that were deleted from the tree. - /// @param preRoot The root value of the tree before these insertions were made. - /// @param postRoot The root value of the tree after these insertions were made. + /// @param preRoot The root value of the tree before these deletions were made. + /// @param postRoot The root value of the tree after these deletions were made. /// @param batchSize The number of identities that were deleted in this batch /// /// @return hash The input hash calculated as described below. diff --git a/src/test/identity-manager/WorldIDIdentityManagerIdentityDeletion.t.sol b/src/test/identity-manager/WorldIDIdentityManagerIdentityDeletion.t.sol index 7e6f173..fcdd41c 100644 --- a/src/test/identity-manager/WorldIDIdentityManagerIdentityDeletion.t.sol +++ b/src/test/identity-manager/WorldIDIdentityManagerIdentityDeletion.t.sol @@ -51,13 +51,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { ); bytes memory deleteCallData = abi.encodeCall( ManagerImpl.deleteIdentities, - ( - deletionProof, - deletionBatchSize, - packedDeletionIndices, - deletionPreRoot, - deletionPostRoot - ) + (deletionProof, packedDeletionIndices, deletionPreRoot, deletionPostRoot) ); bytes memory latestRootCallData = abi.encodeCall(ManagerImplV1.latestRoot, ()); bytes memory queryRootCallData = abi.encodeCall(ManagerImplV1.queryRoot, (deletionPostRoot)); @@ -86,12 +80,13 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { vm.assume(SimpleVerify.isValidInput(uint256(prf[0]))); vm.assume(newPreRoot != newPostRoot); vm.assume(packedDeletionIndices.length <= 125); + vm.assume(packedDeletionIndices.length % 4 == 0); vm.assume(identityOperator != nullAddress && identityOperator != thisAddress); ( VerifierLookupTable insertVerifiers, VerifierLookupTable deletionVerifiers, VerifierLookupTable updateVerifiers - ) = makeVerifierLookupTables(TC.makeDynArray([packedDeletionIndices.length * 8])); + ) = makeVerifierLookupTables(TC.makeDynArray([packedDeletionIndices.length / 4])); makeNewIdentityManager( treeDepth, newPreRoot, @@ -103,13 +98,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { uint256[8] memory actualProof = prepareDeleteIdentitiesTestCase(prf); bytes memory callData = abi.encodeCall( ManagerImpl.deleteIdentities, - ( - actualProof, - uint32(packedDeletionIndices.length * 8), - packedDeletionIndices, - newPreRoot, - newPostRoot - ) + (actualProof, packedDeletionIndices, newPreRoot, newPostRoot) ); bytes memory setupCallData = @@ -131,21 +120,18 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { function testDeleteIdentitiesSelectsCorrectVerifier( uint128[8] memory prf, uint128 newPreRoot, - bytes calldata packedDeletionIndices, uint128 newPostRoot ) public { vm.assume(SimpleVerify.isValidInput(uint256(prf[0]))); vm.assume(newPreRoot != newPostRoot); - vm.assume(packedDeletionIndices.length <= 1000 && packedDeletionIndices.length > 0); bytes memory secondIndices = abi.encodePacked(uint32(0), uint32(2), uint32(4), uint32(6)); - uint32 secondIndicesLength = uint32(secondIndices.length); ( VerifierLookupTable insertVerifiers, VerifierLookupTable deletionVerifiers, VerifierLookupTable updateVerifiers - ) = makeVerifierLookupTables(TC.makeDynArray([deletionBatchSize, secondIndicesLength])); + ) = makeVerifierLookupTables(TC.makeDynArray([deletionBatchSize, secondIndices.length / 4])); makeNewIdentityManager( treeDepth, newPreRoot, @@ -157,12 +143,11 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { uint256[8] memory actualProof = prepareDeleteIdentitiesTestCase(prf); bytes memory firstCallData = abi.encodeCall( ManagerImpl.deleteIdentities, - (actualProof, deletionBatchSize, packedDeletionIndices, newPreRoot, newPostRoot) + (actualProof, packedDeletionIndices, newPreRoot, newPostRoot) ); uint256 secondPostRoot = uint256(newPostRoot) + 1; bytes memory secondCallData = abi.encodeCall( - ManagerImpl.deleteIdentities, - (actualProof, secondIndicesLength, secondIndices, newPostRoot, secondPostRoot) + ManagerImpl.deleteIdentities, (actualProof, secondIndices, newPostRoot, secondPostRoot) ); vm.expectEmit(true, true, true, true); @@ -172,7 +157,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { assertCallSucceedsOn(identityManagerAddress, firstCallData); vm.expectEmit(true, true, true, true); - emit VerifiedProof(uint256(secondIndicesLength)); + emit VerifiedProof(uint256(secondIndices.length / 4)); assertCallSucceedsOn(identityManagerAddress, secondCallData); } @@ -186,12 +171,15 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { ) public { vm.assume(SimpleVerify.isValidInput(uint256(prf[0]))); vm.assume(newPreRoot != newPostRoot); - vm.assume(packedDeletionIndices.length > 0); + vm.assume(packedDeletionIndices.length > 4); + vm.assume(packedDeletionIndices.length % 4 == 0); ( VerifierLookupTable insertVerifiers, VerifierLookupTable deletionVerifiers, VerifierLookupTable updateVerifiers - ) = makeVerifierLookupTables(TC.makeDynArray([deletionBatchSize - 1])); + ) = + // the -1 offsets the correct batch size by 1 thus causing the error + makeVerifierLookupTables(TC.makeDynArray([(packedDeletionIndices.length / 4) - 1])); makeNewIdentityManager( treeDepth, newPreRoot, @@ -204,7 +192,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { bytes memory callData = abi.encodeCall( ManagerImpl.deleteIdentities, - (actualProof, deletionBatchSize, packedDeletionIndices, newPreRoot, newPostRoot) + (actualProof, packedDeletionIndices, newPreRoot, newPostRoot) ); bytes memory errorData = abi.encodeWithSelector(VerifierLookupTable.NoSuchVerifier.selector); @@ -222,7 +210,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { vm.assume(!SimpleVerify.isValidInput(uint256(prf[0]))); vm.assume(newPreRoot != newPostRoot); ITreeVerifier actualVerifier = new TreeVerifier(); - uint32 indicesLength = uint32(packedDeletionIndices.length); + uint32 indicesLength = uint32(packedDeletionIndices.length / 4); ( VerifierLookupTable insertVerifiers, VerifierLookupTable deletionVerifiers, @@ -240,7 +228,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { uint256[8] memory actualProof = prepareDeleteIdentitiesTestCase(prf); bytes memory callData = abi.encodeCall( ManagerImpl.deleteIdentities, - (actualProof, indicesLength, packedDeletionIndices, newPreRoot, newPostRoot) + (actualProof, packedDeletionIndices, newPreRoot, newPostRoot) ); bytes memory expectedError = abi.encodeWithSelector(ManagerImplV1.ProofValidationFailure.selector); @@ -271,7 +259,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { bytes memory deletionCallData = abi.encodeCall( ManagerImpl.deleteIdentities, - (deletionProof, deletionBatchSize, packedDeletionIndices, deletionPreRoot, newPostRoot) + (deletionProof, packedDeletionIndices, deletionPreRoot, newPostRoot) ); bytes memory expectedError = abi.encodeWithSelector(ManagerImplV1.ProofValidationFailure.selector); @@ -287,13 +275,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { vm.assume(nonOperator != address(this) && nonOperator != address(0x0)); bytes memory callData = abi.encodeCall( ManagerImpl.deleteIdentities, - ( - deletionProof, - deletionBatchSize, - packedDeletionIndices, - deletionPreRoot, - deletionPostRoot - ) + (deletionProof, packedDeletionIndices, deletionPreRoot, deletionPostRoot) ); bytes memory errorData = abi.encodeWithSelector(ManagerImplV1.Unauthorized.selector, nonOperator); @@ -323,7 +305,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { ); bytes memory callData = abi.encodeCall( ManagerImpl.deleteIdentities, - (deletionProof, deletionBatchSize, packedDeletionIndices, actualRoot, deletionPostRoot) + (deletionProof, packedDeletionIndices, actualRoot, deletionPostRoot) ); bytes memory expectedError = abi.encodeWithSelector( ManagerImplV1.NotLatestRoot.selector, actualRoot, uint256(currentPreRoot) @@ -342,7 +324,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest { // Test managerImpl.deleteIdentities( - deletionProof, deletionBatchSize, packedDeletionIndices, initialRoot, deletionPostRoot + deletionProof, packedDeletionIndices, initialRoot, deletionPostRoot ); } } diff --git a/src/test/identity-manager/WorldIDIdentityManagerInitialization.t.sol b/src/test/identity-manager/WorldIDIdentityManagerInitialization.t.sol index eb25832..675ad10 100644 --- a/src/test/identity-manager/WorldIDIdentityManagerInitialization.t.sol +++ b/src/test/identity-manager/WorldIDIdentityManagerInitialization.t.sol @@ -18,6 +18,8 @@ contract WorldIDIdentityManagerInitialization is WorldIDIdentityManagerTest { /// @notice Taken from Initializable.sol event Initialized(uint8 version); + event WorldIDIdentityManagerImplV2Initialized(); + /// @notice Checks that it is possible to initialise the contract. function testInitialisation() public { // Setup @@ -55,6 +57,9 @@ contract WorldIDIdentityManagerInitialization is WorldIDIdentityManagerTest { UUPSUpgradeable.upgradeToAndCall, (address(managerImplAddress), initCallV2) ); + vm.expectEmit(true, true, true, true); + emit WorldIDIdentityManagerImplV2Initialized(); + vm.expectEmit(true, true, true, true); emit Initialized(2); // Test diff --git a/src/test/identity-manager/WorldIDIdentityManagerTest.sol b/src/test/identity-manager/WorldIDIdentityManagerTest.sol index f7ffd187..fffcf09 100644 --- a/src/test/identity-manager/WorldIDIdentityManagerTest.sol +++ b/src/test/identity-manager/WorldIDIdentityManagerTest.sol @@ -183,6 +183,10 @@ contract WorldIDIdentityManagerTest is WorldIDTest { treeVerifier = new SimpleVerifier(initialBatchSize); defaultInsertVerifiers = new VerifierLookupTable(); defaultInsertVerifiers.addVerifier(initialBatchSize, treeVerifier); + defaultUpdateVerifiers = new VerifierLookupTable(); + defaultUpdateVerifiers.addVerifier(initialBatchSize, treeVerifier); + defaultDeletionVerifiers = new VerifierLookupTable(); + defaultDeletionVerifiers.addVerifier(initialBatchSize, treeVerifier); makeNewIdentityManager( treeDepth, initialRoot, diff --git a/src/test/identity-manager/WorldIDIdentityManagerUninit.t.sol b/src/test/identity-manager/WorldIDIdentityManagerUninit.t.sol index ddc6146..dcb7efe 100644 --- a/src/test/identity-manager/WorldIDIdentityManagerUninit.t.sol +++ b/src/test/identity-manager/WorldIDIdentityManagerUninit.t.sol @@ -40,13 +40,7 @@ contract WorldIDIdentityManagerUninit is WorldIDIdentityManagerTest { makeUninitIdentityManager(); bytes memory callData = abi.encodeCall( ManagerImpl.deleteIdentities, - ( - deletionProof, - deletionBatchSize, - packedDeletionIndices, - deletionPreRoot, - deletionPostRoot - ) + (deletionProof, packedDeletionIndices, deletionPreRoot, deletionPostRoot) ); bytes memory expectedError = abi.encodeWithSelector(CheckInitialized.ImplementationNotInitialized.selector);