fix(contracts): bridge approval safety + IRYLA interface decoupling#24
Merged
Conversation
Wrap sendERC20 in try/catch — on revert, forceApprove(0) clears the dangling approval and BridgeFailed() is raised. Prevents tokens from being stuck in the adapter with an open approval if the bridge call fails.
…rface Extract IRYLA interface (mint, burn, balanceOf) into src/interfaces/. MARKSettlementModule now depends on the interface rather than the concrete RYLA implementation, reducing coupling between domains.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughA try/catch wrapper is added around the Superchain bridge ERC20 send in MARKBridgeAdapter; on failure it resets token approval and reverts with a new BridgeFailed error. An IRYLA interface is introduced and MARKSettlementModule now depends on IRYLA instead of concrete RYLA. ChangesBridge Error Handling and Settlement Abstraction
Sequence DiagramsequenceDiagram
participant Adapter as MARKBridgeAdapter
participant Bridge as Superchain Bridge
participant Token as ERC20 Token
Adapter->>Token: approve(Bridge, amount)
Adapter->>Bridge: sendERC20(token, to, amount)
alt Bridge success
Bridge->>Token: transferFrom(Adapter, recipient, amount)
Bridge-->>Adapter: returns messageHash
Adapter->>Adapter: store messageHash
else Bridge revert
Bridge-->>Adapter: throws
Adapter->>Token: approve(Bridge, 0) rgba(255,0,0,0.5)
Adapter-->>Adapter: revert BridgeFailed()
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contracts/src/bridge/MARKBridgeAdapter.sol (1)
95-102: ⚡ Quick winAdd a unit test for the new
catchbranch inbridgeTo.The failure path is newly introduced, but current test context only demonstrates success flow. Please add a test that mocks
sendERC20revert and assertsBridgeFailed()is raised.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/bridge/MARKBridgeAdapter.sol` around lines 95 - 102, Add a unit test for the failure path of bridgeTo that stubs/mocks SUPERCHAIN_TOKEN_BRIDGE.sendERC20 to revert, then calls the contract's bridgeTo entry (the function containing the try/catch around SUPERCHAIN_TOKEN_BRIDGE.sendERC20) and asserts the call reverts with BridgeFailed(); additionally verify that TOKEN.forceApprove was invoked with 0 and that no messageHash is returned/updated after the revert. Use the sendERC20, SUPERCHAIN_TOKEN_BRIDGE, TOKEN.forceApprove, BridgeFailed, and the bridgeTo function names to locate and hook the behavior in your test harness/mocks.contracts/src/interfaces/IRYLA.sol (1)
6-10: ⚡ Quick winMake
IRYLAexplicitly inheritIERC20to match downstream usage.
MARKSettlementModulecastsTOKEN(typed asIRYLA) toIERC20when callingsafeTransferFrom(). DeclaringIRYLA is IERC20makes this dependency explicit, eliminates the unsafe type cast, and provides type-safety guarantees for future implementations.♻️ Proposed refactor
// SPDX-License-Identifier: MIT pragma solidity ^0.8.25; +import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; + /// `@title` IRYLA /// `@notice` Minimal interface for settlement modules interacting with the RYLA token. -interface IRYLA { +interface IRYLA is IERC20 { function mint(address to, uint256 amount) external; function burn(uint256 amount) external; - function balanceOf(address account) external view returns (uint256); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/interfaces/IRYLA.sol` around lines 6 - 10, Update the IRYLA interface to explicitly inherit IERC20 (change `interface IRYLA {` to `interface IRYLA is IERC20 {`) and add the required import for IERC20 so the compiler recognizes it; keep the existing mint, burn, and balanceOf signatures and ensure they don't conflict with IERC20. This makes downstream usage in MARKSettlementModule (where `TOKEN` typed as `IRYLA` is cast to IERC20 for `safeTransferFrom`) type-safe and removes the need for unsafe casting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@contracts/src/bridge/MARKBridgeAdapter.sol`:
- Around line 95-102: Add a unit test for the failure path of bridgeTo that
stubs/mocks SUPERCHAIN_TOKEN_BRIDGE.sendERC20 to revert, then calls the
contract's bridgeTo entry (the function containing the try/catch around
SUPERCHAIN_TOKEN_BRIDGE.sendERC20) and asserts the call reverts with
BridgeFailed(); additionally verify that TOKEN.forceApprove was invoked with 0
and that no messageHash is returned/updated after the revert. Use the sendERC20,
SUPERCHAIN_TOKEN_BRIDGE, TOKEN.forceApprove, BridgeFailed, and the bridgeTo
function names to locate and hook the behavior in your test harness/mocks.
In `@contracts/src/interfaces/IRYLA.sol`:
- Around line 6-10: Update the IRYLA interface to explicitly inherit IERC20
(change `interface IRYLA {` to `interface IRYLA is IERC20 {`) and add the
required import for IERC20 so the compiler recognizes it; keep the existing
mint, burn, and balanceOf signatures and ensure they don't conflict with IERC20.
This makes downstream usage in MARKSettlementModule (where `TOKEN` typed as
`IRYLA` is cast to IERC20 for `safeTransferFrom`) type-safe and removes the need
for unsafe casting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 846ac536-85cd-446b-9352-6cabebf858a7
📒 Files selected for processing (4)
contracts/src/bridge/MARKBridgeAdapter.solcontracts/src/errors/BridgeErrors.solcontracts/src/interfaces/IRYLA.solcontracts/src/settlement/MARKSettlementModule.sol
…test - IRYLA now inherits IERC20, eliminating the unsafe cast in MARKSettlementModule (using SafeERC20 for IRYLA directly) - Remove unused IERC20 import from MARKSettlementModule - Add unit test for BridgeFailed catch branch: mocks sendERC20 revert, asserts BridgeFailed() raised and approval reset to 0
This was referenced May 8, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two targeted contract improvements identified during code review.
Changes
fix(bridge): clear approval and revert cleanly on sendERC20 failure
sendERC20intry/catchinMARKBridgeAdapter.bridgeToforceApprove(0)clears dangling approval,BridgeFailed()raisedBridgeFailederror toBridgeErrorsrefactor(settlement): decouple from RYLA concrete type via IRYLA interface
IRYLAinterface (mint,burn,balanceOf) intosrc/interfaces/MARKSettlementModulenow importsIRYLAinstead of concreteRYLAScope
contractsVerification
forge build— cleanforge test— 58/58 passRisk
Low — no logic changes to settlement flow, bridge fix only adds safety on the failure path.
Summary by CodeRabbit
Bug Fixes
Refactor
New
Tests