Skip to content

Contracts: use SafeSend for ETH transfers #2041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 1, 2025
Merged

Contracts: use SafeSend for ETH transfers #2041

merged 3 commits into from
Aug 1, 2025

Conversation

thasmi21
Copy link
Contributor

@thasmi21 thasmi21 commented Jul 16, 2025

Adding the SafeSend library for usage in KlerosCore* and in DisputeKit*.

The Ruler and University contracts are excluded for simplicity since they are not production contracts.


PR-Codex overview

This PR focuses on enhancing the DisputeKitClassic and related contracts by adding support for a wrapped native token (wNative, typically wETH) in various functions, improving the handling of ETH payments through a new SafeSend library for safer transactions.

Detailed summary

  • Updated deployment scripts to include weth.target as an argument for DisputeKitClassic.
  • Modified initialize functions in several contracts to accept _wNative.
  • Introduced SafeSend library for safe ETH transfers.
  • Updated payment handling in KlerosCoreBase, KlerosGovernor, and DisputeKitClassicBase to use safeSend().
  • Added wNative state variable to relevant contracts for managing wrapped native token interactions.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Summary by CodeRabbit

  • New Features

    • Introduced safer native currency transfers with automatic fallback to wrapped tokens (e.g., WETH) if direct transfers fail.
    • Added public visibility for the wrapped native token address in relevant contracts.
    • Deployment and initialization processes now require specifying the wrapped native token address.
  • Bug Fixes

    • Improved reliability of native currency transfers to users and beneficiaries.
  • Tests

    • Updated test suite to support and verify the new wrapped native token logic.

@thasmi21 thasmi21 requested a review from jaybuidl as a code owner July 16, 2025 12:50
Copy link

netlify bot commented Jul 16, 2025

Deploy Preview for kleros-v2-university failed. Why did it fail? →

Name Link
🔨 Latest commit c5a894a
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-university/deploys/688cadf3bfc9f40008cdbb99

Copy link

netlify bot commented Jul 16, 2025

Deploy Preview for kleros-v2-testnet ready!

Name Link
🔨 Latest commit c5a894a
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-testnet/deploys/688cadf3471f27000854de98
😎 Deploy Preview https://deploy-preview-2041--kleros-v2-testnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link

netlify bot commented Jul 16, 2025

Deploy Preview for kleros-v2-testnet-devtools ready!

Name Link
🔨 Latest commit c5a894a
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-testnet-devtools/deploys/688cadf3ce285d000894554c
😎 Deploy Preview https://deploy-preview-2041--kleros-v2-testnet-devtools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Contributor

coderabbitai bot commented Jul 16, 2025

Walkthrough

A new SafeSend library was introduced to safely transfer native tokens with a fallback to wrapped tokens (e.g., WETH) in case of failure. Multiple arbitration and dispute kit contracts were updated to use this library, requiring a wNative address during initialization. Deployment scripts and related tests were updated to handle the new parameter.

Changes

Cohort / File(s) Change Summary
SafeSend Library Introduction
contracts/src/libraries/SafeSend.sol
Introduced the SafeSend library and WethLike interface to enable safe native token transfers with fallback to wrapped tokens.
KlerosCoreBase & Integration
contracts/src/arbitration/KlerosCoreBase.sol, contracts/src/arbitration/KlerosCore.sol, contracts/src/arbitration/KlerosCoreNeo.sol
Added wNative state variable, updated initializer functions to accept wNative, and replaced direct native transfers with SafeSend.safeSend.
Dispute Kits
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol, contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol, contracts/src/arbitration/dispute-kits/DisputeKitGated.sol, contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol, contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol, contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol
Added wNative variable and parameter to initializers; replaced direct .send() calls with SafeSend.safeSend where applicable. All dispute kit initializers now require the wrapped native token address.
KlerosGovernor
contracts/src/arbitration/KlerosGovernor.sol
Added wNative state variable, updated constructor to accept it, and replaced .send calls with SafeSend.safeSend.
Deployment Scripts
contracts/deploy/00-home-chain-arbitration.ts, contracts/deploy/00-home-chain-arbitration-neo.ts, contracts/deploy/00-home-chain-arbitration-university.ts
Updated contract deployment arguments to include the wNative (WETH) address for all relevant contracts and dispute kits.
Foundry Tests
contracts/test/foundry/KlerosCore.t.sol
Added wNative ERC20 mock, updated test setup and initialization calls to include the new parameter for dispute kits and core contract.

