feat: implement the governance smart-contract #193
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant changes to two smart contracts: Changes
Possibly related PRs
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}
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (1 hunks)
- packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol (3 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol
[warning] 15-15: State variables that could be declared immutable
ZetaXPGov.xpNFT (contracts/xp-nft/ZetaXPGov.sol#15) should be immutable
[warning] 16-16: State variables that could be declared immutable
ZetaXPGov.quorumPercentage (contracts/xp-nft/ZetaXPGov.sol#16) should be immutable
[warning] 31-31: Conformance to Solidity naming conventions
Parameter ZetaXPGov.setTagValidToVote(bytes32)._tag (contracts/xp-nft/ZetaXPGov.sol#31) is not in mixedCase
[warning] 85-87: Conformance to Solidity naming conventions
Function ZetaXPGov.CLOCK_MODE() (contracts/xp-nft/ZetaXPGov.sol#85-87) is not in mixedCasepackages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol
[warning] 99-99: Conformance to Solidity naming conventions
Parameter ZetaXP_V2.setBaseURI(string)._uri (contracts/xp-nft/xpNFT_V2.sol#99) is not in mixedCase
[failure] 105-109: ABI encodePacked Collision
ZetaXP_V2.tokenURI(uint256) (contracts/xp-nft/xpNFT_V2.sol#105-109) calls abi.encodePacked() with multiple dynamic arguments:
- string(abi.encodePacked(baseTokenURI,Strings.toString(tokenId))) (contracts/xp-nft/xpNFT_V2.sol#108)
[notice] 115-133: Block timestamp
ZetaXP_V2._verifyUpdateNFTSignature(uint256,ZetaXP_V2.UpdateData) (contracts/xp-nft/xpNFT_V2.sol#115-133) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > updateData.signatureExpiration (contracts/xp-nft/xpNFT_V2.sol#131)
🔇 Additional comments (2)
packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (2)
85-87: Confirm compliance ofCLOCK_MODE()with interface requirementsThe function
CLOCK_MODE()does not follow the standard mixedCase naming convention. However, since it overrides an interface that specifies it in uppercase, this naming is necessary. Ensure that this function name aligns with theIERC6372interface requirements.🧰 Tools
🪛 GitHub Check: Slither
[warning] 85-87: Conformance to Solidity naming conventions
Function ZetaXPGov.CLOCK_MODE() (contracts/xp-nft/ZetaXPGov.sol#85-87) is not in mixedCase
31-33:⚠️ Potential issueEnsure the
onlyGovernancemodifier is properly definedThe function
setTagValidToVoteuses theonlyGovernancemodifier, but its definition is not included in the provided code. Please verify thatonlyGovernanceis correctly defined in one of the inherited contracts or within this contract to enforce access control.Run the following script to confirm the existence of the
onlyGovernancemodifier:🧰 Tools
🪛 GitHub Check: Slither
[warning] 31-31: Conformance to Solidity naming conventions
Parameter ZetaXPGov.setTagValidToVote(bytes32)._tag (contracts/xp-nft/ZetaXPGov.sol#31) is not in mixedCase
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol (3)
19-22: Approve new mapping and event with a minor suggestionThe addition of
levelByTokenIdmapping andLevelSetevent are well-implemented and align with the new level functionality. The event properly uses indexed parameters for efficient filtering.Consider adding the new level to the event for better tracking:
-event LevelSet(address indexed sender, uint256 indexed tokenId, uint256 level); +event LevelSet(address indexed sender, uint256 indexed tokenId, uint256 oldLevel, uint256 newLevel);This change would allow easier tracking of level changes over time.
Line range hint
31-55: Approve level functionality implementation with optimization suggestionThe implementation of level setting and retrieval functionality is well-structured and follows best practices for signature verification and error handling. The
_verifySetLevelSignaturefunction properly checks the signature validity and expiration.To optimize gas usage and prevent unnecessary state changes and event emissions, consider adding a check in the
setLevelfunction to ensure the new level is different from the current level:function setLevel(SetLevelData memory data) external { _verifySetLevelSignature(data); + uint256 currentLevel = levelByTokenId[data.tokenId]; + if (currentLevel == data.level) { + return; + } + levelByTokenId[data.tokenId] = data.level; lastUpdateTimestampByTokenId[data.tokenId] = data.sigTimestamp; - emit LevelSet(msg.sender, data.tokenId, data.level); + emit LevelSet(msg.sender, data.tokenId, currentLevel, data.level); }This change prevents unnecessary state updates and event emissions when the level remains unchanged, potentially saving gas costs.
🧰 Tools
🪛 GitHub Check: Slither
[warning] 27-29: Dead-code
ZetaXP_V2._verifyUpdateNFTSignature(uint256,ZetaXP.UpdateData) (contracts/xp-nft/xpNFT_V2.sol#27-29) is never used and should be removed
57-59: ApprovetotalSupplyimplementation with suggestion for robustnessThe
totalSupplyfunction correctly calculates the total number of minted tokens, assuming that token IDs start from 1 and are sequential. This implementation is suitable for most cases where tokens are not burned.However, to make the contract more robust and account for potential future token burns, consider maintaining a separate
_totalSupplyvariable:+uint256 private _totalSupply; function mint(address to) public virtual { uint256 tokenId = _getNextTokenId(); _safeMint(to, tokenId); _incrementTokenId(); + _totalSupply++; } +function burn(uint256 tokenId) public virtual { + // Implement burn logic + _burn(tokenId); + _totalSupply--; +} function totalSupply() external view returns (uint256) { - return _currentTokenId - 1; + return _totalSupply; }This approach ensures accurate tracking of the total supply even if tokens are burned or if the minting logic changes in the future.
packages/zevm-app-contracts/contracts/xp-nft/xpNFT.sol (1)
Line range hint
100-118: Approved with recommendations: Visibility change enhances flexibility, but timestamp usage needs attention.The modification of the
_verifyfunction fromprivatetointernalis a sound decision, as it allows derived contracts to utilize and potentially extend the verification logic. However, there are two points that warrant attention:
Timestamp Dependency:
The function usesblock.timestampfor comparisons, which was flagged by the static analysis tool. While this is a common practice, it's important to note that miners can manipulate timestamps within a certain range.Recommendation: Add a tolerance threshold to the timestamp comparison to mitigate potential manipulation:
if (block.timestamp > updateData.signatureExpiration + TIMESTAMP_TOLERANCE) revert SignatureExpired();Where
TIMESTAMP_TOLERANCEis a constant representing an acceptable time drift (e.g., 15 minutes).Signature Replay Protection:
To further enhance security, consider implementing a nonce-based approach to prevent signature replay attacks.To implement these improvements, please apply the following changes:
function _verify(uint256 tokenId, UpdateData memory updateData) internal view { + uint256 constant TIMESTAMP_TOLERANCE = 15 minutes; bytes32 structHash = keccak256( abi.encode( MINTORUPDATE_TYPEHASH, updateData.to, updateData.signatureExpiration, updateData.sigTimestamp, updateData.tag + updateData.nonce ) ); bytes32 constructedHash = _hashTypedDataV4(structHash); if (!SignatureChecker.isValidSignatureNow(signerAddress, constructedHash, updateData.signature)) { revert InvalidSigner(); } - if (block.timestamp > updateData.signatureExpiration) revert SignatureExpired(); + if (block.timestamp > updateData.signatureExpiration + TIMESTAMP_TOLERANCE) revert SignatureExpired(); if (updateData.sigTimestamp <= lastUpdateTimestampByTokenId[tokenId]) revert OutdatedSignature(); + if (usedNonces[updateData.to][updateData.nonce]) revert NonceAlreadyUsed(); + usedNonces[updateData.to][updateData.nonce] = true; }Don't forget to add the
noncefield to theUpdateDatastruct and theusedNoncesmapping to the contract state.packages/zevm-app-contracts/test/xp-nft/zeta-xp-gov.ts (1)
76-77: Avoid magic numbers by defining constants for time intervals.The hardcoded value
1000used forsignatureExpirationmay reduce code clarity. Defining a constant variable enhances readability and makes future adjustments easier.Implement the change:
const SIGNATURE_EXPIRATION_OFFSET = 1000; // Usage const signatureExpiration = sigTimestamp + SIGNATURE_EXPIRATION_OFFSET;Also applies to: 106-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (1 hunks)
- packages/zevm-app-contracts/contracts/xp-nft/xpNFT.sol (3 hunks)
- packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol (3 hunks)
- packages/zevm-app-contracts/test/xp-nft/zeta-xp-gov.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol
[warning] 15-15: State variables that could be declared immutable
ZetaXPGov.xpNFT (contracts/xp-nft/ZetaXPGov.sol#15) should be immutable
[warning] 16-16: State variables that could be declared immutable
ZetaXPGov.quorumPercentage (contracts/xp-nft/ZetaXPGov.sol#16) should be immutable
[warning] 32-32: Conformance to Solidity naming conventions
Parameter ZetaXPGov.setTagValidToVote(bytes32)._tag (contracts/xp-nft/ZetaXPGov.sol#32) is not in mixedCase
[warning] 86-88: Conformance to Solidity naming conventions
Function ZetaXPGov.CLOCK_MODE() (contracts/xp-nft/ZetaXPGov.sol#86-88) is not in mixedCasepackages/zevm-app-contracts/contracts/xp-nft/xpNFT.sol
[notice] 100-118: Block timestamp
ZetaXP._verify(uint256,ZetaXP.UpdateData) (contracts/xp-nft/xpNFT.sol#100-118) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > updateData.signatureExpiration (contracts/xp-nft/xpNFT.sol#116)packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol
[warning] 27-29: Dead-code
ZetaXP_V2._verifyUpdateNFTSignature(uint256,ZetaXP.UpdateData) (contracts/xp-nft/xpNFT_V2.sol#27-29) is never used and should be removed
🔇 Additional comments (4)
packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol (1)
7-8: Consider the security implications of changingSETLEVEL_TYPEHASHvisibilityThe visibility of
SETLEVEL_TYPEHASHhas been changed fromprivatetointernal. While this allows for more flexibility in derived contracts, it also increases the attack surface by exposing this constant to potential misuse.Ensure that this change is intentional and necessary. If not, consider reverting to
privatevisibility to maintain the principle of least privilege.packages/zevm-app-contracts/contracts/xp-nft/xpNFT.sol (2)
11-12: Approved: Visibility change enhances contract extensibility.The modification of
MINTORUPDATE_TYPEHASHfromprivatetointernalis a judicious decision. This alteration facilitates access to the constant by derived contracts, thereby enhancing the extensibility of theZetaXPcontract. The change does not introduce any security vulnerabilities, as the constant's value remains immutable.
31-31: Approved with caution: Visibility change requires careful management in derived contracts.The alteration of
_currentTokenIdfromprivatetointernalis acceptable but warrants careful consideration. While this change enables derived contracts to access and potentially modify the token ID counter, it also introduces risks if not properly managed.To mitigate potential issues:
- Implement a
virtualfunction for incrementing_currentTokenIdthat can be overridden in derived contracts.- Add thorough documentation explaining the proper usage and potential risks associated with modifying this variable.
- Consider implementing additional checks to ensure
_currentTokenIdis always incremented and never decremented.To ensure this change doesn't introduce unintended side effects, please run the following verification script:
✅ Verification successful
Verification Successful: Visibility change of
_currentTokenIdis appropriately managed.The visibility alteration of
_currentTokenIdfromprivatetointernalhas been verified successfully. All direct modifications and increment operations are confined toxpNFT.sol, ensuring controlled access within the contract.Recommendations:
- Maintain the use of
virtualfunctions for any future extensions in derived contracts.- Continue thorough documentation to guide developers on the proper usage and potential risks.
- Ensure that additional checks remain in place to safeguard the integrity of
_currentTokenId.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any direct modifications of _currentTokenId in the codebase echo "Searching for direct modifications of _currentTokenId:" rg --type solidity '_currentTokenId\s*=[^+]' . echo "Searching for increment operations on _currentTokenId:" rg --type solidity '_currentTokenId\+\+' .Length of output: 397
Script:
#!/bin/bash # Description: Check for any direct modifications and increment operations of _currentTokenId in Solidity files echo "Searching for Solidity files..." SOL_FILES=$(fd --extension sol) if [ -z "$SOL_FILES" ]; then echo "No Solidity files found in the repository." exit 0 fi echo "Searching for direct modifications of _currentTokenId:" echo "$SOL_FILES" | xargs rg '_currentTokenId\s*=[^+]' echo "Searching for increment operations on _currentTokenId:" echo "$SOL_FILES" | xargs rg '_currentTokenId\+\+'Length of output: 9921
packages/zevm-app-contracts/test/xp-nft/zeta-xp-gov.ts (1)
11-11: Ensure consistent tag encoding across contracts.The function
encodeTagusesethers.utils.keccak256for tag encoding. Confirm that all contracts interacting with these tags use the same encoding method to prevent potential mismatches or inconsistencies.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (2)
19-22: Align constructor parameter names with Solidity conventionsThe constructor parameters use leading underscores (e.g.,
_xpNFT,_timelock). According to Solidity naming conventions, function parameters should be in mixedCase without leading underscores. Consider renaming them for consistency and readability.Apply this change to the constructor signature:
constructor( - ZetaXP_V2 _xpNFT, - TimelockController _timelock, - uint256 _quorumPercentage, - bytes32 _tag + ZetaXP_V2 xpNFTAddress, + TimelockController timelockController, + uint256 quorumPercentageValue, + bytes32 tag )And update the constructor body accordingly:
- xpNFT = _xpNFT; - quorumPercentage = _quorumPercentage; - tagValidToVote = _tag; + xpNFT = xpNFTAddress; + quorumPercentage = quorumPercentageValue; + tagValidToVote = tag;
33-35: Ensure parameter naming conventions insetTagValidToVoteThe parameter
_tagincludes a leading underscore. For consistency with Solidity conventions, consider renaming_tagtotag.Update the function signature:
-function setTagValidToVote(bytes32 _tag) external onlyGovernance { - tagValidToVote = _tag; +function setTagValidToVote(bytes32 tag) external onlyGovernance { + tagValidToVote = tag; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (1 hunks)
- packages/zevm-app-contracts/hardhat.config.ts (1 hunks)
- packages/zevm-app-contracts/test/xp-nft/zeta-xp-gov.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/zevm-app-contracts/test/xp-nft/zeta-xp-gov.ts
🧰 Additional context used
🔇 Additional comments (3)
packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (3)
15-16: Consider declaringxpNFTandquorumPercentageasimmutableSince
xpNFTandquorumPercentageare assigned once in the constructor and not modified thereafter, declaring them asimmutablecan optimize gas usage and indicate their constant nature.
47-55: 🛠️ Refactor suggestionExplicitly handle future NFT levels in voting weights
The
_getVotesfunction assigns voting weights for levels 1 to 3 and defaults to zero otherwise. If additional levels are introduced in the future, they will have zero voting power by default. To make the code more maintainable, consider explicitly handling all possible levels or updating the comments to reflect this behavior.Update the function to handle unexpected levels:
if (level == 1) { return 1; // Rosegold } else if (level == 2) { return 2; // Black } else if (level == 3) { return 3; // Green +} else if (level == 0) { + return 0; // Silver cannot vote +} else { + return 0; // Default voting power for other levels }Alternatively, add comments to clarify that only levels 1 to 3 have voting rights.
Likely invalid or redundant comment.
43-45:⚠️ Potential issuePrevent potential reverts due to missing NFTs in
_getVotesIn
_getVotes, if a user does not have an NFT with the specified tag,xpNFT.tokenByUserTagmight revert or return an invalidtokenId. Similarly,xpNFT.getLevelmay fail for an invalidtokenId. To enhance robustness, add checks to handle cases where the user lacks a valid NFT.Modify the function to handle such scenarios:
uint256 tokenId = xpNFT.tokenByUserTag(account, tagValidToVote); +if (tokenId == 0) { + return 0; // User does not have a valid NFT for voting +} uint256 level = xpNFT.getLevel(tokenId);Ensure that
xpNFT.tokenByUserTagreturns0or a suitable default when no token exists, and thatgetLevelcan handle an invalidtokenIdwithout reverting.Likely invalid or redundant comment.
andresaiello
deleted the
andy/pd-6699-implement-the-governance-smart-contract
branch
Summary
Summary by CodeRabbit
New Features
Bug Fixes
Documentation