feat: 1 smart contract per activity #195
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Suggested reviewers
Warning Tool Failures:Tool Failure Count: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/instant-rewards/InstantRewardsFactory.sol
Dismissed
Show dismissed
Hide dismissed
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.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: 3
🧹 Outside diff range and nitpick comments (8)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (2)
55-55: Consider defining an interface or abstract contract for consistent inheritance.The change from
externaltopublic virtualforclaim()andwithdraw()suggests these functions are meant to be overridden. To ensure consistent implementation across derived contracts:
- Consider extracting an interface or abstract contract
- Evaluate if other functions should also be virtual
Example interface structure:
interface IInstantRewards { function claim(ClaimData memory claimData) external; function withdraw(address wallet, uint256 amount) external; }Also applies to: 75-75
🧰 Tools
🪛 GitHub Check: Slither
[warning] 55-67: Low-level calls
Low level call in InstantRewards.claim(InstantRewards.ClaimData) (contracts/instant-rewards/InstantRewards.sol#55-67):
- (success,None) = claimData.to.call{value: claimData.amount}() (contracts/instant-rewards/InstantRewards.sol#63)
Line range hint
55-67: Add validation for zero-amount claims.While the function is well-protected against reentrancy and follows the checks-effects-interactions pattern, consider adding a zero-amount validation to prevent unnecessary transactions.
function claim(ClaimData memory claimData) public virtual nonReentrant whenNotPaused { claimData.to = msg.sender; + if (claimData.amount == 0) revert InvalidAmount(); _verify(claimData);packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts (4)
8-8: Consider importing and using the ClaimData type definition.For better type safety, consider importing the
ClaimDatainterface from the contract types instead of from test helpers.
19-19: Add JSDoc documentation for helper functions.The helper functions
encodeTaskIdandgetClaimDataSignedwould benefit from JSDoc documentation explaining their purpose, parameters, and return values./** * Encodes a task ID string into a bytes32 hash * @param taskId - The task ID to encode * @returns The keccak256 hash of the encoded task ID */ const encodeTaskId = (taskId: string) => utils.keccak256(utils.defaultAbiCoder.encode(["string"], [taskId])); /** * Creates and signs claim data for testing * @param chainId - The chain ID for the signature * @param verifyingContract - The contract address for the signature * @param signer - The signer's address * @param amount - The reward amount * @param sigExpiration - The signature expiration timestamp * @param taskId - The task ID * @param to - The recipient address * @returns Signed claim data with signature */ const getClaimDataSigned = async (...) => {Also applies to: 21-42
56-81: Enhance test description and add gas assertions.The test name "Should claim" could be more descriptive. Consider adding gas usage assertions to monitor the efficiency of the claim operation.
- it("Should claim", async () => { + it("should successfully claim rewards with valid signature", async () => { // ... existing code ... const tx = instantRewards.claim(claimDataSigned); + const receipt = await (await tx).wait(); + expect(receipt.gasUsed).to.be.lte(200000); // Adjust threshold as needed await expect(tx).to.emit(instantRewards, "Claimed").withArgs(owner.address, taskId, amount); });
302-323: Add more edge cases for withdrawal testing.Consider adding tests for:
- Withdrawing exact contract balance
- Attempting to withdraw more than available balance
- Withdrawing to zero address
it("should revert when withdrawing more than available balance", async () => { await ethers.provider.send("evm_increaseTime", [7200]); await ethers.provider.send("evm_mine", []); const amount = utils.parseEther("1"); await fundContract(amount); const tx = instantRewards.withdraw(user.address, amount.add(1)); await expect(tx).to.be.revertedWith("InsufficientBalance"); }); it("should revert when withdrawing to zero address", async () => { await ethers.provider.send("evm_increaseTime", [7200]); await ethers.provider.send("evm_mine", []); const tx = instantRewards.withdraw(ethers.constants.AddressZero, utils.parseEther("1")); await expect(tx).to.be.revertedWith("InvalidAddress"); });packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol (2)
32-34: Usage ofblock.timestampin Time ComparisonsThe use of
block.timestampin theisActive()andsetTimeframe()functions is acceptable for time-based logic. However, be aware that miners can manipulate the timestamp slightly. Ensure that this potential manipulation does not negatively impact the contract's behavior or security.Also applies to: 36-43
🧰 Tools
🪛 GitHub Check: Slither
[notice] 32-34: Block timestamp
InstantRewardsV2.isActive() (contracts/instant-rewards/InstantRewardsV2.sol#32-34) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp >= start && block.timestamp <= end (contracts/instant-rewards/InstantRewardsV2.sol#33)
6-54: Enhance Code Documentation with NatSpec CommentsConsider adding NatSpec comments to the contract, functions, events, and custom errors to improve code readability and provide clear documentation for users and developers.
🧰 Tools
🪛 GitHub Check: Slither
[notice] 32-34: Block timestamp
InstantRewardsV2.isActive() (contracts/instant-rewards/InstantRewardsV2.sol#32-34) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp >= start && block.timestamp <= end (contracts/instant-rewards/InstantRewardsV2.sol#33)
[notice] 36-43: Block timestamp
InstantRewardsV2.setTimeframe(uint256,uint256) (contracts/instant-rewards/InstantRewardsV2.sol#36-43) uses timestamp for comparisons
Dangerous comparisons:
- start_ < block.timestamp || end_ < block.timestamp (contracts/instant-rewards/InstantRewardsV2.sol#38)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol(2 hunks)packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol(1 hunks)packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol(1 hunks)packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
[warning] 55-67: Low-level calls
Low level call in InstantRewards.claim(InstantRewards.ClaimData) (contracts/instant-rewards/InstantRewards.sol#55-67):
- (success,None) = claimData.to.call{value: claimData.amount}() (contracts/instant-rewards/InstantRewards.sol#63)
[notice] 75-82: Reentrancy vulnerabilities
Reentrancy in InstantRewards.withdraw(address,uint256) (contracts/instant-rewards/InstantRewards.sol#75-82):
External calls:
- (success,None) = wallet.call{value: amount}() (contracts/instant-rewards/InstantRewards.sol#78)
Event emitted after the call(s):
- Withdrawn(wallet,amount) (contracts/instant-rewards/InstantRewards.sol#81)
[warning] 75-82: Low-level calls
Low level call in InstantRewards.withdraw(address,uint256) (contracts/instant-rewards/InstantRewards.sol#75-82):
- (success,None) = wallet.call{value: amount}() (contracts/instant-rewards/InstantRewards.sol#78)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol
[notice] 10-20: Reentrancy vulnerabilities
Reentrancy in InstantRewardsFactory.createInstantRewards(address,uint256,uint256,string) (contracts/instant-rewards/InstantRewardsFactory.sol#10-20):
External calls:
- instantRewards.transferOwnership(owner()) (contracts/instant-rewards/InstantRewardsFactory.sol#17)
Event emitted after the call(s):
- InstantRewardsCreated(address(instantRewards),owner()) (contracts/instant-rewards/InstantRewardsFactory.sol#18)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol
[notice] 32-34: Block timestamp
InstantRewardsV2.isActive() (contracts/instant-rewards/InstantRewardsV2.sol#32-34) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp >= start && block.timestamp <= end (contracts/instant-rewards/InstantRewardsV2.sol#33)
[notice] 36-43: Block timestamp
InstantRewardsV2.setTimeframe(uint256,uint256) (contracts/instant-rewards/InstantRewardsV2.sol#36-43) uses timestamp for comparisons
Dangerous comparisons:
- start_ < block.timestamp || end_ < block.timestamp (contracts/instant-rewards/InstantRewardsV2.sol#38)
🔇 Additional comments (7)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol (3)
1-7: LGTM: Solid foundation with secure contract setup.
The contract uses a recent Solidity version and properly inherits from OpenZeppelin's Ownable2Step, which provides secure two-step ownership transfer.
8-8: LGTM: Well-structured event declaration.
The event properly indexes both the contract address and owner for efficient filtering in event logs.
16-19: Regarding Slither's reentrancy warning: This is a false positive.
The code follows best practices:
- The
transferOwnershipcall is to a trusted OpenZeppelin implementation - The
InstantRewardsV2contract is deployed by us - The event emission after state changes is correct
No changes needed.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (1)
Line range hint 75-82: LGTM: Reentrancy warning is a false positive.
The function is secure because:
- It's protected by
onlyOwner - No state changes occur after the ETH transfer
- Proper balance checks are in place
The event emission after the transfer is safe in this context.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol (3)
18-30: Constructor Validations and Initializations are Appropriate
The constructor correctly initializes state variables and includes necessary validations to prevent invalid configurations, ensuring the contract is set up securely.
45-48: Proper Override of claim Function with Active Period Enforcement
The overridden claim function correctly enforces that claims can only be made during the active reward period, enhancing security and contract integrity.
50-53: Ensuring Withdrawals Only When Rewards are Inactive
The withdraw function appropriately prevents withdrawals while the reward is active, maintaining the intended reward distribution logic.
packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts
Show resolved
Hide resolved
packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts
Show resolved
Hide resolved
andresaiello
force-pushed
the
andy/pd-6808-1-smart-contract-per-ir-activity
branch
from
e076ca4 to
5715168
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (2)
Line range hint
75-82: Add reentrancy protection to withdraw function.The
withdrawfunction requires additional security measures:
- Missing nonReentrant modifier
- Event emission after external call creates reentrancy vulnerability
Apply this diff to fix the issues:
- function withdraw(address wallet, uint256 amount) public virtual onlyOwner { + function withdraw(address wallet, uint256 amount) public virtual nonReentrant onlyOwner { if (wallet == address(0)) revert InvalidAddress(); if (amount > address(this).balance) revert TransferFailed(); + emit Withdrawn(wallet, amount); (bool success, ) = wallet.call{value: amount}(""); if (!success) revert TransferFailed(); - - emit Withdrawn(wallet, amount); }The changes:
- Add nonReentrant modifier to prevent reentrancy
- Move event emission before external call to follow checks-effects-interactions pattern
Line range hint
55-82: Consider using OpenZeppelin's Address library for ETH transfers.Both
claimandwithdrawfunctions use low-level calls for ETH transfers. Consider using OpenZeppelin'sAddress.sendValue()for a more secure and standardized implementation.Import and use the Address library:
import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; import "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; +import "@openzeppelin/contracts/utils/Address.sol"; import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; contract InstantRewards is Ownable2Step, Pausable, ReentrancyGuard, EIP712 { + using Address for address payable;Then replace the low-level calls:
- (bool success, ) = claimData.to.call{value: claimData.amount}(""); - if (!success) revert TransferFailed(); + payable(claimData.to).sendValue(claimData.amount); - (bool success, ) = wallet.call{value: amount}(""); - if (!success) revert TransferFailed(); + payable(wallet).sendValue(amount);Benefits:
- Built-in revert on failure (no need for success check)
- Gas-forwarding protection
- Industry-standard implementation
packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts (1)
289-300: Enhance ownership transfer test to validate permissionsConsider extending the ownership transfer test to verify that the new owner can execute owner-restricted functions, such as pausing the contract, and that the previous owner no longer has these permissions. This will ensure that ownership transitions are correctly enforced.
For example, after the ownership transfer:
// Verify new owner can pause the contract await expect(instantRewards.connect(user).pause()).to.not.be.reverted; // Verify previous owner cannot pause the contract await expect(instantRewards.connect(owner).pause()).to.be.revertedWith("Ownable: caller is not the owner");packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol (1)
7-7: Declarenameasimmutableto Optimize Gas UsageSince
nameis assigned once during contract deployment and does not change thereafter, you can declare it asimmutableto save gas costs and signal its immutability.Apply this diff to implement the change:
-string public name; +string public immutable name;Also applies to: 29-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol(2 hunks)packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol(1 hunks)packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol(1 hunks)packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
[warning] 55-67: Low-level calls
Low level call in InstantRewards.claim(InstantRewards.ClaimData) (contracts/instant-rewards/InstantRewards.sol#55-67):
- (success,None) = claimData.to.call{value: claimData.amount}() (contracts/instant-rewards/InstantRewards.sol#63)
[notice] 75-82: Reentrancy vulnerabilities
Reentrancy in InstantRewards.withdraw(address,uint256) (contracts/instant-rewards/InstantRewards.sol#75-82):
External calls:
- (success,None) = wallet.call{value: amount}() (contracts/instant-rewards/InstantRewards.sol#78)
Event emitted after the call(s):
- Withdrawn(wallet,amount) (contracts/instant-rewards/InstantRewards.sol#81)
[warning] 75-82: Low-level calls
Low level call in InstantRewards.withdraw(address,uint256) (contracts/instant-rewards/InstantRewards.sol#75-82):
- (success,None) = wallet.call{value: amount}() (contracts/instant-rewards/InstantRewards.sol#78)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol
[notice] 10-20: Reentrancy vulnerabilities
Reentrancy in InstantRewardsFactory.createInstantRewards(address,uint256,uint256,string) (contracts/instant-rewards/InstantRewardsFactory.sol#10-20):
External calls:
- instantRewards.transferOwnership(owner()) (contracts/instant-rewards/InstantRewardsFactory.sol#17)
Event emitted after the call(s):
- InstantRewardsCreated(address(instantRewards),owner()) (contracts/instant-rewards/InstantRewardsFactory.sol#18)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol
[notice] 32-34: Block timestamp
InstantRewardsV2.isActive() (contracts/instant-rewards/InstantRewardsV2.sol#32-34) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp >= start && block.timestamp <= end (contracts/instant-rewards/InstantRewardsV2.sol#33)
[notice] 36-43: Block timestamp
InstantRewardsV2.setTimeframe(uint256,uint256) (contracts/instant-rewards/InstantRewardsV2.sol#36-43) uses timestamp for comparisons
Dangerous comparisons:
- start_ < block.timestamp || end_ < block.timestamp (contracts/instant-rewards/InstantRewardsV2.sol#38)
🔇 Additional comments (3)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol (2)
1-7: LGTM! Appropriate contract structure and security measures.
The contract correctly implements Ownable2Step for secure ownership management.
8-9: LGTM! Well-structured event declaration.
The event properly indexes both parameters for efficient filtering in event logs.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (1)
Line range hint 55-67: LGTM: Function visibility change is well-protected.
The change from external to public virtual is appropriate for enabling inheritance while maintaining security:
- NonReentrant guard prevents reentrancy
- Follows checks-effects-interactions pattern
- Proper event emission after state changes
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol
Outdated
Show resolved
Hide resolved
packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts
Outdated
Show resolved
Hide resolved
packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol
Outdated
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol (3)
8-16: Add event for allowPublicCreation changes.For better transparency and off-chain tracking, emit an event when the public creation setting changes.
contract InstantRewardsFactory is Ownable2Step { bool public allowPublicCreation = false; error AccessDenied(); error InvalidSignerAddress(); error EmptyName(); error StartTimeInPast(); error EndTimeBeforeStart(); + event PublicCreationUpdated(bool indexed allowed); event InstantRewardsCreated(address indexed instantRewards, address indexed owner);
22-24: Emit event for allowPublicCreation changes.Emit the suggested PublicCreationUpdated event when the setting changes.
function setAllowPublicCreation(bool allowPublicCreation_) external onlyOwner { allowPublicCreation = allowPublicCreation_; + emit PublicCreationUpdated(allowPublicCreation_); }
26-31: Add NatSpec documentation.Add comprehensive NatSpec documentation to improve code maintainability and developer experience.
+ /// @notice Creates a new InstantRewardsV2 contract + /// @param signerAddress The address authorized to sign reward claims + /// @param start The timestamp when rewards can start being claimed + /// @param end The timestamp when rewards can no longer be claimed + /// @param name The name of the rewards contract + /// @return The address of the newly created InstantRewardsV2 contract function createInstantRewards( address signerAddress, uint256 start, uint256 end, string memory name ) external returns (address) {🧰 Tools
🪛 GitHub Check: Slither
[notice] 26-44: Block timestamp
InstantRewardsFactory.createInstantRewards(address,uint256,uint256,string) (contracts/instant-rewards/InstantRewardsFactory.sol#26-44) uses timestamp for comparisons
Dangerous comparisons:
- start < block.timestamp (contracts/instant-rewards/InstantRewardsFactory.sol#34)packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2.ts (2)
15-21: Add assertions for critical setup steps.While the setup is functional, it's missing validations for critical steps that could fail silently.
Consider adding these assertions:
beforeEach(async () => { [deployer, owner, signer, user, ...addrs] = await ethers.getSigners(); const instantRewardsFactoryF = await ethers.getContractFactory("InstantRewardsFactory"); instantRewardsFactory = (await instantRewardsFactoryF.deploy(owner.address)) as InstantRewardsFactory; await instantRewardsFactory.deployed(); + expect(await instantRewardsFactory.pendingOwner()).to.equal(owner.address); await instantRewardsFactory.connect(owner).acceptOwnership(); + expect(await instantRewardsFactory.owner()).to.equal(owner.address); });
45-52: Enhance access control test coverage.The test case could be more explicit and cover additional edge cases.
Consider these improvements:
-it("Should revert if not owner try to deploy", async () => { +it("Should revert if non-owner tries to deploy", async () => { const currentBlock = await ethers.provider.getBlock("latest"); const start = currentBlock.timestamp + 1000; const end = start + 1000; const name = "Instant Rewards"; - const tx = instantRewardsFactory.createInstantRewards(signer.address, start, end, name); + const tx = instantRewardsFactory.connect(user).createInstantRewards(signer.address, start, end, name); await expect(tx).to.revertedWith("AccessDenied"); + + // Test with zero address signer + await expect( + instantRewardsFactory.connect(owner).createInstantRewards(ethers.constants.AddressZero, start, end, name) + ).to.be.revertedWith("InvalidSignerAddress"); });packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts (2)
21-42: Consider enhancing error handling in getClaimDataSigned helper.The helper function should validate input parameters before processing to prevent potential issues.
const getClaimDataSigned = async ( chainId: number, verifyingContract: string, signer: SignerWithAddress, amount: BigNumber, sigExpiration: number, taskId: string, to: string ) => { + if (amount.lte(0)) throw new Error("Amount must be greater than 0"); + if (sigExpiration <= 0) throw new Error("Signature expiration must be greater than 0"); + if (!taskId || !to) throw new Error("TaskId and to address are required"); const claimData: ClaimData = { amount, sigExpiration, taskId, to, }; const signature = await getSignature(chainId, verifyingContract, signer, claimData); return { ...claimData, signature, }; };
56-243: Consider organizing tests using nested describe blocks.The test suite would benefit from better organization using nested describe blocks to group related test cases (e.g., "claim functionality", "error cases", etc.).
Example structure:
describe("InstantRewardsV2", () => { describe("claim functionality", () => { it("Should claim successfully", async () => { // ... existing test }); it("Should claim after pause and unpause", async () => { // ... existing test }); }); describe("error cases", () => { it("Should revert on invalid claims", async () => { // ... existing test }); // ... other error cases }); });packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol (3)
25-25: Redundant Signer Address ValidationThe check
if (signerAddress_ == address(0)) revert InvalidAddress();duplicates validation that may already exist in the parentInstantRewardscontract's constructor. Review the parent contract to determine if this validation is necessary here. Removing redundant checks can simplify the code and reduce gas costs.
36-43: Potential Issue with Timeframe Updates During Inactive PeriodsThe
setTimeframefunction prevents updating the timeframe if the reward is currently active by reverting withInstantRewardStillActive. This could limit administrative flexibility. Consider allowing updates to theendtime to extend the active period or providing a mechanism to deactivate the reward early if necessary.Apply this diff to allow adjustments to the
endtime during the active period:function setTimeframe(uint256 start_, uint256 end_) external onlyOwner { if (start_ > end_) revert InvalidTimeframe(); - if (start_ < block.timestamp || end_ < block.timestamp) revert InvalidTimeframe(); - if (isActive()) revert InstantRewardStillActive(); + if (start_ < block.timestamp) revert InvalidTimeframe(); start = start_; end = end_; emit TimeframeUpdated(start_, end_); }
45-48: Ensure Proper Handling of Inactive ClaimsIn the
claimfunction, when the reward is inactive, the transaction reverts withInstantRewardNotActive. To enhance user experience, consider providing a more informative message or implementing an event that notifies users when claims are attempted outside the active period.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol(1 hunks)packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol(1 hunks)packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts(1 hunks)packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol
[notice] 26-44: Block timestamp
InstantRewardsFactory.createInstantRewards(address,uint256,uint256,string) (contracts/instant-rewards/InstantRewardsFactory.sol#26-44) uses timestamp for comparisons
Dangerous comparisons:
- start < block.timestamp (contracts/instant-rewards/InstantRewardsFactory.sol#34)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol
[notice] 32-34: Block timestamp
InstantRewardsV2.isActive() (contracts/instant-rewards/InstantRewardsV2.sol#32-34) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp >= start && block.timestamp <= end (contracts/instant-rewards/InstantRewardsV2.sol#33)
🔇 Additional comments (6)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol (2)
1-7: LGTM! Solid foundation with secure ownership management.
The contract uses the latest Solidity version and implements secure ownership transfer through Ownable2Step.
26-44: Implementation approved with a note on reentrancy.
The function implements comprehensive input validation and proper access control. While there's a theoretical reentrancy possibility due to the event emission after transferOwnership, the risk is minimal as:
- Only the owner's address is used
- No state changes occur after the external call
- Event emission after external calls is safe in this context
✅ Verification successful
Implementation is secure with proper reentrancy protection
The verification reveals that the implementation is more secure than initially assessed:
- The base
InstantRewardscontract inherits fromReentrancyGuard, providing protection against reentrancy attacks Ownable2Stepis properly implemented in both factory and reward contracts, ensuring secure ownership transfers- The event emission after
transferOwnershipis safe because:Ownable2Steprequires a two-step ownership transfer- The base contract includes
ReentrancyGuard - The factory maintains strict access control
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if the contract is used in other parts of the codebase
rg "new InstantRewardsFactory" -A 2
Length of output: 37
Script:
#!/bin/bash
# Let's verify the inheritance and usage patterns more comprehensively
# Check for InstantRewardsV2 contract references
rg "InstantRewardsV2" -A 2
# Check for InstantRewardsFactory contract references
rg "InstantRewardsFactory" -A 2
# Look for any potential reentrancy patterns in InstantRewardsV2
ast-grep --pattern 'function transferOwnership($$$) { $$$ }'
Length of output: 5255
Script:
#!/bin/bash
# Let's check the InstantRewards base contract and Ownable2Step implementation
# Check InstantRewards base contract
rg "contract InstantRewards" -A 5
# Check if there are any other state changes in InstantRewardsV2
ast-grep --pattern 'contract InstantRewardsV2 {
$$$
}'
# Look for the Ownable2Step implementation
rg "Ownable2Step" -A 5
Length of output: 5609
🧰 Tools
🪛 GitHub Check: Slither
[notice] 26-44: Block timestamp
InstantRewardsFactory.createInstantRewards(address,uint256,uint256,string) (contracts/instant-rewards/InstantRewardsFactory.sol#26-44) uses timestamp for comparisons
Dangerous comparisons:
- start < block.timestamp (contracts/instant-rewards/InstantRewardsFactory.sol#34)
packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2.ts (1)
1-13: LGTM! Clean imports and type-safe setup.
The import statements and type declarations are well-structured, utilizing TypeScript's type system effectively through typechain-generated types.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol (3)
32-34: Usage of Block Timestamp for Time Comparison
The isActive function relies on block.timestamp for time comparisons. While this is common practice, it's important to be aware that miners can manipulate the timestamp within a permissible range. This could potentially affect the contract's behavior in edge cases. Consider using a more reliable time oracle if absolute precision is required.
🧰 Tools
🪛 GitHub Check: Slither
[notice] 32-34: Block timestamp
InstantRewardsV2.isActive() (contracts/instant-rewards/InstantRewardsV2.sol#32-34) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp >= start && block.timestamp <= end (contracts/instant-rewards/InstantRewardsV2.sol#33)
50-53: Finalize Reward Period Before Withdrawal
The withdraw function correctly restricts withdrawals while the reward is active by checking if (isActive()) revert InstantRewardStillActive();. This ensures that funds are not prematurely removed from the contract, maintaining the integrity of the reward distribution process.
14-16: Consistent Custom Error Definitions
The custom errors InvalidTimeframe, InstantRewardNotActive, and InstantRewardStillActive enhance clarity and gas efficiency. Ensure that all potential revert reasons throughout the contract use custom errors for consistency and optimal gas usage.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol
Show resolved
Hide resolved
packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts
Show resolved
Hide resolved
packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts (1)
29-37: Improve error handling and remove duplicate validation.The current implementation has duplicate network validation and basic error handling.
const main = async () => { - if (!isProtocolNetworkName(networkName)) throw new Error("Invalid network name"); await deployInstantRewards(); }; main().catch((error) => { - console.error(error); + console.error(`Deployment failed at ${new Date().toISOString()}:`); + console.error(`Network: ${networkName}`); + console.error(`Error: ${error.message}`); + if (error.stack) console.error(`Stack: ${error.stack}`); process.exit(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/zevm-app-contracts/data/addresses.json(1 hunks)packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts(1 hunks)
🔇 Additional comments (3)
packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts (1)
1-8: LGTM! Imports and setup are well-structured.
The necessary dependencies are properly imported, including protocol-specific utilities and type definitions.
packages/zevm-app-contracts/data/addresses.json (2)
Line range hint 27-29: Review mainnet address synchronization.
The mainnet configuration shows updates to ZetaXP and ProofOfLiveness addresses. Given the PR's objective of refactoring InstantRewards, please verify if the mainnet InstantRewards contract should also be updated to reflect the new implementation.
Let's check for any pending mainnet deployments:
#!/bin/bash
# Description: Check for pending mainnet deployments
# Expected: Should find deployment preparations or discussions
# Search for mainnet deployment references
echo "Searching for mainnet deployment references..."
rg -l "mainnet.*deployment" --type md11-11: Verify the new InstantRewards testnet deployment.
The InstantRewards contract address has been updated in the testnet configuration. Given the critical nature of contract address changes, it's essential to verify this deployment.
Let's verify the contract deployment and implementation:
✅ Verification successful
InstantRewards contract address update appears valid and well-implemented
The address change in testnet reflects a proper upgrade to InstantRewardsV2, which adds important new features:
- Time-bound rewards with start/end timestamps
- Named reward programs
- Enhanced security with active period restrictions
- Factory pattern implementation for controlled deployments
The deployment process is handled through a well-structured factory contract with proper access controls and validation checks. The implementation shows this is a planned upgrade rather than a simple redeployment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new InstantRewards contract deployment on testnet
# Expected: Should find deployment transaction and contract verification status
# Search for deployment transaction
echo "Searching for contract deployment transaction..."
gh api \
-H "Accept: application/vnd.github+json" \
-H "X-GitHub-Api-Version: 2022-11-28" \
"/repos/zeta-chain/app-contracts/commits" \
--jq '.[] | select(.commit.message | contains("0xd91164c9671C5A2ee1C95fa34A95C9141dA691D4"))'
Length of output: 432
Script:
#!/bin/bash
# Let's verify the contract address changes by:
# 1. Looking for deployment references in code/docs
# 2. Checking for contract verification details
# 3. Examining related configuration changes
# Search for any references to this contract address
echo "Searching for contract address references..."
rg -i "0xd91164c9671C5A2ee1C95fa34A95C9141dA691D4"
# Look for deployment scripts or configurations
echo -e "\nSearching for deployment configurations..."
fd -e ts -e js -e json . | xargs rg -l "InstantRewards"
# Check for contract verification details
echo -e "\nSearching for verification details..."
fd -e md . | xargs rg -l "InstantRewards.*testnet"
Length of output: 1032
Script:
#!/bin/bash
# Let's examine the deployment scripts and contract implementation
# Check the deployment script content
echo "Examining deployment scripts..."
cat packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts
# Look for contract implementation changes
echo -e "\nChecking contract implementation..."
fd -e sol . | xargs rg -l "InstantRewards"
# Check test files for contract changes
echo -e "\nExamining test files for contract changes..."
cat packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2.ts
Length of output: 4758
Script:
#!/bin/bash
# Let's examine the contract implementation to understand the changes
# Check the V2 contract implementation
echo "Examining InstantRewardsV2 contract..."
cat packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol
# Check the factory contract
echo -e "\nExamining InstantRewardsFactory contract..."
cat packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol
# Look for any migration scripts
echo -e "\nSearching for migration scripts..."
fd -e ts -e js . | xargs rg -l "migrate.*InstantRewards|upgrade.*InstantRewards"
Length of output: 3656
andresaiello
deleted the
andy/pd-6808-1-smart-contract-per-ir-activity
branch
Summary
Summary by CodeRabbit
Release Notes
New Features
InstantRewardsFactorycontract for easier creation of instant rewards contracts.InstantRewardsV2with enhanced reward management, including customizable reward timeframes.Improvements
claimandwithdrawinInstantRewardsto allow more flexible usage.InstantRewardsV2.Tests
InstantRewardsV2andInstantRewardsFactoryto ensure functionality and security.Address Updates
InstantRewardsand other entities in thezeta_testnetandzeta_mainnetconfigurations.