DisputeKit with token gating #1804
Conversation
WalkthroughThe pull request introduces a new smart contract Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🔇 Additional comments (6)
🪧 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 (
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
|
![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
🧹 Nitpick comments (4)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (4)
18-23: Ensure IToken interface is clearly documented and tested.
The IToken interface usage is straightforward, but ensure that you have corresponding tests to verify that calls to balanceOf() do not revert and handle edge cases (such as token contracts that implement custom logic).
70-73: Evaluate constants for potential configurability.
The multipliers (WINNER_STAKE_MULTIPLIER, LOSER_STAKE_MULTIPLIER, etc.) are hardcoded. Depending on governance requirements, you might want to allow these values to be adjusted in the future or make them configurable by the governor.
136-149: Modifiers appear consistent but consider dedicated revert messages for each.
While the existing revert strings are descriptive, they could be refined to clarify which contract, function, or condition has failed. This can speed up debugging.
628-638: Token gating logic in _postDrawCheck is correct but check edge cases.
You are verifying that the juror’s tokenGate balance is > 0. Ensure that corner cases (e.g., tokens with deflationary mechanisms, transfer fees, or off-chain bridging) don’t cause unintended gating failures. Thorough testing is recommended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(1 hunks)
🔇 Additional comments (2)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (2)
160-168: Initialize function covers the essential references.
The initialize function correctly sets governor, core, and tokenGate. Ensure tests verify that these values are not overwritten multiple times except via governance calls.
421-466: Ensure safe usage of .send() during refunds.
Using .send() is a deliberate choice to avoid revert loops. Verify that you handle the scenario where .send() fails (returns false) due to a custom fallback or insufficient gas. Consider emitting an event or adopting the “withdraw” pattern.
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jaybuidl
force-pushed
the
feat/gated-dk
branch
from
a9ce7a7 to
7308fd8
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (3)
449-493: Ensure safe usage ofsendinwithdrawFeesAndRewardsThe function uses
_beneficiary.send(amount);to transfer ETH. While the use ofsendmitigates reentrancy by forwarding limited gas, it could fail silently if the recipient reverts or if the gas stipend is insufficient.Consider using
callwith a checks-effects-interactions pattern and handle potential failures appropriately:round.contributions[_beneficiary][_choice] = 0; if (amount != 0) { - _beneficiary.send(amount); // Deliberate use of send to prevent reverting fallback. It's the user's responsibility to accept ETH. + (bool success, ) = _beneficiary.call{value: amount}(""); + require(success, "Transfer failed."); emit Withdrawal(_coreDisputeID, _coreRoundID, _choice, _beneficiary, amount); }Note: Ensure reentrancy is prevented by updating the state before external calls and consider the implications of reverting on failed transfers.
383-448: Reentrancy considerations infundAppealAlthough previous discussions mention that
coreis a trusted contract, it's good practice to protect against potential reentrancy, especially when external calls are involved.Consider adding the
nonReentrantmodifier from OpenZeppelin'sReentrancyGuardto critical functions likefundAppealto enhance security.+import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; + contract DisputeKitGated is IDisputeKit, Initializable, UUPSProxiable, ReentrancyGuard { // ... - function fundAppeal(uint256 _coreDisputeID, uint256 _choice) external payable notJumped(_coreDisputeID) { + function fundAppeal(uint256 _coreDisputeID, uint256 _choice) external payable notJumped(_coreDisputeID) nonReentrant { // Function body } }
351-354: Enhance commit scheme security incastVoteIn the commit-reveal scheme, including the voter's address in the commitment can prevent certain attacks like vote replays.
Consider modifying the commit to include the voter's address:
require( !hiddenVotes || round.votes[_voteIDs[i]].commit == keccak256(abi.encodePacked(_choice, _salt)), "The commit must match the choice in courts with hidden votes." );Change to:
require( !hiddenVotes || round.votes[_voteIDs[i]].commit == keccak256(abi.encodePacked(_choice, _salt, msg.sender)), "The commit must match the choice in courts with hidden votes." );Note: This change would require voters to include their address when creating their commit during the
castCommitphase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(1 hunks)
🔇 Additional comments (2)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (2)
647-654: Consider potential issues with IERC20OrERC721balanceOfusageThe
IERC20OrERC721interface uses thebalanceOffunction for both ERC20 and ERC721 tokens. While ERC20'sbalanceOfreturns the token amount, ERC721'sbalanceOfreturns the number of NFTs owned. Ensure that usingbalanceOfwithout differentiation does not introduce logical errors, especially if the juror must own a specific NFT.Please confirm that this implementation correctly enforces the token gating mechanism for both ERC20 and ERC721 tokens.
656-668: Check for potential race conditions during contract references updatePrevious reviews highlighted potential race conditions when updating
coreandtokenGatereferences.Ensure that transitional states are handled safely when changing critical contract addresses to prevent inconsistent behavior.
jaybuidl
force-pushed
the
feat/gated-dk
branch
from
7308fd8 to
c0f2f89
Compare
|
Code Climate has analyzed commit c0f2f89 and detected 0 issues on this pull request. View more on Code Climate. |
|
A juror must have a non-zero balance of
tokenGateto be drawn.It is enforced by
_postDrawCheck()here.PR-Codex overview
This PR introduces the
DisputeKitGatedcontract, an implementation of a dispute kit that leverages token gating for participation. It incorporates structures for managing disputes, rounds, and votes while offering governance and funding mechanisms.Detailed summary
DisputeKitGatedcontract implementingIDisputeKit.Dispute,Round, andVotestructs for dispute management.Summary by CodeRabbit
New Features
Bug Fixes