6.1 Nullifier hashes may be consumed without a valid proof
When attempting to execute a priority transaction through the PBH entrypoint, a user must provide a valid external nullifier and nullifier hash. These are combined with other data to verify a proof, ensuring that the user has a valid World ID. To prevent replay attacks, a storage mapping nullifierHashes tracks all previously used hashes. Upon successful execution of a PBH transaction, the nullifier hash will be marked as "used" and cannot be used again.
The entrypoint contract supports two PBH flows: handleAggregatedOps and pbhMulticall. Both functions verify the proof and mark the nullifier hash as used. If the entrypoint storage variable worldId is not set, then proofs are not verified onchain, and it becomes the builder's responsibility to validate them.
function verifyPbh(uint256 signalHash, PBHPayload memory pbhPayload) ... {
// Check for nullifier hash reuse
if (nullifierHashes[pbhPayload.nullifierHash]) {
revert InvalidNullifier();
}
// Verify the external nullifier
PBHExternalNullifier.verify(pbhPayload.pbhExternalNullifier, numPbhPerMonth);
// Skip onchain verification if `worldId` is not set
if (address(worldId) != address(0)) {
worldId.verifyProof(
..., pbhPayload.nullifierHash, ...
);
}
}
When the worldId is not set, it may be possible for a user to call either of the two entrypoint functions with an invalid proof and still consume a nullifier hash. Depending on the builder implementation, there are two behaviors when an invalid proof is provided to the PBH entrypoint contract:
- Allow the transaction to be included in the block but without priority
- Reject the transaction, preventing it from being included in the block
If the builder follows the first behavior, users could call handleAggregatedOps or pbhMulticall with invalid proofs and still consume these nullifier hashes. During discussions related to this finding, the Worldcoin team raised the concern that an entity could become a bundler and get access to user PBH calls before they have been executed. This malicious entity could then attempt to mark a user's nullifier hashes as used before the user transaction executes, causing the user's call to revert since the nullifier hash is already used.
Recommendation
Ensure that the Worldchain block builder excludes transactions that include a call to the PBH entrypoint functions handleAggregatedOps and pbhMulticall if they do not meet the PBH conditions (invalid proof).
Resolution
The World Chain block builder ensures that transactions can not call into the PBHEntryPoint unless it is specified as the transaction’s to address. Additionally, in the follow-up commit a change has been applied to only allow authorized builder addresses to spend nullifier hashes.
6.2 Missing length check for pbhPayloads and userOps
In the function handleAggregatedOps, there is a nested loop which iterates through the userOps for each aggregator. Each userOp should have a phbPayload associated with it, which is used in verifyPbh to ensure that a valid proof has been provided. If any userOp has an invalid proof then the transaction will revert. The code is shown below:
function handleAggregatedOps(...) ... {
for (uint256 i = 0; i < opsPerAggregator.length; ++i) {
//...
PBHPayload[] memory pbhPayloads = abi.decode(
opsPerAggregator[i].signature,
(PBHPayload[])
);
for (uint256 j = 0; j < pbhPayloads.length; ++j) {
address sender = opsPerAggregator[i].userOps[j].sender;
// Generate signal hash for verification
uint256 signalHash = abi.encodePacked(...).hashToField();
// Verify proof
verifyPbh(signalHash, pbhPayloads[j]);
// ...
}
}
// ...
}
However, there are no checks to ensure that the length of pbhPayloads extracted from the aggregator signature is equal to the number of userOps in the aggregator's struct. The inner loop which does the proof verification will stop once it has reached the length of pbhPayloads. When pbhPayloads is less than userOps the loop will stop executing even though there are additional user operations which have not been verified. These skipped userOps will not be verified and their nullifier hash will not be consumed, but their call will still execute successfully.
This method of skipping proof verification can be done even when the worldId address is set and onchain proof verification is supposed to occur. Since this logic flow of skipping proof verification is unexpected, the builder implementation may not be written in such a way that can detect this, and may consider all userOps as PBH regardless. If this is the case, then any user may be able to append additional userOps without an associated proof and bypass the numPbhPerMonth limit.
Recommendation
Consider adding a check to handleAggregatedOps to ensure that the length of pbhPayloads is equal to opsPerAggregator.
Resolution
Addressed in PR #140.
6.3 Unnecessary gas check in pbhMulticall
The function pbhMulticall verifies a user's calls and pbhPayload before making a call to Multicall3.aggregate3, which executes the users intended actions. After these calls are executed, the following gas check is done:
function pbhMulticall(...) ... returns (...) {
// Verify calls and proof
// ...
returnData = IMulticall3(_multicall3).aggregate3{gas: pbhGasLimit}(calls);
// ...
// Check if pbh gas limit is exceeded
if (gasleft() > pbhGasLimit) {
revert GasLimitExceeded(gasleft());
}
// ...
}
The purpose of this check is to ensure that an excessive amount of gas hasn't been spent in the transaction, as it may affect other PBH users if excessive gas amounts reach the block limits. However, this check is ineffective as it's possible for gas to have been spent before the call to pbhMulticall. The check only ensures that there is some small amount of gas (pbhGasLimit) remaining after the call.
Recommendation
Consider removing the ineffective check, and explore options for a more complete gas spend check, potentially one that is integrated into the builder.
Resolution
Addressed in PR #139.
6.4 Opportunity to save gas in checkSignatures on next Safe release
The PBH4337Module is an override of a forked Safe4337Module with a change to _validateSignatures which allows it to handle the unique PBH signature structure. Once the userOp.signature has been processed properly (prepended and appended data extracted) the module calls checkSignatures on the user's Safe smart contract account, as shown below:
function _validateSignatures(...) ... {
// ...
// Get threshold
uint256 threshold = ISafe(payable(userOp.sender)).getThreshold();
// ...
// Extract signature
if (isPBH && userOp.signature.length > expectedLength) {
if (userOp.signature.length - expectedLength != ENCODED_PROOF_BYTES) {
revert InvalidProofSize();
}
signatures = userOp.signature[TIMESTAMP_BYTES:expectedLength];
}
// ...
try ISafe(payable(userOp.sender)).checkSignatures(keccak256(operationData), operationData, signatures) {}
catch {
validSignature = false;
}
// ...
}
In Safe's latest code (ahead of the latest version 1.4.1-3), there are two versions of this function, each accepting different arguments:
checkSignatures(bytes32,bytes,bytes)
checkSignatures(address,bytes32,bytes)
A comment in the Safe implementation states that the (bytes32,bytes,bytes) function will only exist for compatibility reasons, and that the newer (address,bytes32,bytes) should be used instead. When the next Safe version is released, using the newer function will reduce gas costs since the unhashed operationData won't have to be sent as part of the call, only the hashed data.
Recommendation
No action needs to be taken now, this finding only serves to highlight the opportunity to reduce gas costs during signature verification when a future Safe version is released.
Resolution
Acknowledged.
6.5 Natspec and comments
The following is a list of Natspec and comment corrections:
`PBHEntryPointImplV1.initialize` the args `multicall3` and `_pbhGasLimit` are missing natspec comments
`PBHEntryPointImplV1.initialize` @dev comment for `_worldId` has typo "addess" -> "address"
`PBHEntryPointImplV1.PBHEntryPointImplInitialized` event arg `pbhGasLimit` missing natspec comment
`PBHExternalNullifier.encode` the arg `version` is missing a natspec comment
Recommendation
Consider correcting the comments mentioned above.
Resolution
Addressed in PR #142 and PR #153.
