PBHVerifier #74
PBHVerifier #74
Conversation
v1.9.4
pbh-verifier/src/PBHVerifier.sol
Outdated
| * The paymaster will pay for the transaction instead of the sender. | ||
| * @param signature - Sender-verified signature over the entire request, the EntryPoint address and the chain ID. | ||
| */ | ||
| struct PackedUserOperation { |
There was a problem hiding this comment.
Can we inherit this type from the account-abstraction contracts?
There was a problem hiding this comment.
Haha yeah for sure :) sorry this isn't even close to being ready for review haha. I just opened the PR to show the external nullifier implementation!
pbh-verifier/src/PBHVerifier.sol
Outdated
| IWorldIDGroups internal immutable worldId; | ||
|
|
||
| /// @dev The World ID group ID (always 1) | ||
| uint256 internal immutable groupId = 1; |
There was a problem hiding this comment.
Nit: Consider SCREAMING_SNAKE_CASE for immutables, similar to constants. There is no hard and fast rule for this but this is a common pattern.
|
Looks great, very clean! |
pbh-verifier/src/PBHVerifier.sol
Outdated
| PackedUserOperation memory userOp, | ||
| uint256 root, | ||
| uint256 nullifierHash, | ||
| ExternalNullifier memory externalNullifier, |
There was a problem hiding this comment.
how are we getting the externalNullifier data here - do we need to add this with the proof in the signature
or is this determined by the builder?
There was a problem hiding this comment.
@karankurbur Yes, this should be decoded from the signature on the UserOperation
There was a problem hiding this comment.
This looks good as a first pass, very clean! One small nit would be to rename the top level directory to contracts since the PbhAggregator I'm assuming will live in the foundry workspace + different contracts, and utils.
Additionally can we simply emit the proof instead of verifying on chain after checking the uniqueness of the nullifier hash for the builder to verify during transaction validation.
There was a problem hiding this comment.
Can we also move this workflow to the root .github/workflows
| /// @return The encoded PBHExternalNullifier. | ||
| function encode(uint8 pbhNonce, uint8 month, uint16 year) internal pure returns (uint256) { | ||
| require(month > 0 && month < 13, InvalidExternalNullifierMonth()); | ||
| require(year <= 9999, InvalidExternalNullifierYear()); |
There was a problem hiding this comment.
Nit: You can use type(uint16).max here.
require(year <= type(uint16).max, InvalidExternalNullifierYear());
![semgrep-code-worldcoin[bot]](https://avatars.githubusercontent.com/u/60940476?s=60&v=4){kind=small}
| RUN ./script/generate_anvil_state.sh | ||
|
|
||
| ENTRYPOINT ["anvil", "--host", "0.0.0.0", "--load-state", "state.json"] | ||
| CMD [] |
There was a problem hiding this comment.
By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'.
| CMD [] | |
| USER non-root | |
| CMD [] |
There was a problem hiding this comment.
Semgrep Assistant suggests the following fix: Add a non-root user to the Dockerfile and specify it with the USER instruction before the CMD.
View step-by-step instructions
- Add a new user to the Dockerfile by including the following line before the
CMDinstruction:RUN groupadd -r app && useradd -r -g app -u 2000 app. - Change the ownership of the application directory to the new user by adding:
chown app:app -R /world-id. - Specify the user to run the application by adding the line
USER 2000before theCMDinstruction.
Leave feedback with a 👍 / 👎. Save a memory with /remember <your custom instructions>.
| # RUN ls script; exit 1 | ||
| RUN ./script/generate_anvil_state.sh | ||
|
|
||
| ENTRYPOINT ["anvil", "--host", "0.0.0.0", "--load-state", "state.json"] |
There was a problem hiding this comment.
By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'.
| ENTRYPOINT ["anvil", "--host", "0.0.0.0", "--load-state", "state.json"] | |
| USER non-root | |
| ENTRYPOINT ["anvil", "--host", "0.0.0.0", "--load-state", "state.json"] |
There was a problem hiding this comment.
Semgrep Assistant suggests the following fix: Switch to a non-root user in the Dockerfile to run the application with reduced privileges.
View step-by-step instructions
- Add a non-root user to the Dockerfile. You can do this by adding a line such as
RUN useradd -m non-rootafter theCOPY . .line. - Switch to the non-root user by adding
USER non-rootbefore theENTRYPOINTline. This ensures that the application runs with non-root privileges, reducing security risks.
Leave feedback with a 👍 / 👎. Save a memory with /remember <your custom instructions>.
Smart contract that verifies PBH Proofs on chain.