feat: new enrollment contract #196
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several changes across multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/zevm-app-contracts/contracts/zeta-points/InvitationManagerV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/zevm-app-contracts/contracts/zeta-points/InvitationManagerV2.sol
Dismissed
Show dismissed
Hide dismissed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (13)
.vscode/settings.json (1)
9-9: Excellent change to ESLint behavior.Switching from automatic to explicit ESLint fixes is a prudent decision, especially in the context of smart contract development where unintended code modifications could have significant implications. This change ensures developers maintain full control over code modifications while still benefiting from ESLint's suggestions.
Consider documenting this change in the project's contributing guidelines to ensure all team members understand the new workflow and how to manually trigger ESLint fixes when needed.
packages/zevm-app-contracts/test/zeta-points/invitationManager.helpers.ts (2)
3-10: Enhance type safety and add documentation for interfaces.Consider improving type safety and adding documentation:
+/** + * Represents a basic enrollment request. + */ export interface Enrollment { - to: string; + to: `0x${string}`; // Ethereum address } +/** + * Represents a signed enrollment request with expiration. + */ export interface EnrollmentSigned extends Enrollment { signature: string; - signatureExpiration: number; + signatureExpiration: bigint; // Unix timestamp }
26-31: Consider adding nonce to prevent signature replay attacks.The current implementation might be vulnerable to signature replay attacks.
Consider adding a nonce to the signature payload:
const types = { Verify: [ { name: "to", type: "address" }, { name: "signatureExpiration", type: "uint256" }, + { name: "nonce", type: "uint256" }, ], };This change would require corresponding updates in the smart contract to track and validate nonces.
packages/zevm-app-contracts/data/addresses.json (1)
14-15: Clarify the migration strategy for InvitationManagerV2.The configuration now includes both
invitationManagerandinvitationManagerV2. Please clarify:
- The migration strategy from V1 to V2
- Whether both versions will coexist
- If there's a plan to deprecate V1
Consider documenting the migration strategy in the README or a separate migration guide.
packages/zevm-app-contracts/test/zeta-points/InvitationManagerV2.ts (5)
10-10: Extract chain ID as a named constant.The hardhat chain ID should be defined in a shared constants file since it might be used across multiple test files.
-const HARDHAT_CHAIN_ID = 1337; +import { HARDHAT_CHAIN_ID } from '../constants';
30-35: Enhance timestamp helper function.The timestamp calculation contains magic numbers and could be more flexible.
- const getTomorrowTimestamp = async () => { + const ONE_DAY_IN_SECONDS = 24 * 60 * 60; + const getFutureTimestamp = async (secondsToAdd: number = ONE_DAY_IN_SECONDS) => { const block = await ethers.provider.getBlock("latest"); const now = block.timestamp; - const tomorrow = now + 24 * 60 * 60; - return tomorrow; + return now + secondsToAdd; };
58-58: Fix spelling inconsistencies.The word "enrollment" is spelled inconsistently throughout the file ("enrollement" vs "enrollment").
- it("Should execute enrollement with from other", async () => { + it("Should execute enrollment with from other", async () => { - const enrollementParams: EnrollmentSigned = { + const enrollmentParams: EnrollmentSigned = {Also applies to: 72-72, 114-114, 138-138, 165-165
86-86: Fix typos in test descriptions.Test descriptions contain typos ("previus" should be "previous").
- it("Should check if user was enroll in previus version", async () => { + it("Should check if user was enrolled in previous version", async () => { - it("Should fail if try to enroll and was enrolled with previus contract", async () => { + it("Should fail if try to enroll and was enrolled with previous contract", async () => {Also applies to: 150-150
45-48: Reduce verification boilerplate.The verification check blocks are duplicated across test cases. Consider extracting into a helper function.
const expectUserNotVerified = async (address: string) => { const hasBeenVerifiedBefore = await invitationManagerV2.hasBeenVerified(address); await expect(hasBeenVerifiedBefore).to.be.eq(false); };Also applies to: 59-62, 87-90, 100-104, 124-128, 151-154
packages/zevm-app-contracts/contracts/zeta-points/InvitationManagerV2.sol (4)
59-59: Validate signature expiration before signature verificationCurrently, the signature expiration check occurs after the signature verification. To optimize gas usage and enhance security, consider checking if the signature has expired before performing signature verification. This prevents unnecessary signature verification if the signature has already expired.
Apply this diff to reorder the checks:
function _verify(VerifyData memory claimData) private view { + if (block.timestamp > claimData.signatureExpiration) revert SignatureExpired(); bytes32 structHash = keccak256(abi.encode(VERIFY_TYPEHASH, claimData.to, claimData.signatureExpiration)); bytes32 constructedHash = _hashTypedDataV4(structHash); if (!SignatureChecker.isValidSignatureNow(claimData.to, constructedHash, claimData.signature)) { revert InvalidSigner(); } - if (block.timestamp > claimData.signatureExpiration) revert SignatureExpired(); }
46-46: Consider potential risks withblock.timestampmanipulationIn the
hasBeenVerifiedfunction, you useblock.timestampindirectly throughuserVerificationTimestampsfor verification logic. While acceptable in many contexts, be aware that miners can manipulateblock.timestampwithin a limited range. Ensure that this does not introduce security vulnerabilities in the verification process.
59-59: Be cautious withblock.timestampin time-sensitive validationsUsing
block.timestampfor signature expiration in_verifycan be susceptible to minor manipulations by miners. Consider adding a small time buffer to account for potential discrepancies, or document the acceptable range of deviation to mitigate any risks.
55-57: Optimize signature verification usingECDSA.recoverWhile
SignatureChecker.isValidSignatureNowis suitable for general cases, usingECDSA.recovercan be more efficient when the signer is expected to be an externally owned account (EOA). This can reduce gas costs and simplify the verification logic.Apply this diff to use
ECDSA.recover:+import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; function _verify(VerifyData memory claimData) private view { + if (block.timestamp > claimData.signatureExpiration) revert SignatureExpired(); bytes32 structHash = keccak256(abi.encode(VERIFY_TYPEHASH, claimData.to, claimData.signatureExpiration)); bytes32 constructedHash = _hashTypedDataV4(structHash); - if (!SignatureChecker.isValidSignatureNow(claimData.to, constructedHash, claimData.signature)) { + address signer = ECDSA.recover(constructedHash, claimData.signature); + if (signer != claimData.to) { revert InvalidSigner(); } - if (block.timestamp > claimData.signatureExpiration) revert SignatureExpired(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.vscode/settings.json(1 hunks)packages/zevm-app-contracts/contracts/zeta-points/InvitationManager.sol(2 hunks)packages/zevm-app-contracts/contracts/zeta-points/InvitationManagerV2.sol(1 hunks)packages/zevm-app-contracts/data/addresses.json(2 hunks)packages/zevm-app-contracts/scripts/zeta-points/deploy-invitationV2.ts(1 hunks)packages/zevm-app-contracts/test/zeta-points/InvitationManagerV2.ts(1 hunks)packages/zevm-app-contracts/test/zeta-points/invitationManager.helpers.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/zevm-app-contracts/contracts/zeta-points/InvitationManagerV2.sol
[notice] 45-49: Block timestamp
InvitationManagerV2.hasBeenVerified(address) (contracts/zeta-points/InvitationManagerV2.sol#45-49) uses timestamp for comparisons
Dangerous comparisons:
- userVerificationTimestamps[userAddress] > 0 (contracts/zeta-points/InvitationManagerV2.sol#46)
[notice] 51-60: Block timestamp
InvitationManagerV2._verify(InvitationManagerV2.VerifyData) (contracts/zeta-points/InvitationManagerV2.sol#51-60) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > claimData.signatureExpiration (contracts/zeta-points/InvitationManagerV2.sol#59)
🔇 Additional comments (3)
packages/zevm-app-contracts/scripts/zeta-points/deploy-invitationV2.ts (1)
1-8: LGTM: Well-structured imports and setup
The imports are properly organized, with clear separation between external dependencies and internal helpers.
packages/zevm-app-contracts/data/addresses.json (1)
4-4: Verify the disperse contract address update.
The disperse contract address has been updated in the testnet environment. This change appears unrelated to the PR objectives focusing on the new enrollment contract.
Let's verify this change:
✅ Verification successful
Let me gather more information about the disperse contract deployment history.
Disperse contract address update appears intentional and valid
The address change was introduced in commit 764cb2d as part of the "new enrollment contract" feature deployment. The deployment script exists at packages/zevm-app-contracts/scripts/disperse/deploy.ts and follows the standard deployment pattern. The change only affects the testnet environment while maintaining the production mainnet address, which is a proper deployment practice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any deployment scripts or tests related to the disperse contract update
# Search for deployment scripts mentioning the old or new address
rg -l "0x23ce409Ea60c3d75827d04D9db3d52F3af62e44d|0x049893Bd0fC4923FC1B1136Ef2ac996C55D4942C"
# Search for recent changes related to the disperse contract
rg -i "disperse" --type ts
Length of output: 2872
Script:
#!/bin/bash
# Check git history for the addresses.json file to understand the context of the address change
git log -p packages/zevm-app-contracts/data/addresses.json
# Check for any recent deployment logs or configuration changes
rg -l "deploy.*disperse" --type ts --type log --type json
Length of output: 3598
packages/zevm-app-contracts/contracts/zeta-points/InvitationManager.sol (1)
72-72: LGTM: Function visibility change aligns with inheritance needs.
Changing _verifySignature to internal is appropriate as it enables reuse in derived contracts while maintaining security. This change supports the new enrollment contract feature.
#!/bin/bash
# Check for potential derived contracts that might use this function
echo "Checking for contracts that inherit from InvitationManager..."
ast-grep --pattern 'contract $_ is InvitationManager'
packages/zevm-app-contracts/test/zeta-points/invitationManager.helpers.ts
Show resolved
Hide resolved
packages/zevm-app-contracts/test/zeta-points/invitationManager.helpers.ts
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/zeta-points/InvitationManagerV2.sol
Show resolved
Hide resolved
Summary
Summary by CodeRabbit
Release Notes
New Features
InvitationManagerV2contract, enhancing user verification through signatures and implementing EIP712 standards.InvitationManagerV2.Bug Fixes
Updates
InvitationManagercontract.addresses.jsonfile for thezeta_testnetenvironment.Tests
InvitationManagerV2contract, covering various verification scenarios.