review fixes #220
review fixes #220
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (9)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR refactors gas account management and protocol infrastructure: removes public gas query methods, expands external access to wrap/unwrap functions, converts the deposit flow from opaque payload encoding to explicit parameters, adds stricter fee consumption access control, simplifies internal return types, and adjusts payload encoding and escrow state tracking. Changes
Sequence DiagramsequenceDiagram
participant Caller as External Caller
participant GasStation as GasStation
participant GAM as GasAccountManager
participant GasAccountToken as GasAccountToken
Note over Caller,GasAccountToken: New Deposit Flow (Explicit Parameters)
Caller->>GasStation: depositFromChain(token, receiver, gasAmount, nativeAmount)
GasStation->>GAM: depositFromChain(token, receiver, gasAmount, nativeAmount)
GAM->>GasAccountToken: burn/transfer operations
GAM-->>GasStation: returns payloadId (bytes)
GasStation-->>Caller: emit GasDeposited(payloadId)
Note over Caller,GAM: Access Control Changes
Caller->>GAM: wrapToGas(receiver) [now external, no onlyWatcher]
Caller->>GAM: unwrapFromGas(amount, receiver) [now external, no onlyWatcher]
GAM-->>Caller: ✓ Allowed (previously gated by watcher)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/evmx/fees/GasEscrow.sol (1)
48-64: Event emits new deposit amount, but storage tracks total—this breaks semantic consistency.Line 63 emits
amount_(the new deposit), while line 59 storesamount(the cumulative total including any existing escrow for the payload). Off-chain listeners monitoringGasEscrowedwill see only the incremental deposit, not the actual escrowed total for the payload. This forces external systems to manually sum all events to reconstruct the total, and creates a mismatch withgetPayloadEscrow()which returns the cumulative amount.Either emit
amount(the total) to match storage semantics, or clearly document that the event reflects only the incremental deposit.- emit GasEscrowed(payloadId_, consumeFrom_, amount_); + emit GasEscrowed(payloadId_, consumeFrom_, amount);Or, if the incremental deposit behavior is intentional, add a fourth parameter to the event to include both values:
emit GasEscrowed(payloadId_, consumeFrom_, amount_, amount); // deposit and totalcontracts/protocol/switchboard/SwitchboardBase.sol (1)
63-69: Digest encoding change will break signature compatibility—ensure coordinated migrationThe change from
abi.encode(address(socket__), payloadId_)toabi.encodePacked(address(socket__), payloadId_)produces different hash values, invalidating all signatures created with the old format. Any off-chain signers, external verifiers, or watcher helpers still using the old encoding scheme will fail signature recovery silently.This should be coordinated across all components that produce or consume these signatures—including any external systems not in this repository. Note: active test coverage for transmitter signature verification is absent outside the deprecated folder, which increases the risk of integration issues with coordinating systems.
🧹 Nitpick comments (5)
contracts/evmx/fees/GasEscrow.sol (3)
78-93: TODO indicates incomplete design around partial settlement workflow.The comment asks about final states and the settle+settle+release workflow. The current implementation supports partial settlements (line 89 decrements entry.amount), but the interaction between multiple settles and potential releases needs clarification. Specifically:
- Can
releaseEscrowbe called after partial settlement?- Should partial settlements transition to an intermediate state?
- What happens if settlement amount exceeds remaining escrow?
This should be resolved before merging.
Do you want me to analyze the possible state transitions and propose a state machine design for the escrow lifecycle?
79-93: Add explicit validation for settlement amount.Line 89 subtracts
amountfromentry.amountwithout checking thatamount <= entry.amount. While Solidity 0.8+ prevents underflow, an explicit check with a descriptive error improves clarity and helps catch bugs in the calling contract.function settleGasPayment( bytes32 payloadId, address transmitter, uint256 amount ) external onlyGasAccountManager { EscrowEntry storage entry = payloadEscrow[payloadId]; if (entry.state != EscrowState.Active) revert NotActive(); if (entry.amount == 0) revert NoEscrow(); + if (amount > entry.amount) revert InsufficientEscrow(); accountEscrow[entry.account] -= amount; entry.amount -= amount; if (entry.amount == 0) entry.state = EscrowState.Settled; emit EscrowSettled(payloadId, entry.account, transmitter, amount); }Define the error at contract level:
error InsufficientEscrow();
112-118: Rescue function should be owner-only, not gasAccountManager-only.Line 116 restricts
rescueFundstogasAccountManagerinstead ofowner. This is inconsistent with typical security patterns where rescue functions require the highest privilege level. IfgasAccountManageris compromised, funds can be drained. The contract inheritsOwnablebut doesn't useonlyOwnerfor this critical function.- ) external onlyGasAccountManager { + ) external onlyOwner { RescueFundsLib._rescueFunds(token_, rescueTo_, amount_); }contracts/protocol/Socket.sol (1)
109-136: _verify now ignores switchboardId parameter; consider cleaning it up
_verifyderivesswitchboardAddressvia_verifyPlugSwitchboard(executeParams_.target)and no longer uses theuint32parameter passed in fromexecute. This should either drop that parameter (and adjust call sites) or document that the parameter is intentionally kept only to preserve an override signature, because right now it adds call complexity without contributing to behavior.test/PausableTest.t.sol (1)
47-57: New Watcher.initialize argument is a magic value; name its intentThe added
bytes32(0)keeps the test in sync with the updatedWatcher.initializesignature, but its role is opaque from the test (it looks like a placeholder ID/anchor). This should either be given a named constant or documented in a brief comment so that future signature changes or non‑zero defaults don’t silently misconfigure the watcher in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
contracts/evmx/fees/GasAccountManager.sol(5 hunks)contracts/evmx/fees/GasAccountToken.sol(1 hunks)contracts/evmx/fees/GasEscrow.sol(1 hunks)contracts/evmx/helpers/AsyncPromise.sol(0 hunks)contracts/evmx/interfaces/IGasAccountManager.sol(2 hunks)contracts/evmx/plugs/GasStation.sol(2 hunks)contracts/protocol/Socket.sol(3 hunks)contracts/protocol/switchboard/SwitchboardBase.sol(1 hunks)contracts/utils/common/Structs.sol(0 hunks)script/helpers/TransferRemainingGas.s.sol(1 hunks)test/PausableTest.t.sol(1 hunks)test/SetupTest.t.sol(4 hunks)
💤 Files with no reviewable changes (2)
- contracts/utils/common/Structs.sol
- contracts/evmx/helpers/AsyncPromise.sol
🔇 Additional comments (5)
contracts/protocol/Socket.sol (3)
217-231: sendPayload now consistently reuses switchboard validation
_sendPayloadnow routes plug validation through_verifyPlugSwitchboard, so switchboard existence and registration are enforced in one place before callingprocessPayload. This should keep plug/switchboard checks consistent across the contract.
238-245: increaseFeesForPayload correctly gates on a registered plugUsing
_verifyPlugSwitchboard(msg.sender)here ensures that only a known plug with a registered switchboard can increase fees, and that the call always targets the correct switchboard address. This should prevent misrouted fee top‑ups to invalid or unregistered boards.
247-255: _verifyPlugSwitchboard encapsulates plug → switchboard checks cleanlyThe helper now returns only the switchboard address while still enforcing
PlugNotFoundandInvalidSwitchboardconditions based onplugSwitchboardIdsandisValidSwitchboard. This should centralize plug/switchboard validation and reduce the chance that callers accidentally use an ID without checking registration.contracts/evmx/interfaces/IGasAccountManager.sol (1)
82-87: settleGasPayment formatting change only; no behavioral impact
settleGasPaymentkeeps the same parameters and visibility; only line wrapping changed. This should be a no‑op semantically.script/helpers/TransferRemainingGas.s.sol (1)
28-37: Verification confirms totalBalanceOf semantics are correct.The implementation shows
totalBalanceOfreturns the fullsuper.balanceOf(account), whilebalanceOfreturnssuper.balanceOf(account) - escrowed. This means the script's computationpayloadEscrow = totalBalanceOf - balanceOfcorrectly yields the escrowed amount, with no underflow risk. The change is sound.
| function depositFromChain( | ||
| address token_, | ||
| address depositTo_, | ||
| uint256 gasAmount_, | ||
| uint256 nativeAmount_ | ||
| ) external onlyWatcher { | ||
| uint32 chainSlug = watcher__().triggerFromChainSlug(); | ||
| tokenOnChainBalances[chainSlug][toBytes32Format(token_)] += gasAmount_ + nativeAmount_; | ||
|
|
||
| // Mint tokens to the user | ||
| gasAccountToken__().mint(depositTo, gasAmount); | ||
| if (nativeAmount > 0) { | ||
| gasAccountToken__().mint(depositTo_, gasAmount_); | ||
| if (nativeAmount_ > 0) { | ||
| // if native transfer fails, add to gas | ||
| bool success = gasVault__().withdraw(depositTo, nativeAmount); | ||
| bool success = gasVault__().withdraw(depositTo_, nativeAmount_); | ||
|
|
||
| if (!success) { | ||
| // Convert failed native amount to gas | ||
| gasAccountToken__().mint(depositTo, nativeAmount); | ||
| gasAmount += nativeAmount; | ||
| nativeAmount = 0; | ||
| gasAccountToken__().mint(depositTo_, nativeAmount_); | ||
| gasAmount_ += nativeAmount_; | ||
| nativeAmount_ = 0; | ||
| } | ||
| } | ||
|
|
||
| emit Deposited(chainSlug, token, depositTo, gasAmount, nativeAmount); | ||
| emit Deposited(chainSlug, token_, depositTo_, gasAmount_, nativeAmount_); | ||
| } |
There was a problem hiding this comment.
Make depositFromChain return the payload id (or stop exposing it).
Line 107: GasStation now invokes this through an interface that returns bytes memory, but this implementation returns nothing. The ABI decoder therefore reverts on every call because it expects dynamic return data. This should return the payload identifier you want to emit (and be marked override accordingly), or the caller must stop decoding a value. As written, any on-chain deposit flow breaks.
🤖 Prompt for AI Agents
In contracts/evmx/fees/GasAccountManager.sol around lines 106-130, the
depositFromChain function currently has no return but the external
interface/consumer expects a bytes (dynamic) return value causing ABI decode
reverts; change the function signature to return bytes memory (and add override
if required by the interface), compute/derive the payload identifier you intend
to emit (or reuse the emitted identifier) and return it at the end of the
function, and update the contract/interface declarations so ABI types match (or
alternatively remove the return from the interface/caller if you prefer not to
expose it).
| /// @notice Deposit tokens from a chain into gas account | ||
| /// @dev Called by watcher after detecting GasStation deposit | ||
| function depositFromChain(bytes memory payload_) external; | ||
| function depositFromChain( | ||
| address token_, | ||
| address receiver_, | ||
| uint256 gasAmount_, | ||
| uint256 nativeAmount_ | ||
| ) external; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
depositFromChain API is now explicit; ensure all implementors and callers are updated
Changing depositFromChain to (address token_, address receiver_, uint256 gasAmount_, uint256 nativeAmount_) is a good move away from opaque bytes payloads and should make cross‑chain deposits less error‑prone. This should be reflected in every contract implementing IGasAccountManager and every caller (e.g. GasStation, watchers) so there are no lingering uses of the old encoded payload form.
🏁 Script executed:
#!/bin/bash
# Ensure all implementations and uses of depositFromChain match the new signature
rg -n "depositFromChain" -C3Length of output: 2676
Parameter name mismatch in GasAccountManager.depositFromChain
The interface declares the second parameter as receiver_, but the implementation in GasAccountManager.sol:108 names it depositTo_. This violates the interface contract. The implementation should rename depositTo_ to receiver_ to match the interface signature and maintain consistency with all callers (GasStation and tests).
🤖 Prompt for AI Agents
In contracts/evmx/interfaces/IGasAccountManager.sol around lines 52-60 and
implementation in contracts/.../GasAccountManager.sol at line 108, the interface
declares the second parameter as receiver_ but the implementation uses
depositTo_; rename the implementation parameter depositTo_ to receiver_ to match
the interface signature, update all references inside the function body and any
NatSpec/comments to use receiver_, and run tests to ensure callers
(GasStation/tests) remain compatible.
| bytes memory payloadId = IGasAccountManager(address(socket__)).depositFromChain( | ||
| token_, | ||
| receiver_, | ||
| gasAmount_, | ||
| nativeAmount_ | ||
| ); | ||
| // todo: IGasAccountManager(socket__).depositFromChain(token_, receiver_, gasAmount_, nativeAmount_); | ||
|
|
||
| // Create trigger via Socket to get unique payloadId | ||
| bytes32 payloadId = socket__.sendPayload(payload); | ||
| token_.safeTransferFrom(msg.sender, address(this), gasAmount_ + nativeAmount_); | ||
| emit GasDeposited(token_, receiver_, gasAmount_, nativeAmount_, payloadId); | ||
| emit GasDeposited(token_, receiver_, gasAmount_, nativeAmount_, bytes32(payloadId)); | ||
| } |
There was a problem hiding this comment.
Fix the deposit call target and return handling.
Line 80: This should not cast socket__ (the Socket contract) to IGasAccountManager. Socket does not implement depositFromChain, so the very first deposit will revert with an unknown selector. Even if you swap in the real GasAccountManager address, the call still reverts because you decode a bytes memory payload while the current implementation returns no data. Restore the previous socket payload flow or invoke the actual GasAccountManager contract, and align the ABI so the callee either returns the payload id or the caller stops decoding it. Until then, deposits are dead.
🤖 Prompt for AI Agents
In contracts/evmx/plugs/GasStation.sol around lines 80–90, the code incorrectly
casts socket__ to IGasAccountManager and attempts to decode a bytes return from
depositFromChain; fix by calling the real GasAccountManager contract (use the
stored gasAccountManager address/type) instead of socket__, and align the
ABI/handling: if GasAccountManager.depositFromChain does not return data, stop
assigning/decoding bytes memory payloadId and instead rely on the socket flow
that creates/returns the payloadId (or emit/derive the payloadId from the
correct call); alternatively, update the callee ABI to return the payload id
(bytes/bytes32) and handle that return consistently so the deposit no longer
reverts.
Summary by CodeRabbit
Bug Fixes
Refactor