Skip to content

Commit

Permalink
PRO-343 Implement audit suggestions (#134)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@dcbuild3r
dcbuild3r authored Oct 11, 2023
1 parent 42c26ec commit 121cffc
Show file tree
Hide file tree
Showing 6 changed files with 98 additions and 59 deletions.
30 changes: 29 additions & 1 deletion src/WorldIDIdentityManagerImplV1.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 ///
///////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -287,13 +292,26 @@ 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,
VerifierLookupTable _batchInsertionVerifiers,
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();

Expand Down Expand Up @@ -515,13 +533,18 @@ 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
onlyProxy
onlyInitialized
onlyOwner
{
if (address(newTable) == address(0)) {
revert InvalidVerifierLUT();
}

VerifierLookupTable oldTable = batchInsertionVerifiers;
batchInsertionVerifiers = newTable;
emit DependencyUpdated(
Expand All @@ -548,13 +571,18 @@ 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
onlyProxy
onlyInitialized
onlyOwner
{
if (address(newVerifier) == address(0)) {
revert InvalidVerifier();
}

ISemaphoreVerifier oldVerifier = semaphoreVerifier;
semaphoreVerifier = newVerifier;
emit DependencyUpdated(
Expand Down
52 changes: 39 additions & 13 deletions src/WorldIDIdentityManagerImplV2.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
///////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -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)
Expand All @@ -61,16 +68,23 @@ 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();
}

///////////////////////////////////////////////////////////////////
/// IDENTITY MANAGEMENT ///
///////////////////////////////////////////////////////////////////

/// @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.
///
Expand All @@ -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`.
///
Expand All @@ -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);
}
Expand All @@ -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
Expand Down Expand Up @@ -158,13 +179,18 @@ 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
onlyProxy
onlyInitialized
onlyOwner
{
if (address(newTable) == address(0)) {
revert InvalidVerifierLUT();
}

VerifierLookupTable oldTable = batchDeletionVerifiers;
batchDeletionVerifiers = newTable;
emit DependencyUpdated(
Expand All @@ -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.
Expand Down
58 changes: 20 additions & 38 deletions src/test/identity-manager/WorldIDIdentityManagerIdentityDeletion.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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,
Expand All @@ -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 =
Expand All @@ -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,
Expand All @@ -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);
Expand All @@ -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);
}
Expand All @@ -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,
Expand All @@ -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);

Expand All @@ -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,
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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)
Expand All @@ -342,7 +324,7 @@ contract WorldIDIdentityManagerIdentityDeletion is WorldIDIdentityManagerTest {

// Test
managerImpl.deleteIdentities(
deletionProof, deletionBatchSize, packedDeletionIndices, initialRoot, deletionPostRoot
deletionProof, packedDeletionIndices, initialRoot, deletionPostRoot
);
}
}
Loading

0 comments on commit 121cffc

Please sign in to comment.