Sequence Diagram(s)

sequenceDiagram
    participant Contract as AnyContract (e.g., KlerosCoreBase)
    participant SafeSend as SafeSend Library
    participant WETH as WETH Contract
    participant Recipient as Recipient

    Contract->>SafeSend: safeSend(recipient, value, wNative)
    alt Native send succeeds
        SafeSend-->>Recipient: ETH transfer
    else Native send fails
        SafeSend->>WETH: deposit() with value
        SafeSend->>WETH: transfer(recipient, value)
        WETH-->>Recipient: WETH transfer
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~15–25 minutes

Possibly related PRs

Poem

A bunny hops with code anew,
Wrapped tokens now for safety’s view!
ETH or WETH, you’ll get your pay,
Even if the old send fails today.
Deployments tweaked, dispute kits too—
This rabbit cheers the smart review!
🐇✨

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a92ddcf and c5a894a.

📒 Files selected for processing (15)
  • contracts/deploy/00-home-chain-arbitration-neo.ts (2 hunks)
  • contracts/deploy/00-home-chain-arbitration-university.ts (1 hunks)
  • contracts/deploy/00-home-chain-arbitration.ts (3 hunks)
  • contracts/src/arbitration/KlerosCore.sol (3 hunks)
  • contracts/src/arbitration/KlerosCoreBase.sol (6 hunks)
  • contracts/src/arbitration/KlerosCoreNeo.sol (3 hunks)
  • contracts/src/arbitration/KlerosGovernor.sol (7 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (6 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1 hunks)
  • contracts/src/libraries/SafeSend.sol (1 hunks)
  • contracts/test/foundry/KlerosCore.t.sol (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
  • contracts/deploy/00-home-chain-arbitration-university.ts
  • contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol
  • contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol
  • contracts/deploy/00-home-chain-arbitration.ts
  • contracts/src/arbitration/KlerosCoreBase.sol
  • contracts/src/libraries/SafeSend.sol
  • contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol
  • contracts/deploy/00-home-chain-arbitration-neo.ts
  • contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
  • contracts/src/arbitration/KlerosCore.sol
  • contracts/src/arbitration/KlerosGovernor.sol
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
  • contracts/src/arbitration/KlerosCoreNeo.sol
  • contracts/src/arbitration/dispute-kits/DisputeKitGated.sol
  • contracts/test/foundry/KlerosCore.t.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
  • GitHub Check: Redirect rules - kleros-v2-neo
  • GitHub Check: Redirect rules - kleros-v2-university
  • GitHub Check: Redirect rules - kleros-v2-testnet
  • GitHub Check: Header rules - kleros-v2-neo
  • GitHub Check: Redirect rules - kleros-v2-testnet
  • GitHub Check: Header rules - kleros-v2-university
  • GitHub Check: Header rules - kleros-v2-testnet
  • GitHub Check: Header rules - kleros-v2-testnet
  • GitHub Check: Pages changed - kleros-v2-neo
  • GitHub Check: Pages changed - kleros-v2-university
  • GitHub Check: Pages changed - kleros-v2-testnet
  • GitHub Check: Pages changed - kleros-v2-testnet
  • GitHub Check: contracts-testing
  • GitHub Check: Analyze (javascript)
  • GitHub Check: SonarCloud
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/ETH-transfer

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

netlify bot commented Jul 16, 2025

Deploy Preview for kleros-v2-neo failed. Why did it fail? →

Name Link
🔨 Latest commit c5a894a
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-neo/deploys/688cadf37aa547000804eaec

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (2)
contracts/src/arbitration/KlerosCoreBase.sol (1)

104-104: Fix typo in comment.

The storage variable declaration is correct, but there's a typo in the comment: "tranfers" should be "transfers".

Apply this diff to fix the typo:

-    address public wNative; // The address for WETH tranfers.
+    address public wNative; // The address for WETH transfers.
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)

104-104: Fix typo in comment.

The storage variable declaration is correct, but there's a typo in the comment: "tranfers" should be "transfers".

Apply this diff to fix the typo:

-    address public wNative; // The address for WETH tranfers.
+    address public wNative; // The address for WETH transfers.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e99f3e6 and 1c7cc28.

📒 Files selected for processing (5)
  • contracts/src/arbitration/KlerosCoreBase.sol (6 hunks)
  • contracts/src/arbitration/devtools/KlerosCoreRuler.sol (5 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (6 hunks)
  • contracts/src/arbitration/university/KlerosCoreUniversity.sol (8 hunks)
  • contracts/src/libraries/SafeSend.sol (1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: jaybuidl
PR: kleros/kleros-v2#1620
File: contracts/test/arbitration/draw.ts:84-84
Timestamp: 2024-11-05T11:32:11.238Z
Learning: In TypeScript code using ethers.js version 6, `contract.target` should be used instead of `contract.address` to access a contract's address.
contracts/src/arbitration/devtools/KlerosCoreRuler.sol (1)
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (5)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/utils/getDispute.ts:38-40
Timestamp: 2024-10-21T10:32:16.970Z
Learning: The variables 'arbitrableChainID' and 'externalDisputeID' are required by the context to have uppercase 'ID', so they should remain unchanged even if the corresponding source properties use 'Id'.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/utils/getDispute.ts:43-43
Timestamp: 2024-10-14T15:29:32.954Z
Learning: In the Kleros SDK, when using the `populateTemplate` function in `kleros-sdk/src/utils/getDispute.ts`, missing variables in Mustache templates are acceptable. Mustache fills in blanks, and it's preferable to return the partially populated template without throwing errors, as it's still helpful for the consumer.
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
contracts/src/arbitration/KlerosCoreBase.sol (2)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1839
File: web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx:141-149
Timestamp: 2025-01-17T11:11:32.535Z
Learning: In the Kleros v2 codebase, using -1 as an initial value for choice tracking is preferred over undefined on the client-side to explicitly indicate that no option has been selected. This value is only used internally and never reaches the contract.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
contracts/src/arbitration/university/KlerosCoreUniversity.sol (3)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/utils/getDispute.ts:38-40
Timestamp: 2024-10-21T10:32:16.970Z
Learning: The variables 'arbitrableChainID' and 'externalDisputeID' are required by the context to have uppercase 'ID', so they should remain unchanged even if the corresponding source properties use 'Id'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
  • GitHub Check: Redirect rules - kleros-v2-testnet-devtools
  • GitHub Check: Redirect rules - kleros-v2-testnet
  • GitHub Check: Redirect rules - kleros-v2-testnet
  • GitHub Check: Redirect rules - kleros-v2-university
  • GitHub Check: Header rules - kleros-v2-testnet-devtools
  • GitHub Check: Header rules - kleros-v2-testnet
  • GitHub Check: Header rules - kleros-v2-testnet
  • GitHub Check: Header rules - kleros-v2-university
  • GitHub Check: Pages changed - kleros-v2-testnet
  • GitHub Check: Pages changed - kleros-v2-testnet
  • GitHub Check: Pages changed - kleros-v2-testnet-devtools
  • GitHub Check: Pages changed - kleros-v2-university
  • GitHub Check: Analyze (javascript)
  • GitHub Check: contracts-testing
🔇 Additional comments (26)
contracts/src/libraries/SafeSend.sol (1)

11-15: LGTM: WethLike interface is correctly defined.

The interface correctly defines the standard WETH methods needed for the fallback mechanism.

contracts/src/arbitration/devtools/KlerosCoreRuler.sol (4)

3-3: LGTM: Pragma version update is consistent.

The update from 0.8.24 to 0.8.25 aligns with the other contracts in this PR.


7-7: LGTM: SafeSend library integration is correct.

The import and using directive are properly added to enable SafeSend functionality.

Also applies to: 16-16


97-97: LGTM: wNative state variable addition is appropriate.

The variable is correctly typed and documented for storing the wrapped native token address.


533-533: LGTM: SafeSend usage is correct for fee distribution.

The replacement of direct .send() with SafeSend.safeSend() properly handles the fee reward transfer to the ruler account with WETH fallback.

contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (5)

3-3: LGTM: Pragma version update is consistent.

The update from 0.8.24 to 0.8.25 maintains consistency across the codebase.


8-8: LGTM: SafeSend library integration is correct.

The import and using directive properly enable SafeSend functionality for address payable types.

Also applies to: 17-17


62-62: LGTM: wNative state variable is appropriately added.

The variable is correctly positioned and documented for storing the wrapped native token address.


403-403: LGTM: SafeSend usage for refunding excess contributions.

The replacement correctly handles refunding excess contributions to the sender with WETH fallback mechanism.


448-448: LGTM: SafeSend usage for fee and reward withdrawals.

The replacement properly handles fee and reward transfers to beneficiaries with WETH fallback. The comment correctly notes the deliberate choice to prevent reverting fallbacks.

contracts/src/arbitration/KlerosCoreBase.sol (7)

3-3: Verify Solidity 0.8.25 compatibility and stability.

The pragma version update from 0.8.24 to 0.8.25 looks good, but please ensure that version 0.8.25 is stable and doesn't introduce any breaking changes that could affect the contract functionality.


11-11: LGTM: SafeSend library import added.

The import statement correctly adds the SafeSend library and WethLike interface, which aligns with the PR objectives of introducing safer ETH transfers.


19-19: LGTM: SafeSend using statement added.

The using statement correctly enables SafeSend methods for payable addresses, allowing for cleaner syntax when calling SafeSend.safeSend().


205-206: LGTM: Initialize function signature updated.

The function signature correctly adds the _wNative parameter for setting the wrapped native token address during initialization.


808-808: LGTM: SafeSend implementation for penalty execution.

The SafeSend.safeSend() call correctly replaces the direct ETH transfer with a safer alternative that falls back to WETH transfers if native sends fail.


865-865: LGTM: SafeSend implementation for reward execution.

The SafeSend.safeSend() call correctly replaces the direct ETH transfer with a safer alternative that falls back to WETH transfers if native sends fail.


891-891: LGTM: SafeSend implementation for leftover rewards.

The SafeSend.safeSend() call correctly replaces the direct ETH transfer with a safer alternative that falls back to WETH transfers if native sends fail.

contracts/src/arbitration/university/KlerosCoreUniversity.sol (9)

3-3: Verify Solidity 0.8.25 compatibility and stability.

The pragma version update from 0.8.24 to 0.8.25 looks good, but please ensure that version 0.8.25 is stable and doesn't introduce any breaking changes that could affect the contract functionality.


9-9: LGTM: SafeSend library import added.

The import statement correctly adds the SafeSend library and WethLike interface with the appropriate relative path for this contract's location.


18-18: LGTM: SafeSend using statement added.

The using statement correctly enables SafeSend methods for payable addresses, allowing for cleaner syntax when calling SafeSend.safeSend().


203-204: LGTM: Documentation added for wNative parameter.

The documentation correctly describes the purpose of the _wNative parameter for SafeSend fallback transfers.


213-214: LGTM: Initialize function signature updated.

The function signature correctly adds the _wNative parameter for setting the wrapped native token address during initialization.


221-221: LGTM: Correct assignment implementation.

The assignment correctly assigns the parameter value to the state variable: wNative = _wNative;. This is the proper implementation that was missing in KlerosCoreBase.sol.


797-797: LGTM: SafeSend implementation for penalty execution.

The SafeSend.safeSend() call correctly replaces the direct ETH transfer with a safer alternative that falls back to WETH transfers if native sends fail.


854-854: LGTM: SafeSend implementation for reward execution.

The SafeSend.safeSend() call correctly replaces the direct ETH transfer with a safer alternative that falls back to WETH transfers if native sends fail.


880-880: LGTM: SafeSend implementation for leftover rewards.

The SafeSend.safeSend() call correctly replaces the direct ETH transfer with a safer alternative that falls back to WETH transfers if native sends fail.

@jaybuidl jaybuidl changed the title Fix/eth transfer Contracts: use SafeSend Jul 18, 2025
@jaybuidl jaybuidl changed the title Contracts: use SafeSend Contracts: use SafeSend for ETH transfers Jul 18, 2025
coderabbitai[bot]
coderabbitai bot previously approved these changes Jul 18, 2025
coderabbitai[bot]
coderabbitai bot previously approved these changes Jul 22, 2025
@jaybuidl jaybuidl marked this pull request as draft July 24, 2025 11:18
@jaybuidl jaybuidl linked an issue Jul 24, 2025 that may be closed by this pull request
coderabbitai[bot]
coderabbitai bot previously approved these changes Jul 24, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf1f23b and d68ef7c.

📒 Files selected for processing (11)
  • contracts/src/arbitration/KlerosCore.sol (2 hunks)
  • contracts/src/arbitration/KlerosCoreBase.sol (10 hunks)
  • contracts/src/arbitration/KlerosCoreNeo.sol (3 hunks)
  • contracts/src/arbitration/KlerosGovernor.sol (7 hunks)
  • contracts/src/arbitration/devtools/KlerosCoreRuler.sol (5 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (3 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (12 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (3 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (2 hunks)
  • contracts/src/arbitration/university/KlerosCoreUniversity.sol (16 hunks)
  • contracts/src/libraries/SafeSend.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • contracts/src/libraries/SafeSend.sol
  • contracts/src/arbitration/devtools/KlerosCoreRuler.sol
  • contracts/src/arbitration/KlerosGovernor.sol
  • contracts/src/arbitration/KlerosCoreBase.sol
  • contracts/src/arbitration/university/KlerosCoreUniversity.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: Redirect rules - kleros-v2-testnet
  • GitHub Check: Redirect rules - kleros-v2-testnet
  • GitHub Check: Redirect rules - kleros-v2-testnet-devtools
  • GitHub Check: Header rules - kleros-v2-testnet
  • GitHub Check: Header rules - kleros-v2-testnet
  • GitHub Check: Header rules - kleros-v2-testnet-devtools
  • GitHub Check: Analyze (javascript)
  • GitHub Check: Pages changed - kleros-v2-testnet
  • GitHub Check: Pages changed - kleros-v2-testnet
  • GitHub Check: Pages changed - kleros-v2-testnet-devtools
  • GitHub Check: contracts-testing
  • GitHub Check: SonarCloud
🔇 Additional comments (24)
contracts/src/arbitration/KlerosCore.sol (2)

3-3: Version fixing looks good but consider implications.

Fixing the pragma version from ^0.8.24 to 0.8.24 removes compiler version flexibility but ensures consistency across the codebase. This aligns with the broader pattern in this PR.


18-18: Constructor base call is consistent with SafeSend integration.

The explicit call to KlerosCoreBase(address(0)) suggests the base constructor now requires a wNative parameter for SafeSend functionality. Passing address(0) indicates this contract doesn't use wrapped native token fallback, which is appropriate for the main KlerosCore implementation.

contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (2)

3-3: Pragma version fixing maintains consistency.

The change from ^0.8.24 to 0.8.24 aligns with the codebase-wide standardization effort in this PR.


34-34: Base constructor call supports SafeSend integration.

The explicit call to DisputeKitClassicBase(address(0)) is consistent with the SafeSend library integration where the base class now requires a wNative parameter.

contracts/src/arbitration/KlerosCoreNeo.sol (3)

3-3: Pragma version standardization approved.

Fixing the version to 0.8.24 maintains consistency with the broader codebase changes.


26-26: Constructor update aligns with SafeSend integration.

The explicit base constructor call with address(0) follows the pattern established across core contracts for SafeSend integration.


108-108: Function signature update handles transfer flag correctly.

The addition of the false parameter to _setStake call indicates that tokens haven't been transferred yet, which is correct for this user-facing staking function. This aligns with the refined staking logic mentioned in the AI summary.

contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (4)

3-3: Pragma version standardization approved.

The change to fixed version 0.8.24 maintains consistency across the codebase.


21-21: Constructor update follows SafeSend integration pattern.

The explicit call to DisputeKitClassicBase(address(0)) is consistent with SafeSend library integration.


14-14: Align DisputeKitClassic version with broader strategy

DisputeKitClassic’s version was set to "0.10.0", matching DisputeKitSybilResistant and DisputeKitGated, whereas DisputeKitGatedShutter and DisputeKitShutter remain at "0.11.x". Please confirm this downgrade (from the previous "0.11.0") is intentional and aligns with our overall versioning plan.

• contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol: version = "0.10.0"
• contracts/src/arbitration/dispute-kits/DisputeKitGated.sol: version = "0.10.0"
• contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol: version = "0.11.0"
• contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol: version = "0.11.1"
• contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol: version = "0.10.0"


32-32: Double-check DisputeKitClassic initializer version

I noticed the NOP initializer in DisputeKitClassic was changed to:

• contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
function initialize6() external reinitializer(6) { /* NOP */ }

Meanwhile, other dispute‐kit contracts use higher versions:
– DisputeKitGatedShutter: initialize7() → reinitializer(7)
– DisputeKitShutter: initialize8() → reinitializer(8)

Please confirm that version 6 correctly follows the last deployed migration for Classic and that no proxies have already run a version 7 initializer. You’ll want to ensure this downgrade won’t skip or collide with existing upgrade paths.

contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (7)

3-3: Pragma version standardization approved.

The change to fixed version 0.8.24 maintains consistency with the broader codebase changes.


30-30: Version downgrade aligns with dispute kit standardization.

The version downgrade from "0.11.0" to "0.10.0" is consistent with similar changes in DisputeKitClassic, suggesting a coordinated versioning effort.


36-38: New storage variables improve token gating management.

The addition of explicit storage variables (tokenGate, tokenId, isERC1155) replaces the previous dynamic parsing approach, making the contract more predictable and easier to manage.


45-45: Constructor follows SafeSend integration pattern.

The explicit call to DisputeKitClassicBase(address(0)) is consistent with the SafeSend library integration across dispute kit contracts.


55-66: Enhanced initialization function improves configurability.

The extended initialize function now accepts token gating parameters directly, making the setup more explicit and eliminating the need for extraData parsing during initialization.


78-92: Governance functions provide post-deployment flexibility.

The addition of changeTokenGateERC20OrERC721 and changeTokenGateERC1155 functions allows for updating token gating configuration after deployment, providing necessary flexibility while maintaining security through governor-only access.


99-111: Simplified token gating logic improves readability.

The refactored _postDrawCheck function is much cleaner, using stored state variables instead of parsing extraData. The logic correctly handles both ERC-1155 and ERC-20/ERC-721 token types with appropriate balance checks.

contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (6)

3-3: LGTM! Proper integration of SafeSend library.

The pragma version update and SafeSend library import with the using directive are correctly implemented.

Also applies to: 8-8, 17-17


62-62: Good use of immutable for gas optimization.

The W_NATIVE variable is correctly declared as immutable since it's set once during deployment.


408-408: Correct implementation of SafeSend for refunds.

The SafeSend integration properly handles excess ETH refunds with WETH fallback.


453-453: Good use of SafeSend for withdrawal protection.

The implementation correctly uses SafeSend to prevent withdrawal failures, with appropriate documentation about user responsibility.


644-650: Excellent addition of juror solvency check.

The enhanced _postDrawCheck now properly verifies that jurors have sufficient staked tokens to cover their locked amount plus the new stake requirement. This prevents selection of potentially insolvent jurors.


313-314: Commit hash calculation change verified

  • DisputeKitClassicBase now uses
    keccak256(abi.encodePacked(_choice, _salt)) without the justification string.
  • No occurrences of the old abi.encodePacked(..., _justification) format remain in Classic (confirmed via global search).
  • DisputeKitShutter and DisputeKitGatedShutter still use their own hashVote (which includes justification) and are unaffected.
  • Tests in KlerosCore.t.sol have already been updated to use the new Classic commit format.

No further changes are needed in the Solidity contracts. Please ensure any front-end or documentation that generates Classic DK commits is updated to drop the justification from the hash.

@thasmi21 thasmi21 force-pushed the fix/ETH-transfer branch 2 times, most recently from 5bedbd7 to cf1f23b Compare July 25, 2025 21:12
@jaybuidl jaybuidl requested a review from unknownunknown1 July 30, 2025 10:53
Copy link

sonarqubecloud bot commented Aug 1, 2025

@@ -1,20 +1,22 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.8.24;
pragma solidity 0.8.25;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't update the solidity version, it's out-of-scope for this PR

@@ -85,14 +87,14 @@ contract KlerosCoreRuler is IArbitratorV2, UUPSProxiable, Initializable {

uint256 private constant NON_PAYABLE_AMOUNT = (2 ** 256 - 2) / 2; // An amount higher than the supply of ETH.

address public governor; // The governor of the contract.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed? It breaks compilation.

@@ -800,7 +805,7 @@ abstract contract KlerosCoreBase is IArbitratorV2, Initializable, UUPSProxiable
// No one was coherent, send the rewards to the governor.
if (round.feeToken == NATIVE_CURRENCY) {
// The dispute fees were paid in ETH
payable(governor).send(round.totalFeesForJurors);
SafeSend.safeSend(payable(governor), round.totalFeesForJurors, wNative);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short syntax to call the library:
payable(governor).safeSend(round.totalFeesForJurors, wNative);

@@ -15,7 +15,7 @@ contract KlerosCore is KlerosCoreBase {
// ************************************* //

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
constructor() KlerosCoreBase(address(0)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not like this, it's an upgradable contract

@@ -201,7 +201,7 @@ abstract contract KlerosCoreBase is IArbitratorV2, Initializable, UUPSProxiable
*/
constructor(address _wNative) {
W_NATIVE = _wNative;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's an upgradable contract so it happens in the initializer

uint96 _courtID,
uint256 _newStake,
bool _alreadyTransferred
) external {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is reverting the changes made by others in the dev branch which breaks everything

@jaybuidl jaybuidl marked this pull request as ready for review August 1, 2025 12:17
@jaybuidl
Copy link
Member

jaybuidl commented Aug 1, 2025

@coderabbitai resolve

Copy link
Contributor

coderabbitai bot commented Aug 1, 2025

✅ Actions performed

Comments resolved and changes approved.

@jaybuidl jaybuidl merged commit 6786c29 into dev Aug 1, 2025
15 of 26 checks passed
@jaybuidl jaybuidl deleted the fix/ETH-transfer branch August 1, 2025 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contracts: use SafeSend when sending ETH to the users
3 participants