feat: Instant rewards smart contract #171
Conversation
WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InstantRewards
participant ECDSA
User->>InstantRewards: claim(claimData)
InstantRewards->>ECDSA: verify(signature)
ECDSA-->>InstantRewards: valid / invalid
alt valid
InstantRewards->>InstantRewards: update claim status
InstantRewards->>User: transfer(amount)
InstantRewards->>User: emit Claimed event
else invalid
InstantRewards->>User: revert transaction
end
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 as PR comments)
Additionally, you can add 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/InstantRewards.sol
Fixed
Show fixed
Hide fixed
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Fixed
Show fixed
Hide fixed
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Fixed
Show fixed
Hide fixed
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (1 hunks)
Additional context used
GitHub Check: Slither
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
[warning] 41-60: Dead-code
InstantRewards._uint2str(uint256) (contracts/instant-rewards/InstantRewards.sol#41-60) is never used and should be removed
[warning] 84-95: Reentrancy vulnerabilities
Reentrancy in InstantRewards.claim(InstantRewards.ClaimData) (contracts/instant-rewards/InstantRewards.sol#84-95):
External calls:
- address(claimData.to).transfer(claimData.amount) (contracts/instant-rewards/InstantRewards.sol#92)
Event emitted after the call(s):
- Claimed(claimData.to,claimData.taskId,claimData.amount) (contracts/instant-rewards/InstantRewards.sol#94)
[notice] 102-102: Missing zero address validation
InstantRewards.withdraw(address).wallet (contracts/instant-rewards/InstantRewards.sol#102) lacks a zero-check on :
- address(wallet).transfer(address(this).balance) (contracts/instant-rewards/InstantRewards.sol#103)
Additional comments not posted (5)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (5)
9-38: Contract Initialization is Appropriate.The use of OpenZeppelin libraries for security and access control is well-implemented. The constructor correctly initializes the signer address and transfers ownership.
62-75: Signature Verification Logic is Correct.The
_verifyfunction correctly implements signature verification using ECDSA, ensuring the authenticity of claims.
77-82: Hash Calculation Logic is Correct.The
_calculateHashfunction correctly encodes the claim data and computes the keccak256 hash for signature verification.
97-100: Signer Address Update Logic is Correct.The
setSignerAddressfunction correctly checks for a zero address and updates the signer address.
106-107: Receive Function is Correctly Implemented.The
receivefunction is correctly implemented to allow the contract to receive Ether.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Outdated
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Outdated
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Outdated
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (1 hunks)
Additional context used
GitHub Check: Slither
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
[notice] 43-57: Block timestamp
InstantRewards._verify(InstantRewards.ClaimData) (contracts/instant-rewards/InstantRewards.sol#43-57) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > claimData.sigExpiration (contracts/instant-rewards/InstantRewards.sol#56)
Additional comments not posted (6)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (6)
37-41: Constructor and State Variables Initialization Approved.The constructor correctly initializes the signer address and owner with necessary zero address checks. This ensures that the contract is set up securely.
43-57: Consider Alternatives toblock.timestampfor Expiration Checks.Using
block.timestampfor expiration checks can be risky due to potential miner manipulation. If possible, consider using a more reliable time source, though it is commonly used in Solidity contracts.Tools
GitHub Check: Slither
[notice] 43-57: Block timestamp
InstantRewards._verify(InstantRewards.ClaimData) (contracts/instant-rewards/InstantRewards.sol#43-57) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > claimData.sigExpiration (contracts/instant-rewards/InstantRewards.sol#56)
59-69: Hash Calculation Function Approved.The
_calculateHashfunction correctly useskeccak256to hash the claim data, ensuring data integrity.
85-88: Signer Address Setter Function Approved.The
setSignerAddressfunction is correctly implemented with a zero address check, ensuring secure updates.
90-94: Withdraw Function Approved.The
withdrawfunction is correctly implemented with zero address and balance checks, ensuring secure fund transfers.
96-97: Receive Function Approved.The
receivefunction is correctly implemented, allowing the contract to accept Ether.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Show resolved
Hide resolved
andresaiello
force-pushed
the
andy/pd-6524-instantrewards-smart-contract
branch
from
64d8e10
to
d8f3834
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (1 hunks)
Additional context used
GitHub Check: Slither
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
[notice] 43-57: Block timestamp
InstantRewards._verify(InstantRewards.ClaimData) (contracts/instant-rewards/InstantRewards.sol#43-57) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > claimData.sigExpiration (contracts/instant-rewards/InstantRewards.sol#56)
Additional comments not posted (6)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (6)
37-41: Constructor Validation and Initialization.The constructor correctly initializes the signer address and transfers ownership. Ensure that the owner address is valid and intended.
59-68: Hash Calculation is Correct.The
_calculateHashfunction correctly encodes and hashes the claim data. Ensure that the encoding order matches the signing process.
85-88: Signer Address Update is Secure.The
setSignerAddressfunction includes a zero address check, ensuring security when updating the signer address.
90-94: Withdraw Function is Secure with Zero Address Check.The
withdrawfunction includes a zero address check and balance verification, ensuring secure fund transfers.
96-97: Receive Function for Ether Transfers.The
receivefunction allows the contract to accept Ether. Ensure that this is intended and that there are no unintended consequences.
71-83: Refactor to Prevent Reentrancy and Avoid Low-Level Calls.The
claimfunction has a potential reentrancy vulnerability due to the external call before updating the state. Update the state before making the external call to prevent this issue. Additionally, prefer usingtransferorsendover low-level calls for safety.Apply this diff to refactor the function:
function claim(ClaimData memory claimData) external whenNotPaused nonReentrant { claimData.to = msg.sender; _verify(claimData); if (taskCompletedByUser[claimData.to][claimData.taskId]) revert TaskAlreadyClaimed(); + taskCompletedByUser[claimData.to][claimData.taskId] = true; - (bool success, ) = claimData.to.call{value: claimData.amount}(""); + payable(claimData.to).transfer(claimData.amount); emit Claimed(claimData.to, claimData.taskId, claimData.amount); }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/zevm-app-contracts/test/instant-rewards/instant-rewards.ts (1 hunks)
- packages/zevm-app-contracts/test/instant-rewards/test.helpers.ts (1 hunks)
Additional comments not posted (5)
packages/zevm-app-contracts/test/instant-rewards/test.helpers.ts (2)
5-20: Interfaces are well-defined.The
Signature,ClaimData, andClaimDataSignedinterfaces are correctly structured for their intended purposes.
22-33: Ensure payload encoding aligns with contract expectations.The
getSignaturefunction encodes the payload and signs it. Ensure that the encoding order and types match the contract's expectations for signature verification.Run the following script to verify the encoding alignment with the contract:
Verification successful
Payload Encoding Alignment Verified
The encoding order and types in the
getSignaturefunction align with the contract's expectations for signature verification. The Solidity contract's_calculateHashfunction uses the same encoding order and types as the TypeScript function. No changes are needed.
- TypeScript
getSignaturefunction:["address", "uint256", "bytes32", "uint256"]- Solidity
_calculateHashfunction:abi.encode(claimData.to, claimData.sigExpiration, claimData.taskId, claimData.amount)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the payload encoding in `getSignature` matches the contract expectations. # Test: Search for the contract's signature verification logic. Expect: Encoding order and types match. rg --type solidity -A 10 $'function verifySignature'Length of output: 86
Script:
#!/bin/bash # Description: Search for functions related to signature verification or payload decoding in Solidity files. # Find Solidity files and search for functions that might be related to signature verification. fd --type f --extension sol --exec rg -A 10 'function ' {}Length of output: 31768
packages/zevm-app-contracts/test/instant-rewards/instant-rewards.ts (3)
16-16: Helper functionencodeTaskIdis correctly implemented.The
encodeTaskIdfunction useskeccak256to hash the task ID, which is appropriate for generating unique identifiers.
27-55: Test case for claiming rewards is comprehensive.The test case for the
claimfunction covers the essential steps, including signature generation and event emission verification. Ensure that edge cases, such as expired signatures and incorrect amounts, are also tested in other test cases.
57-67: Ownership transfer test is well-structured.The test case for transferring ownership correctly verifies the change in ownership. Consider adding a test for unauthorized ownership transfer attempts.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Dismissed
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (1 hunks)
- packages/zevm-app-contracts/test/instant-rewards/instant-rewards.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/zevm-app-contracts/test/instant-rewards/instant-rewards.ts
Additional context used
GitHub Check: Slither
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
[notice] 44-58: Block timestamp
InstantRewards._verify(InstantRewards.ClaimData) (contracts/instant-rewards/InstantRewards.sol#44-58) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > claimData.sigExpiration (contracts/instant-rewards/InstantRewards.sol#57)
[warning] 91-96: Reentrancy vulnerabilities
Reentrancy in InstantRewards.withdraw(address,uint256) (contracts/instant-rewards/InstantRewards.sol#91-96):
External calls:
- address(wallet).transfer(amount) (contracts/instant-rewards/InstantRewards.sol#94)
Event emitted after the call(s):
- Withdrawn(wallet,amount) (contracts/instant-rewards/InstantRewards.sol#95)
Additional comments not posted (8)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (8)
4-7: Imports are appropriate.The imported OpenZeppelin libraries are well-suited for the functionalities implemented in the contract, providing essential security features.
9-35: Contract declaration and state variables are well-structured.The contract is appropriately structured with necessary state variables, events, and custom errors. Ensure the signer address is correctly initialized and managed.
38-42: Constructor is well-implemented.The constructor correctly initializes the signer address and transfers ownership, with a zero address check in place.
60-70: Hash calculation is correctly implemented.The
_calculateHashfunction usesabi.encodefor hashing, which is appropriate for computing the keccak256 hash.
86-89: Signer address update is correctly implemented.The
setSignerAddressfunction includes a zero address check, ensuring safe updates.
98-104: Pause and unpause functions are correctly implemented.The
pauseandunpausefunctions are well-implemented, utilizing OpenZeppelin's Pausable contract.
106-107: Receive function is correctly implemented.The
receivefunction allows the contract to accept Ether, which is implemented correctly.
72-84: Refactor to prevent reentrancy and avoid low-level calls.The
claimfunction has a potential reentrancy vulnerability due to the external call before updating the state. Update the state before making the external call to prevent this issue. Additionally, prefer usingtransferorsendover low-level calls for safety.function claim(ClaimData memory claimData) external whenNotPaused nonReentrant { claimData.to = msg.sender; _verify(claimData); if (taskCompletedByUser[claimData.to][claimData.taskId]) revert TaskAlreadyClaimed(); + taskCompletedByUser[claimData.to][claimData.taskId] = true; - (bool success, ) = claimData.to.call{value: claimData.amount}(""); + payable(claimData.to).transfer(claimData.amount); emit Claimed(claimData.to, claimData.taskId, claimData.amount); }Likely invalid or redundant comment.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/zevm-app-contracts/test/instant-rewards/instant-rewards.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/zevm-app-contracts/test/instant-rewards/instant-rewards.ts
Summary
Summary by CodeRabbit
InstantRewardscontract, allowing users to claim rewards based on verified signatures.InstantRewardscontract, covering claim functionality, paused claims, and ownership transfer.