Merge again solana-triggers to dev-solana-v2 #195
Conversation
Itchy Palm Devnet
Folder rearrange
Folder rearrange
refactor: adderss <-> bytes32
…socket-protocol into fees-deposit-hook
feat: hyper gas limti
…socket-protocol into fees-deposit-hook
|
Caution Review failedThe pull request is closed. WalkthroughRefactors switchboard addressing to uint64 IDs, replaces digest/attest flow with trigger/processTrigger, introduces Message/CCTP switchboards and SUSDC ERC20 fee flows, consolidates payload addressing into a packed payloadPointer, and updates many interfaces, structs, tests, deployments, and scripts to the new model. (≈46 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Plug as Plug (IPlug)
participant Socket as Socket
participant SB as ISwitchboard
participant AG as AppGateway
Note over Plug,Socket: Trigger-based call (new flow)
Plug->>Socket: fallback/triggerAppGateway(data, value)
Socket->>Socket: create triggerId, lookup plug config (switchboardId)
Socket->>SB: processTrigger(plug, triggerId, payload, overrides) payable
SB-->>Socket: ack / emit TriggerProcessed
alt execution
SB->>Socket: getTransmitter(...)/allowPayload
Socket->>AG: tryCall(execute payload)
AG-->>Socket: success / returnData
Socket->>Fees: settle via IERC20.transferFrom or refunds
else fail
Socket-->>Plug: revert/refund
end
sequenceDiagram
autonumber
participant W as Watcher
participant Req as RequestHandler
participant FM as FeesManager (IERC20)
participant AG as AppGateway
Note over Req,FM: Batch processing uses payloadPointer and IERC20.transferFrom
W->>Req: submitRequest(...)
Req->>Req: build payloadPointer, payloadId (IdUtils.pack)
Req->>FM: IERC20(address(FM)).transferFrom(appGateway, Req, totalFees)
alt settle success
Req->>AG: tryCall(handlePayload)
AG-->>Req: ok
else call fails
Req-->>W: emit RequestCompletedWithErrors
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (67)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 44
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
test/apps/ParallelCounter.t.sol (1)
44-110: Removeviewfrom helper assertions.assertEqfrom forge-std emits logs and is notview. Calling it from_testForwarderAddresses,_assertForwarderAddresses, and_assertForwarderAddressdeclaredviewtriggers the compiler error “Function declared as view but this call may modify state.” This should drop theviewmutability from these helpers.- function _testForwarderAddresses() internal view { + function _testForwarderAddresses() internal { ... - ) internal view { + ) internal { ... - ) internal view { + ) internal {script/counter/WithdrawFeesArbitrumFeesPlug.s.sol (1)
38-42: Fix unit mismatch: ETH gas (wei) vs token credits
estimatedGasCostis in wei (ETH),availableFeesis a token balance (e.g., USDC credits). Subtracting ETH gas from token units is incorrect. This should either:
- Convert ETH gas to token units via a price feed (off-chain or oracle), or
- Skip the subtraction and withdraw the intended token amount explicitly.
Quick safe fallback (remove ETH subtraction):
- uint256 amountToWithdraw = availableFees > estimatedGasCost - ? availableFees - estimatedGasCost - : 0; + uint256 amountToWithdraw = availableFees;test/SetupTest.t.sol (1)
535-548: Fix createSignature mutabilityThis should not be declared
purebecause it callsvm.sign, which the compiler treats as a non-pure external call. Solidity will refuse to compile with “Function declared as pure, but this expression (potentially) modifies the state.” Switch it back toview(or drop the mutability specifier) so the suite builds.- ) public pure returns (bytes memory sig) { + ) public view returns (bytes memory sig) {hardhat-scripts/deploy/2.roles.ts (1)
78-90: Assign watcher role to MessageSwitchboardThe new watcher role entry for
MessageSwitchboardfalls through tosigner.addressbecause the special-case gate only whitelistsFastSwitchboardandCCTPSwitchboard. This should includeContracts.MessageSwitchboard; otherwise, the watcher cannot exercise theWATCHER_ROLEon the new switchboard and cross-chain message attestation will fail in production. Apply the following fix.- [Contracts.FastSwitchboard, Contracts.CCTPSwitchboard].includes( + [ + Contracts.FastSwitchboard, + Contracts.CCTPSwitchboard, + Contracts.MessageSwitchboard, + ].includes( contractName as Contracts ) && roleName === ROLES.WATCHER_ROLEhardhat-scripts/deploy/6.connect.ts (3)
166-166: Fix missing ethers import by using existing helper.
ethersisn’t imported in this file; this will throw at runtime. Use the already-imported toBytes32 helper instead ofethers.utils.hexZeroPad.Apply:
- const solanaAppGatewayId = ethers.utils.hexZeroPad(appGatewayAddress, 32); + const solanaAppGatewayId = toBytes32FormatHexString(appGatewayAddress);
162-166: Avoid null-deref on SWITCHBOARD_SOLANA and sanitize hex.You’re calling
.slice(2)on a possibly undefined env var. This should check before slicing and normalize the 0x prefix safely.Apply:
- const solanaSwitchboard = process.env.SWITCHBOARD_SOLANA!.slice(2); // remove 0x prefix for Buffer from conversion - if (!solanaSwitchboard) throw new Error("SWITCHBOARD_SOLANA is not set"); + const solanaSwitchboardRaw = process.env.SWITCHBOARD_SOLANA; + if (!solanaSwitchboardRaw) throw new Error("SWITCHBOARD_SOLANA is not set"); + const solanaSwitchboard = solanaSwitchboardRaw.startsWith("0x") + ? solanaSwitchboardRaw.slice(2) + : solanaSwitchboardRaw;
175-178: Inconsistent struct field: use switchboardId (uint64), not switchboard (bytes32).Everywhere else
plugConfigusesswitchboardId. Here it usesswitchboard, which will breakAppGatewayConfigtyping and watcher encoding. This should pass a uint64 switchboardId for Solana too.If you have a numeric env for Solana’s switchboard id:
- plugConfig: { - appGatewayId: solanaAppGatewayId, - switchboard: "0x" + solanaSwitchboardBytes32.toString("hex"), - }, + plugConfig: { + appGatewayId: solanaAppGatewayId, + switchboardId: process.env.SWITCHBOARD_SOLANA_ID!, + },If not, add one and decode as a uint64, e.g.,
const solanaSwitchboardId = BigInt(process.env.SWITCHBOARD_SOLANA_ID!).contracts/evmx/fees/FeesManager.sol (1)
75-82: Fix blockCredits availability check.
This should reject blocks that exceed the unblocked balance. Right now the guard only comparesbalanceOfagainstcredits_, so a payer with 100 SUSDC can have 80 already blocked and still successfully block another 80. The first request that unblocks will burn 80 and leave the account with 20, making the secondunblockAndAssignrevert when_burntries to pull 80 tokens. That wedges settlement for every subsequent batch. Gate againstbalanceOf - userBlockedCreditsbefore incrementing the blocked tally.- if (balanceOf(consumeFrom_) < credits_) revert InsufficientCreditsAvailable(); - - userBlockedCredits[consumeFrom_] += credits_; + uint256 blocked = userBlockedCredits[consumeFrom_]; + uint256 available = balanceOf(consumeFrom_) - blocked; + if (available < credits_) revert InsufficientCreditsAvailable(); + + userBlockedCredits[consumeFrom_] = blocked + credits_;hardhat-scripts/deploy/3.configureChains.ts (2)
191-207: Fix compile-time error:toBytes32Formatis undefined; use the existing hex helper and remove incorrect Buffer conversion.
toBytes32Formatis not imported and likely doesn’t exist here. Also,Buffer.from(toBytes32Format(...))is incorrect for a hex string. This should usetoBytes32FormatHexStringconsistently.Apply this diff:
- console.log( - "solanaSwitchboard as bytes32 reversed: ", - Buffer.from(toBytes32Format(solanaSwitchboard)).toString("hex") - ); + console.log( + "solanaSwitchboard (bytes32): ", + toBytes32FormatHexString(solanaSwitchboard) + ); @@ - [ - ChainSlug.SOLANA_DEVNET, - FAST_SWITCHBOARD_TYPE, - toBytes32Format(solanaSwitchboard), - ], + [ + ChainSlug.SOLANA_DEVNET, + FAST_SWITCHBOARD_TYPE, + toBytes32FormatHexString(solanaSwitchboard), + ],
390-418: Use BigNumber for switchboardId to avoid type bugs and bad numeric coercions.Assigning a BigNumber to a string variable and using
Number()risks subtle bugs and TS errors.Apply this diff:
-): Promise<string> => { - let switchboardId: string = "0"; +): Promise<string> => { + let switchboardIdBn: BigNumber = BigNumber.from(0); @@ - switchboardId = await socket.switchboardIds(sbAddress, { + switchboardIdBn = await socket.switchboardIds(sbAddress, { from: signer.address, }); @@ - if (Number(switchboardId) == 0) { + if (switchboardIdBn.eq(0)) { const registerTx = await switchboard.registerSwitchboard({ ...(await overrides(chain)), }); console.log(`Registering Switchboard ${sbAddress}: ${registerTx.hash}`); await registerTx.wait(); - switchboardId = await switchboard.switchboardId(); - console.log(`Switchboard ID: ${switchboardId}`); + switchboardIdBn = await switchboard.switchboardId(); + console.log(`Switchboard ID: ${switchboardIdBn.toString()}`); } - return switchboardId; + return switchboardIdBn.toString();test/mock/MockSocket.sol (1)
76-77: Undefined ExecutionStatus enum breaks compilation
ExecutionStatusis still referenced in storage, but the enum definition was removed from this contract and no import provides it. Solidity treats this as an unknown type, so MockSocket no longer compiles. This should reintroduce the enum (or change the mapping to use an existing type) before the mapping declaration.struct PlugConfigEvm { // address of the sibling plug on the remote chain bytes32 appGatewayId; // switchboard id for the plug connection uint64 switchboardId; } + enum ExecutionStatus { + None, + Success, + Failed + } + // plug => (appGateway, switchboardId) mapping(address => PlugConfigEvm) internal _plugConfigs;
🧹 Nitpick comments (59)
src/signer.ts (1)
2-30: Use ethers’ randomBytes instead of uuid.Pulling in
uuidjust to mint a hex string is unnecessary and still only yields 128 bits of entropy. This should reuseethers.utils.randomBytesto get a full 256-bit value while dropping the extra dependency.-import { v4 as uuidv4 } from "uuid"; ... -export const getNonce = () => { - const randomId = uuidv4(); - const nonceHex = randomId.replace(/-/g, ""); - return "0x" + nonceHex; -}; +export const getNonce = () => { + return ethers.utils.hexlify(ethers.utils.randomBytes(32)); +};script/supertoken/TransferSuperToken.s.sol (1)
29-33: Use the contract selectorThis should use
SuperTokenAppGateway.transfer.selectorinstead of re-hashing the signature to avoid drift if the signature definition ever changes.- bytes memory encodedPayload = abi.encodeWithSelector( - bytes4(keccak256("transfer(bytes)")), - encodedOrder - ); + bytes memory encodedPayload = abi.encodeWithSelector( + SuperTokenAppGateway.transfer.selector, + encodedOrder + );contracts/evmx/interfaces/IAppGateway.sol (1)
44-46: Avoid duplicating switchboard type exposure
sbType()duplicates the second return value ofgetOverrideParams()(Line 16). This should be either:
- Documented as identical and invariant, or
- Removed from one surface to prevent drift.
If both must exist, explicitly state in NatSpec that both return the same value and are guaranteed to stay in sync.
script/counter/WithdrawFeesArbitrumFeesPlug.s.sol (1)
48-48: Avoid hardcoding chain slugHardcoding
421614ties the script to Arbitrum Sepolia. This should use a constant/source of truth (e.g., a Chains/Slugs map) or an env var to prevent errors across environments.hardhat-scripts/constants/feeConstants.ts (1)
23-25: Verify new fee pool addressesUpdated DEV/STAGE fee pool addresses should be validated (correct chain, checksummed, owned). This should be cross-checked in deploy/connect scripts and with actual on-chain contracts.
Optionally freeze these maps to avoid accidental mutation.
You can add:
-const feePools: { [key: string]: string } = { +const feePools: { [key: string]: string } = { ... -}; +} as const;contracts/evmx/interfaces/ISUSDC.sol (2)
7-7: Use calldata for bytes to avoid unnecessary copiesExternal function should take
bytes calldatainstead ofbytes memoryto prevent ABI decoding into memory.- function burn(address receiver_, uint256 amount, bytes memory data_) external; + function burn(address receiver_, uint256 amount, bytes calldata data_) external;
5-7: Align parameter naming for symmetry and clarityMint uses
towhile burn usesreceiver_. This should be consistent (e.g.,account_orfrom_for burn) for readability across implementations. ABI is unaffected by param names.- function burn(address receiver_, uint256 amount, bytes calldata data_) external; + function burn(address account_, uint256 amount, bytes calldata data_) external;contracts/protocol/interfaces/IMessageHandler.sol (1)
14-20: Name the return variable to match NatSpecNatSpec says “@return success” but the signature returns anonymous
bool. Name it to keep docs and ABI metadata aligned.- ) external returns (bool); + ) external returns (bool success);script/supertoken/DeployEVMxSuperTokenApp.s.sol (3)
36-37: Stop broadcast at the end of the scriptLeaving broadcast open is sloppy and can mask later mistakes. Explicitly stop it.
console.log("SuperTokenAppGateway:", address(gateway)); gateway.setSbType(CCTP); + vm.stopBroadcast();
8-8: Fix run-command path casingThe filename is
DeployEVMxSuperTokenApp.s.sol, but the example uses lowercase. On case‑sensitive filesystems this will fail.-// source .env && forge script script/supertoken/deployEVMxSuperTokenApp.s.sol --broadcast --skip-simulation --legacy --gas-price 0 +// source .env && forge script script/supertoken/DeployEVMxSuperTokenApp.s.sol --broadcast --skip-simulation --legacy --gas-price 0
26-31: Improve large constant readabilityDefine
initialSupplywith grouped digits and1e18for clarity.- initialSupplyHolder_: vm.addr(deployerPrivateKey), - initialSupply_: 1000000000000000000000000000 + initialSupplyHolder_: vm.addr(deployerPrivateKey), + initialSupply_: 1_000_000_000 * 1e18src/types.ts (1)
58-59: S3Config.evmxChainSlug as required may break consumersIf existing S3 blobs don’t include this, downstream code may read
undefined. Make it optional or add explicit runtime validation.- evmxChainSlug: number; + evmxChainSlug?: number;Add a validation check at load (example):
if (cfg.evmxChainSlug == null) throw new Error("Missing S3Config.evmxChainSlug");contracts/evmx/interfaces/IConfigurations.sol (3)
36-40: Docstring incorrect for switchboards()The comment says “Returns the socket” but this returns a switchboardId (uint64). Fix the NatSpec to prevent confusion.
- /// @notice Returns the socket for a given chain slug - /// @param chainSlug_ The chain slug - /// @return The socket + /// @notice Returns the switchboardId for a given chain slug and switchboard type + /// @param chainSlug_ The chain slug + /// @param sbType_ The switchboard type identifier + /// @return switchboardId The switchboard id
26-30: Name return values for clarityName the return values to document semantics (e.g.,
appGatewayId,switchboardId).- ) external view returns (bytes32, uint64); + ) external view returns (bytes32 appGatewayId, uint64 switchboardId);
39-43: Unify parameter naming with verifyConnections
verifyConnectionsusesswitchboardType_. Keep parameter names consistent across the interface.-function switchboards(uint32 chainSlug_, bytes32 sbType_) external view returns (uint64); +function switchboards(uint32 chainSlug_, bytes32 switchboardType_) external view returns (uint64);-function setSwitchboard(uint32 chainSlug_, bytes32 sbType_, uint64 switchboardId_) external; +function setSwitchboard(uint32 chainSlug_, bytes32 switchboardType_, uint64 switchboardId_) external;contracts/protocol/SocketUtils.sol (6)
127-140: Make simulate nonpayable and tighten loop for gasThis should not accept ETH and should avoid redundant length loads and checked increments.
-function simulate( - SimulateParams[] calldata params -) external payable onlyOffChain returns (SimulationResult[] memory) { - SimulationResult[] memory results = new SimulationResult[](params.length); - - for (uint256 i = 0; i < params.length; i++) { - (bool success, bool exceededMaxCopy, bytes memory returnData) = params[i] - .target - .tryCall(params[i].value, params[i].gasLimit, maxCopyBytes, params[i].payload); - results[i] = SimulationResult(success, returnData, exceededMaxCopy); - } - - return results; -} +function simulate( + SimulateParams[] calldata params +) external onlyOffChain returns (SimulationResult[] memory) { + uint256 len = params.length; + SimulationResult[] memory results = new SimulationResult[](len); + for (uint256 i; i < len;) { + (bool success, bool exceededMaxCopy, bytes memory returnData) = params[i] + .target + .tryCall(params[i].value, params[i].gasLimit, maxCopyBytes, params[i].payload); + results[i] = SimulationResult(success, returnData, exceededMaxCopy); + unchecked { ++i; } + } + return results; +}
75-98: Use abi.encode over abi.encodePacked for digestThis should use abi.encode to avoid any packed-encoding ambiguity and align with canonical ABI encoding. Changing this alters the digest; ensure all producers/consumers are updated together.
- return - keccak256( - abi.encodePacked( - toBytes32Format(address(this)), - toBytes32Format(transmitter_), - payloadId_, - executeParams_.deadline, - executeParams_.callType, - executeParams_.gasLimit, - executeParams_.value, - executeParams_.payload, - toBytes32Format(executeParams_.target), - appGatewayId_, - executeParams_.prevBatchDigestHash, - executeParams_.extraData - ) - ); + return keccak256( + abi.encode( + toBytes32Format(address(this)), + toBytes32Format(transmitter_), + payloadId_, + executeParams_.deadline, + executeParams_.callType, + executeParams_.gasLimit, + executeParams_.value, + executeParams_.payload, + toBytes32Format(executeParams_.target), + appGatewayId_, + executeParams_.prevBatchDigestHash, + executeParams_.extraData + ) + );Run end-to-end tests for all digest verifiers/attesters before merging.
28-30: Avoid hard-coded 0xDEAD; make off-chain caller configurableThis should be immutable and set at deploy (or allow owner fallback) to reduce integration friction with clients that can’t set from=0xDEAD.
-// address of the off-chain caller -address public constant OFF_CHAIN_CALLER = address(0xDEAD); +// address of the off-chain caller (RPC eth_call "from") +address public immutable OFF_CHAIN_CALLER;-/** - * @notice constructor for creating a new Socket contract instance - * @param chainSlug_ The unique identifier of the chain this socket is deployed on - * @param owner_ The address of the owner who has the initial admin role - * @param version_ The version string which is hashed and stored in socket - */ -constructor(uint32 chainSlug_, address owner_, string memory version_) { +/** + * @param offChainCaller_ Address that must be used as msg.sender in eth_call for off-chain-only functions + */ +constructor(uint32 chainSlug_, address owner_, string memory version_, address offChainCaller_) { chainSlug = chainSlug_; version = keccak256(bytes(version_)); triggerPrefix = (uint256(chainSlug_) << 224) | (uint256(uint160(address(this))) << 64); + OFF_CHAIN_CALLER = offChainCaller_;If you keep the constant, at minimum document that clients must set from=0x...dead in eth_call and add a test asserting the modifier behavior.
Also applies to: 58-64
105-107: Avoid post-increment in expression for clarityThis should separate the increment to avoid subtlety around post-increment.
-function _encodeTriggerId() internal returns (bytes32) { - return bytes32(triggerPrefix | triggerCounter++); -} +function _encodeTriggerId() internal returns (bytes32) { + bytes32 id = bytes32(triggerPrefix | triggerCounter); + // Overflow reverts in ^0.8; if wrap-around is acceptable, wrap in unchecked. + triggerCounter++; + return id; +}
43-45: Unused error: SimulationFailedThis should either be emitted/used or removed to keep the surface minimal.
4-4: Remove unused ECDSA importECDSA isn’t referenced here after removing signer recovery. Drop to save bytecode and reduce lint warnings.
-import {ECDSA} from "solady/utils/ECDSA.sol";contracts/evmx/interfaces/IERC20.sol (1)
4-22: Align with OZ interfaces or split metadataThis should either:
- Adopt OpenZeppelin’s IERC20 (without decimals) and, where needed, use IERC20Metadata for decimals; or
- Rename this to IERC20WithMetadata to avoid confusion/collisions with OZ IERC20 in consumers.
Mixing decimals into IERC20 deviates from common tooling and can cause import conflicts.
contracts/protocol/SocketFeeManager.sol (1)
22-27: Drop the unused FeeTooLow error.
FeeTooLowis declared but never triggered. This should either be enforced insetSocketFeesor removed—otherwise it just increases surface area without value.- /// @notice Thrown when the fees are too low - error FeeTooLow();contracts/evmx/interfaces/IReceiver.sol (1)
7-13: Expose the payload parameter as calldata.
onTransferis external; declaringdata_asbytes memoryforces every implementation to copy the payload into memory. Usingbytes calldatakeeps the call zero-copy.- bytes memory data_ + bytes calldata data_hardhat-scripts/deploy/1.deploy.ts (1)
360-372: Guard against missing MESSAGE_TRANSMITTER entries
Add a check forMESSAGE_TRANSMITTER[chain], throwing a clear error or skipping CCTP deployment if it’sundefined. Mapping currently covers all configured chains.hardhat-scripts/deploy/6.connect.ts (2)
65-67: Treat isSocketInitialized as a boolean (or verify the return type).Calling
.toNumber()will throw if the function returns a bool (common with ethers). This should branch on the boolean (or compare enum directly if it returns a uint8).Example:
- const isInitialized = await plug.isSocketInitialized(); - if (isInitialized.toNumber() === 1) functionName = "connectSocket"; + const isInitialized = await plug.isSocketInitialized(); + if (isInitialized) functionName = "connectSocket";
52-58: Ensure switchboardId type matches expected string in isConfigSetOnSocket.
isConfigSetOnSocketcompares strings; ifaddr[Contracts.FastSwitchboardId]isn’t already a string, equality will fail. Coerce to string before passing.- const switchboardId = addr[Contracts.FastSwitchboardId]; + const switchboardId = String(addr[Contracts.FastSwitchboardId]);hardhat-scripts/utils/appConfig.ts (2)
12-15: Normalize switchboardId comparison to strings on both sides.Avoid hidden type mismatches by casting both to strings explicitly.
- plugConfigRegistered.switchboardId.toString() === switchboardId + plugConfigRegistered.switchboardId.toString() === String(switchboardId)
30-32: Minor: symmetric string coercion.Keep comparisons consistent; coerce both sides to string.
- plugConfigRegistered[1].toString() === switchboardId.toString() + plugConfigRegistered[1].toString() === String(switchboardId)script/helpers/WithdrawRemainingCredits.s.sol (1)
25-29: Guard zero-balance and confirm transferFrom allowance/role.
transferFrom(appGateway, ...)will revert without allowance/authorization. Add a zero-balance guard and ensure the deployer has permission (allowance or role) to spend credits.- uint256 availableFees = feesManager.balanceOf(appGateway); - console.log("Available fees:", availableFees); - feesManager.transferFrom(appGateway, vm.addr(deployerPrivateKey), availableFees); + uint256 availableFees = feesManager.balanceOf(appGateway); + console.log("Available fees:", availableFees); + if (availableFees == 0) { + console.log("No available fees to withdraw"); + } else { + // Requires allowance or appropriate role for msg.sender + feesManager.transferFrom(appGateway, vm.addr(deployerPrivateKey), availableFees); + }contracts/evmx/interfaces/IAuctionManager.sol (1)
27-30: Name both parameters for clarity and ABI docs.Unnamed params hinder readability and NatSpec generation. Name the second parameter or align docs accordingly.
- /// @param data The encoded request count - function endAuction(bytes memory data, bytes memory) external; + /// @param data The encoded request count + /// @param returnData The return data (unused) + function endAuction(bytes memory data, bytes memory returnData) external;contracts/evmx/base/AppGatewayBase.sol (3)
167-178: Solana gap: getOnChainAddress/setAddress never invokedCurrent flow depends on Forwarder/AsyncDeployer, which doesn’t run for Solana. This should add a Solana path (e.g., separate mapping keyed by chainSlug → bytes32 program/account id) and bypass Forwarder for validation and address resolution.
I can sketch a minimal Solana registry (mapping(uint32 => mapping(bytes32 => bytes32)) plus setters guarded by onlyOwner/onlyWatcher) and adjust _setValidPlug/getOnChainAddress to read from it. Want a patch?
225-230: Solana gap in _setValidPlugThis still relies on getOnChainAddress(), which is unimplemented for Solana. This should handle Solana’s address form and not depend on Forwarder state.
Same as above; I can wire a Solana-specific branch and storage to support setIsValidPlug.
374-378: Defensive decodeDecoding without validating length can revert if onCompleteData_ is malformed. Since onlyWatcher protects this, impact is low, but a length check to 32 bytes would harden this path.
- uint32 chainSlug = abi.decode(onCompleteData_, (uint32)); + if (onCompleteData_.length != 32) return; + uint32 chainSlug = abi.decode(onCompleteData_, (uint32));test/evmx/Watcher.t.sol (1)
176-179: Unify event qualification for clarity.Elsewhere you emit
WritePrecompile.WriteProofUploaded; here you emitWriteProofUploadedunqualified. This should be consistent to avoid symbol ambiguity.Apply this diff:
- emit WriteProofUploaded(payloadId, proof); + emit WritePrecompile.WriteProofUploaded(payloadId, proof);contracts/evmx/watcher/RequestHandler.sol (2)
252-255: Make payloadPointer packing explicit and future-proof.Define bit widths and mask to 40/40/40 to prevent accidental overflow if types change. Improves readability and decoding consistency.
Example refactor:
- uint160 payloadPointer = (uint160(requestCount_) << 120) | - (uint160(nextBatchCount) << 80) | - uint160(payloadCounter++); + // 40 | 40 | 40 bits: requestCount | batchCount | payloadIndex + unchecked { + uint160 pc = uint160(payloadCounter++); // payloadCounter is uint40 + uint160 payloadPointer = + (uint160(requestCount_) << 120) | + (uint160(nextBatchCount) << 80) | + (pc & ((uint160(1) << 40) - 1)); + }
238-242: Guard against unset switchboardId.If
switchboards(chainSlug, switchboardType)returns 0 for an unconfigured chain/type, downstreamcreatePayloadIdwill still proceed. This should revert early for clarity.Apply this diff:
uint64 switchboardId = watcher__().configurations__().switchboards( queuePayloadParam.transaction.chainSlug, queuePayloadParam.switchboardType ); + if (switchboardId == 0) revert InvalidSwitchboard();contracts/protocol/base/MessagePlugBase.sol (4)
35-37: Use the custom error instead of string revert.Cheaper and consistent with the rest of the codebase.
Apply this diff:
- function initSocket(bytes32, address, uint64) external override socketInitializer { - revert("Not Supported"); - } + function initSocket(bytes32, address, uint64) external override socketInitializer { + revert NotSupported(); + }
42-45: Add access control to registerSibling.Registering siblings should not be callable by arbitrary EOAs. Restrict to owner or socket.
Apply this diff (assuming PlugBase exposes an onlyOwner or onlySocket modifier):
- function registerSibling(uint32 chainSlug_, address siblingPlug_) public { + function registerSibling(uint32 chainSlug_, address siblingPlug_) public onlyOwner { IMessageSwitchboard(switchboard).registerSibling(chainSlug_, toBytes32Format(siblingPlug_)); }
25-32: Validate switchboard resolution during construction.This should fail fast if
switchboardAddresses(switchboardId_)is unset, instead of silently storing address(0).Apply this diff:
switchboardId = switchboardId_; switchboard = socket__.switchboardAddresses(switchboardId_); + if (switchboard == address(0)) revert NotSupported(); socket__.__connect(APP_GATEWAY_ID, switchboardId_);
31-32: Unused triggerPrefix; either use it in getNextTriggerId or remove it.Currently dead state. If kept, refactor getNextTriggerId to reuse it.
Example:
- function getNextTriggerId(uint32 chainSlug_) public view returns (bytes32) { - return - bytes32( - (uint256(chainSlug_) << 224) | - (uint256(uint160(address(socket__))) << 64) | - (uint256(socket__.triggerCounter()) << 16) - ); - } + function getNextTriggerId(uint32 chainSlug_) public view returns (bytes32) { + uint256 prefix = (uint256(chainSlug_) << 224) | (uint256(uint160(address(socket__))) << 64); + return bytes32(prefix | (uint256(socket__.triggerCounter()) << 16)); + }hardhat-scripts/deploy/3.configureChains.ts (2)
318-323: Remove unused parametersocketfromaddRemoteEndpointsToCCTPSwitchboard.
socketis not used; this should be removed to avoid confusion.
- Update function signature and all call sites.
221-236: Avoid redundant hex conversions (nit).
feesPlugandsocketare already bytes32-encoded viatoBytes32FormatHexString. Re-wrapping them again and calling.toString()is unnecessary. This should simplify params and reduce error surface.test/protocol/Socket.t.sol (2)
794-801: Brittle test: hard-coded storage slot for counter will break with layout changes.Hard-coding
counterSlot = 57makes the test fragile. This should avoid peeking storage and instead exercise the public wrapper repeatedly, or add a test-only setter/getter in the wrapper gated toaddress(this)for determinism.
- Option A: Drop vm.store and assert format/monotonicity by calling
encodeTriggerId()multiple times and decoding fields.- Option B: Add a test-only function in
SocketTestWrapperto set/read the counter (only in test).
870-873: Use return value to assert revert (nit).You ignore
successafter expecting revert. Optional: assert!successto make intent explicit, or rely solely onexpectRevertand drop the variable.
- Current behavior is fine; this is a clarity improvement only.
contracts/protocol/Socket.sol (2)
168-172: External refund call can reenter; guard recommended.
forceSafeTransferETHperforms an external call in the failure path. This should add a reentrancy guard (nonReentrant modifier) or move to CEI with a simple mutex if there’s any future state added before the refund. Today, payload status is set before the call, which limits impact, but the surface remains.
- Add OpenZeppelin ReentrancyGuard and mark
executeand_executeas nonReentrant, or add a minimal boolean lock.
100-129: Digest is stored beforeallowPayloadreturns (ok due to revert semantics, but document it).A revert in
allowPayloadreverts the mapping write; functionally safe. This should have a brief comment to prevent future refactors from moving the write after an external call.contracts/protocol/interfaces/ISocket.sol (1)
129-130: This getter exposes internal mapping that may changeThe
switchboardAddressesfunction directly exposes the internal switchboard ID to address mapping. This creates a tight coupling between the interface and implementation details.Consider whether this should be part of a separate registry interface or if there's a more abstract way to query switchboard information that won't lock you into this specific data structure.
test/protocol/switchboards/FastSwitchboardTest.t.sol (1)
89-91: Hardcoded test private key is predictableUsing a simple sequential hex value for the private key makes the test less realistic and could mask issues with actual key generation/handling.
-uint256 nonWatcherPrivateKey = 0x1234567890123456789012345678901234567890123456789012345678901234; +uint256 nonWatcherPrivateKey = uint256(keccak256(abi.encodePacked("test.nonwatcher.key", block.timestamp)));contracts/protocol/SocketConfig.sol (3)
123-132: Reject zero appGatewayId in connect()A zero appGatewayId makes the plug indistinguishable from "disconnected" in disconnect(), and is likely invalid config. Add a guard.
function connect(bytes32 appGatewayId_, uint64 switchboardId_) external override { if (isValidSwitchboard[switchboardId_] != SwitchboardStatus.REGISTERED) revert InvalidSwitchboard(); + require(appGatewayId_ != bytes32(0), "appGatewayId is zero"); PlugConfigEvm storage _plugConfig = _plugConfigs[msg.sender]; _plugConfig.appGatewayId = appGatewayId_; _plugConfig.switchboardId = switchboardId_; emit PlugConnected(msg.sender, appGatewayId_, switchboardId_); }
162-165: Bound maxCopyBytes to a safe rangeThis should enforce non-zero and an upper bound (you already import MAX_COPY_BYTES). Prevents misconfiguration that can cause excessive memory copies.
function setMaxCopyBytes(uint16 maxCopyBytes_) external onlyRole(GOVERNANCE_ROLE) { - maxCopyBytes = maxCopyBytes_; + require(maxCopyBytes_ > 0 && maxCopyBytes_ <= MAX_COPY_BYTES, "maxCopyBytes out of bounds"); + maxCopyBytes = maxCopyBytes_; emit MaxCopyBytesUpdated(maxCopyBytes_); }
90-95: Optionally validate switchboard existence on enable/disableEnabling/disabling non-existent IDs (e.g., 0) silently mutates state. This should check that switchboardAddresses[switchboardId_] != address(0) to avoid operator mistakes.
Also applies to: 102-105
contracts/protocol/switchboard/CCTPSwitchboard.sol (3)
26-32: Add explicit errors for endpoint/digest validationDefine specific errors to use in the checks below.
error RemoteExecutionNotFound(); error PrevBatchDigestHashMismatch(); error NotAttested(); error NotExecuted(); error InvalidSender(); error OnlyMessageTransmitter(); +error UnknownEndpoint(); +error InvalidRemoteDigest();
80-84: Validate messages/attestations length matchOut-of-bounds will revert with a generic panic. Provide a clear revert early.
function verifyAttestations(bytes[] calldata messages, bytes[] calldata attestations) public { + require(messages.length == attestations.length, "length mismatch"); for (uint256 i = 0; i < messages.length; i++) { messageTransmitter.receiveMessage(messages[i], attestations[i]); } }
86-122: Bind current payload to received remote digestThis should assert that a remote-executed digest exists for payloadId_ and matches the reconstructed digest. Otherwise isRemoteExecuted could be set without anchoring to the received message for this payload.
function proveRemoteExecutions( bytes32[] calldata previousPayloadIds_, bytes32 payloadId_, bytes calldata transmitterSignature_, ExecuteParams calldata executeParams_ ) public { @@ - // Construct current digest + // Construct current digest (bytes32 appGatewayId, ) = socket__.getPlugConfig(executeParams_.target); bytes32 constructedDigest = _createDigest( transmitter, payloadId_, appGatewayId, executeParams_ ); - // Verify the constructed digest matches the stored one - if (!isAttested[constructedDigest]) revert NotAttested(); + // Verify attested and matches remote-executed digest for current payload + if (!isAttested[constructedDigest]) revert NotAttested(); + bytes32 remoteDigest = remoteExecutedDigests[payloadId_]; + if (remoteDigest == bytes32(0)) revert RemoteExecutionNotFound(); + if (remoteDigest != constructedDigest) revert InvalidRemoteDigest(); isRemoteExecuted[payloadId_] = true; }contracts/protocol/switchboard/MessageSwitchboard.sol (3)
34-44: Doc/key mismatch: attestation keyed by digest, not payloadIdMapping comment says payloadId => isAttested, but attest/allowPayload use digest. Update the comment to avoid misuse.
- // payloadId => isAttested - mapping(bytes32 => bool) public isAttested; + // digest => isAttested + mapping(bytes32 => bool) public isAttested;
80-91: Validate sibling config inputsSetting zero socket/switchboard silently creates a broken config later causing SiblingNotFound at runtime. Validate inputs on set.
function setSiblingConfig( uint32 chainSlug_, uint256 fee_, bytes32 socket_, bytes32 switchboard_ ) external onlyOwner { + if (socket_ == bytes32(0) || switchboard_ == bytes32(0)) revert SiblingNotFound();
239-256: Digest domain separation: include contract version/switchboardIdThe digest is chained with toBytes32Format(this) and chainSlug. Consider also mixing switchboardId to tighten cross-switchboard separation.
- keccak256( - abi.encodePacked( + keccak256( + abi.encodePacked( digest_.socket, digest_.transmitter, digest_.payloadId, digest_.deadline, digest_.callType, digest_.gasLimit, digest_.value, digest_.payload, digest_.target, digest_.appGatewayId, digest_.prevBatchDigestHash, digest_.extraData ) );If you’re already anchoring elsewhere, ignore.
contracts/evmx/fees/Credit.sol (1)
246-251: isCreditSpendable allowance check is binaryYou block if allowance == 0 but ignore amount < allowance. This allows spending more than approved amount if allowance > 0 and spender != consumeFrom. transferFrom later uses ERC20 allowance enforcement, but you pre-approve watchers (Line 269) which can bypass end-user intended caps. If per-spend caps are needed, compare allowance to amount_ here when spender is not a watcher.
- if (!_isWatcher(spender_) && consumeFrom_ != spender_) { - if (allowance(consumeFrom_, spender_) == 0) return false; - } + if (!_isWatcher(spender_) && consumeFrom_ != spender_) { + if (allowance(consumeFrom_, spender_) < amount_) return false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (117)
Errors.md(3 hunks)EventTopics.md(1 hunks)FunctionSignatures.md(11 hunks)contracts/evmx/AuctionManager.sol(4 hunks)contracts/evmx/base/AppGatewayBase.sol(5 hunks)contracts/evmx/fees/Credit.sol(13 hunks)contracts/evmx/fees/FeesManager.sol(4 hunks)contracts/evmx/helpers/AsyncDeployer.sol(3 hunks)contracts/evmx/interfaces/IAppGateway.sol(1 hunks)contracts/evmx/interfaces/IAuctionManager.sol(1 hunks)contracts/evmx/interfaces/IConfigurations.sol(2 hunks)contracts/evmx/interfaces/IContractFactoryPlug.sol(1 hunks)contracts/evmx/interfaces/IERC20.sol(1 hunks)contracts/evmx/interfaces/IFeesManager.sol(2 hunks)contracts/evmx/interfaces/IFeesPlug.sol(2 hunks)contracts/evmx/interfaces/IPromise.sol(1 hunks)contracts/evmx/interfaces/IReceiver.sol(1 hunks)contracts/evmx/interfaces/ISUSDC.sol(1 hunks)contracts/evmx/plugs/ContractFactoryPlug.sol(3 hunks)contracts/evmx/plugs/FeesPlug.sol(4 hunks)contracts/evmx/plugs/SUSDC.sol(1 hunks)contracts/evmx/watcher/Configurations.sol(6 hunks)contracts/evmx/watcher/PromiseResolver.sol(2 hunks)contracts/evmx/watcher/RequestHandler.sol(8 hunks)contracts/evmx/watcher/Trigger.sol(2 hunks)contracts/evmx/watcher/Watcher.sol(3 hunks)contracts/evmx/watcher/WatcherBase.sol(2 hunks)contracts/evmx/watcher/precompiles/SchedulePrecompile.sol(3 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(3 hunks)contracts/protocol/Socket.sol(8 hunks)contracts/protocol/SocketBatcher.sol(2 hunks)contracts/protocol/SocketConfig.sol(1 hunks)contracts/protocol/SocketFeeManager.sol(2 hunks)contracts/protocol/SocketUtils.sol(5 hunks)contracts/protocol/base/MessagePlugBase.sol(1 hunks)contracts/protocol/base/PlugBase.sol(3 hunks)contracts/protocol/interfaces/ICCTPSwitchboard.sol(1 hunks)contracts/protocol/interfaces/IMessageHandler.sol(1 hunks)contracts/protocol/interfaces/IMessageTransmitter.sol(1 hunks)contracts/protocol/interfaces/IPlug.sol(1 hunks)contracts/protocol/interfaces/ISocket.sol(3 hunks)contracts/protocol/interfaces/ISocketBatcher.sol(2 hunks)contracts/protocol/interfaces/ISocketFeeManager.sol(2 hunks)contracts/protocol/interfaces/ISwitchboard.sol(1 hunks)contracts/protocol/switchboard/CCTPSwitchboard.sol(1 hunks)contracts/protocol/switchboard/FastSwitchboard.sol(2 hunks)contracts/protocol/switchboard/MessageSwitchboard.sol(1 hunks)contracts/protocol/switchboard/SwitchboardBase.sol(2 hunks)contracts/utils/RescueFundsLib.sol(2 hunks)contracts/utils/common/Constants.sol(2 hunks)contracts/utils/common/Errors.sol(1 hunks)contracts/utils/common/IdUtils.sol(1 hunks)contracts/utils/common/Structs.sol(6 hunks)deployments/dev_addresses.json(1 hunks)deployments/dev_verification.json(1 hunks)deployments/stage_addresses.json(1 hunks)deployments/stage_verification.json(1 hunks)foundry.toml(1 hunks)hardhat-scripts/admin/disable-sb.ts(2 hunks)hardhat-scripts/admin/disconnect.ts(4 hunks)hardhat-scripts/config/config.ts(3 hunks)hardhat-scripts/constants/constants.ts(1 hunks)hardhat-scripts/constants/feeConstants.ts(1 hunks)hardhat-scripts/constants/types.ts(1 hunks)hardhat-scripts/deploy/1.deploy.ts(6 hunks)hardhat-scripts/deploy/2.roles.ts(2 hunks)hardhat-scripts/deploy/3.configureChains.ts(8 hunks)hardhat-scripts/deploy/6.connect.ts(5 hunks)hardhat-scripts/deploy/9.setupTransmitter.ts(4 hunks)hardhat-scripts/misc-scripts/getAttestations.ts(1 hunks)hardhat-scripts/s3Config/buildConfig.ts(1 hunks)hardhat-scripts/utils/appConfig.ts(2 hunks)hardhat-scripts/utils/gatewayId.ts(1 hunks)hardhat-scripts/utils/overrides.ts(1 hunks)hardhat-scripts/utils/sign.ts(4 hunks)hardhat.config.ts(1 hunks)package.json(3 hunks)script/counter/DeployEVMxCounterApp.s.sol(1 hunks)script/counter/WithdrawFeesArbitrumFeesPlug.s.sol(1 hunks)script/helpers/CheckDepositedCredits.s.sol(1 hunks)script/helpers/DepositCredit.s.sol(1 hunks)script/helpers/DepositCreditAndNative.s.sol(1 hunks)script/helpers/DepositCreditMainnet.s.sol(1 hunks)script/helpers/TransferRemainingCredits.s.sol(1 hunks)script/helpers/WithdrawRemainingCredits.s.sol(1 hunks)script/super-token-solana/DeployEVMSolanaApps.s.sol(1 hunks)script/supertoken/DeployEVMxSuperTokenApp.s.sol(1 hunks)script/supertoken/TransferSuperToken.s.sol(1 hunks)src/enums.ts(1 hunks)src/signer.ts(2 hunks)src/types.ts(2 hunks)test/DigestTest.t.sol(2 hunks)test/SetupTest.t.sol(27 hunks)test/Utils.t.sol(1 hunks)test/Watcher.t.sol(0 hunks)test/apps/Counter.t.sol(1 hunks)test/apps/ParallelCounter.t.sol(2 hunks)test/apps/app-gateways/counter/CounterAppGateway.sol(4 hunks)test/apps/app-gateways/counter/MessageCounter.sol(1 hunks)test/apps/app-gateways/super-token/SuperTokenAppGateway.sol(2 hunks)test/evmx/AuctionManager.t.sol(2 hunks)test/evmx/FeesTest.t.sol(11 hunks)test/evmx/ProxyMigration.t.sol(1 hunks)test/evmx/ProxyStorage.t.sol(1 hunks)test/evmx/Watcher.t.sol(1 hunks)test/mock/CCTPMessageTransmitter.sol(1 hunks)test/mock/MockERC721.sol(1 hunks)test/mock/MockFastSwitchboard.sol(2 hunks)test/mock/MockFeesManager.sol(1 hunks)test/mock/MockPlug.sol(1 hunks)test/mock/MockSocket.sol(3 hunks)test/protocol/Socket.t.sol(1 hunks)test/protocol/SocketFeeManager.t.sol(3 hunks)test/protocol/TriggerTest.t.sol(3 hunks)test/protocol/switchboards/FastSwitchboardTest.t.sol(1 hunks)test/protocol/switchboards/MessageSwitchboardTest.t copy.sol(1 hunks)trace.sh(1 hunks)
💤 Files with no reviewable changes (1)
- test/Watcher.t.sol
🧰 Additional context used
🧬 Code graph analysis (10)
hardhat-scripts/utils/appConfig.ts (2)
hardhat-scripts/config/config.ts (1)
watcher(96-96)hardhat-scripts/utils/address.ts (1)
toBytes32Format(54-61)
hardhat-scripts/s3Config/buildConfig.ts (1)
hardhat-scripts/config/config.ts (1)
EVMX_CHAIN_ID(100-100)
hardhat-scripts/deploy/1.deploy.ts (3)
hardhat-scripts/constants/constants.ts (1)
WRITE_MAX_FEES(16-16)hardhat-scripts/utils/deployUtils.ts (1)
getOrDeploy(38-77)src/cctp.ts (1)
MESSAGE_TRANSMITTER(4-18)
hardhat-scripts/admin/disconnect.ts (4)
hardhat-scripts/utils/appConfig.ts (1)
isConfigSetOnSocket(4-16)hardhat-scripts/constants/constants.ts (1)
BYTES32_ZERO(12-12)hardhat-scripts/utils/overrides.ts (1)
overrides(43-54)hardhat-scripts/utils/address.ts (1)
toBytes32FormatHexString(38-52)
hardhat-scripts/utils/overrides.ts (1)
hardhat-scripts/config/config.ts (1)
EVMX_CHAIN_ID(100-100)
hardhat-scripts/admin/disable-sb.ts (1)
hardhat-scripts/constants/constants.ts (1)
FAST_SWITCHBOARD_TYPE(9-9)
hardhat-scripts/utils/sign.ts (5)
hardhat-scripts/utils/address.ts (1)
getAddresses(7-23)hardhat-scripts/config/config.ts (2)
mode(6-8)EVMX_CHAIN_ID(100-100)src/types.ts (1)
EVMxAddressesObj(35-51)src/signer.ts (1)
signWatcherMultiCallMessage(3-25)hardhat-scripts/utils/deployUtils.ts (1)
getInstance(126-130)
hardhat-scripts/deploy/9.setupTransmitter.ts (2)
hardhat-scripts/config/config.ts (3)
transmitter(97-97)TRANSMITTER_CREDIT_THRESHOLD(125-125)TRANSMITTER_NATIVE_THRESHOLD(126-126)hardhat-scripts/utils/sign.ts (1)
getWatcherSigner(11-14)
hardhat-scripts/deploy/3.configureChains.ts (9)
hardhat-scripts/utils/deployUtils.ts (4)
DeployParams(31-36)getInstance(126-130)storeAddresses(143-170)updateContractSettings(323-362)hardhat-scripts/config/config.ts (5)
EVMX_CHAIN_ID(100-100)mode(6-8)testnetChains(76-80)chains(87-87)mainnetChains(81-85)hardhat-scripts/constants/constants.ts (3)
FAST_SWITCHBOARD_TYPE(9-9)CCTP_SWITCHBOARD_TYPE(10-10)MSG_SB_FEES(14-14)hardhat-scripts/utils/address.ts (1)
toBytes32FormatHexString(38-52)src/types.ts (1)
ChainAddressesObj(18-33)hardhat-scripts/utils/overrides.ts (1)
overrides(43-54)src/cctp.ts (1)
CCTP_DOMAINS(20-34)hardhat-scripts/constants/feeConstants.ts (1)
getFeeTokens(27-29)hardhat-scripts/utils/sign.ts (1)
getWatcherSigner(11-14)
hardhat-scripts/deploy/6.connect.ts (3)
hardhat-scripts/utils/gatewayId.ts (2)
getAppGatewayId(6-24)checkIfAppGatewayIdExists(26-39)hardhat-scripts/utils/appConfig.ts (2)
isConfigSetOnSocket(4-16)isConfigSetOnEVMx(18-33)hardhat-scripts/utils/address.ts (1)
toBytes32FormatHexString(38-52)
🔇 Additional comments (30)
hardhat.config.ts (1)
142-143: New EVMX explorer endpoints align with the conduit setup.This matches the active staging explorer host and keeps the Hardhat verify flow consistent. No further action.
script/super-token-solana/DeployEVMSolanaApps.s.sol (1)
26-26: Forwarder address update looks correct.
This should keep the deployment script aligned with the new ForwarderSolana instance.test/evmx/ProxyMigration.t.sol (1)
5-5: Path fix keeps mock import resolvableMockWatcherPrecompile moved up to
test/mock, so this should restore the compiler’s ability to locate it when running the migration tests.test/evmx/ProxyStorage.t.sol (1)
4-4: Import path fix matches shared SetupTest location.
This should keep the test wired to the relocated shared fixture; no issues spotted.trace.sh (1)
57-57: Confirmcast runaccepts--quick.
This should be verified because the Foundrycast runCLI does not document a--quickswitch; if the flag is unsupported, the trace command will exit immediately with “Found argument '--quick' which wasn't expected…”. Please runcast run --help(or the exact command) in the current toolchain and drop the flag if it is rejected.hardhat-scripts/s3Config/buildConfig.ts (1)
19-27: Explicit slug exposure aligns with the expanded S3Config contract. This should keep downstream consumers from guessing the EVMX slug and lets them rely on the config object instead.src/signer.ts (1)
10-24: Keep the nonce return type in sync with downstream expectations.
signatureNonceused to be anumber(viaDate.now()), and now resolves to a hex string. This should either update every consumer’s typing/serialization logic or keep the numeric surface. Please confirm every call site and type definition has been aligned with the newstringcontract; otherwise this will regress consumers still assumingnumber.script/counter/WithdrawFeesArbitrumFeesPlug.s.sol (1)
20-20: BalanceOf returns only withdrawable credits
This should be used for available funds: Credit.sol overrides balanceOf to exclude blocked credits; use totalBalanceOf/getBlockedCredits for raw or blocked values.Likely an incorrect or invalid review comment.
src/types.ts (1)
23-26: Keep fields required
All deployments JSON (e.g. deployments/*_addresses.json) already include MessageSwitchboard, FastSwitchboardId, CCTPSwitchboardId, MessageSwitchboardId and SwitchboardIdToAddressMap. Making them required preserves type safety without breaking any loaders.Errors.md (2)
105-110: No relocation needed for new access-control errors
OnlyRequestHandlerAllowed and OnlyPromiseResolverAllowed are already documented under utils/AccessControl.sol, not utils/common/Errors.Likely an incorrect or invalid review comment.
52-56: Verify error selectors match compiled artifacts
Errors.md sections (lines 52–56, 66–70, 73–75, 79–87, 97–104) must list 4-byte selectors matching eacherror Name(args)keccak-256 signature. This should be automated or manually confirmed to prevent silent mismatches.test/protocol/TriggerTest.t.sol (2)
13-20: Event signature update looks correct.Switching switchboard to uint64 aligns with ID-based refactor; payload/overrides types match updated interfaces.
60-68: Emitted values align with new switchboardId accessor.Using
arbConfig.switchboard.switchboardId()is consistent with the new API.contracts/evmx/interfaces/IAuctionManager.sol (1)
34-37: NatSpec aligns with new signature. Verify all callers updated.Given signature changes, ensure all off-chain callers and scripts have been updated to encode
data/returnDataas expected.contracts/protocol/interfaces/IMessageTransmitter.sol (2)
16-21: sendMessage signature looks correctMatches CCTP usage (domain, bytes32 recipient, bytes payload → uint64 nonce).
28-31: receiveMessage signature looks correctMatches CCTP usage (bytes message + bytes attestation → bool).
deployments/dev_verification.json (2)
13-17: Validate FeesPool constructor dependency Confirm the passed address (0xb62505feacC486e809392c65614Ce4d7b051923b) matches the intended resolver/owner/token contract on network 7625382; an incorrect address will brick fee flows at runtime.
25-41: Ensure SchedulePrecompile timeouts align with trigger cadence
Ensure the updated BigNumber hex values (0x02540be400, 0xe8d4a51000) and 3600s timeout in deployments/dev_verification.json match the intended trigger cadence to prevent premature expiries or backlog.contracts/evmx/base/AppGatewayBase.sol (2)
234-235: No action needed: approveWithSignature tuple matches interface
Interface and implementation both return (address consumeFrom, address spender, bool approval); destructuring(consumeFrom, , )correctly captures the first value.
124-129: Downstream decode matches updated packing
onDeployComplete decodes onCompleteData_ as a single uint32, aligning with the newabi.encode(chainSlug_)wrapper.contracts/evmx/watcher/RequestHandler.sol (2)
426-428: Keep three-value tryCall destructuring
This should remain; tryCall returns (bool success, bool exceededMaxCopy, bytes memory returnData) in this repo (see contracts/protocol/SocketUtils.sol:133).Likely an incorrect or invalid review comment.
324-324: Check transferFrom return or use SafeERC20; ensure allowance• Use
SafeERC20.safeTransferFromor verify the boolean from
IERC20(address(feesManager__())).transferFrom(...)(e.g.
if (!IERC20(...).transferFrom(...)) revert TransferFailed();) to avoid silent failures.
• EnsurewatcherFeesPayerhas approved this contract to spend ≥totalFeeson the fee token (feesManager__()).Add
error TransferFailed();near other custom errors.contracts/protocol/Socket.sol (1)
242-257: Fallback path mirrors trigger entrypoint correctly.The fallback returns the encoded triggerId and preserves payable semantics while
receiverejects plain ETH as intended. This aligns with tests.contracts/protocol/interfaces/ISocket.sol (2)
86-87: Removed function that is still referenced in the interface documentationThe
registerSwitchboard()function is removed but line 10 still mentions "setting up switchboards" as part of the interface's purpose. This inconsistency needs to be fixed.Apply this diff to update the interface documentation:
* @dev This interface provides methods for transmitting and executing payloads, - * connecting a plug to a remote chain and setting up switchboards for the payload transmission + * connecting a plug to a remote chain for payload transmission * This interface also emits events for important operations such as payload transmission, execution status,Likely an incorrect or invalid review comment.
75-76: No internal address‐type connect() calls remain
All internal connect() calls now pass a uint64 switchboardId; external integrations must update their calls to use uint64 rather than address.test/protocol/switchboards/FastSwitchboardTest.t.sol (2)
10-32: Test extension doesn't validate switchboard behaviorThe
FastSwitchboardExtendedcontract only emits an event inprocessTriggerwithout any actual processing logic. This doesn't test whether the base contract would handle triggers correctly if it had implementation.The test should either:
- Test the actual
processTriggerimplementation if it exists in the base contract- Remove this extended contract if
processTriggeris meant to be abstract/empty- Add assertions that verify the expected no-op behavior is intentional
function processTrigger( address plug_, bytes32 triggerId_, bytes calldata payload_, bytes calldata overrides_ ) external payable override { + // Verify base implementation is called if needed + // super.processTrigger(plug_, triggerId_, payload_, overrides_); emit TriggerProcessed(plug_, triggerId_, payload_, overrides_); }Likely an incorrect or invalid review comment.
154-166: Test correctly asserts InvalidSignature
largeProof’s pattern (invalid v byte + wrong length) guarantees_recoverSignerreverts withInvalidSignature. attest can’t hit other errors here. No change required.deployments/stage_addresses.json (2)
13-13: ValidatestartBlockagainst deployment block
This should fetch the contract’s deployment receipt viaeth_getTransactionReceipton your RPC for chain ID 14323 (a custom/private network), extract itsblockNumber, and updatestartBlockindeployments/stage_addresses.json(line 13) accordingly for accurate event indexing.
38-62: Ignore contract set asymmetry on chain 14323
Chains vary by design: 14323 uses resolver/impl pattern; other chains use switchboard contracts.Likely an incorrect or invalid review comment.
FunctionSignatures.md (1)
19-22: Verify selector doc matches implementation
Selector table may be stale—script returned no matches. Recompute 4-byte selectors for all public/external functions (e.g. via compiled ABIs withforge inspect … selectorsor a correct keccak-256 script against your Solidity sources) and diff against FunctionSignatures.md to catch any mismatches.
| function endAuction(bytes memory data, bytes memory) external onlyPromises { | ||
| uint40 requestCount_ = abi.decode(data, (uint40)); | ||
| if (requestCount_ == 0) revert InvalidBid(); |
There was a problem hiding this comment.
Add the missing override on endAuction.
This should still fulfil the IAuctionManager interface. Removing override breaks compilation because the interface now declares endAuction(bytes,bytes), so the implementation must be marked override.
Apply this diff to fix the override:
-function endAuction(bytes memory data, bytes memory) external onlyPromises {
+function endAuction(bytes memory data, bytes memory) external override onlyPromises {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function endAuction(bytes memory data, bytes memory) external onlyPromises { | |
| uint40 requestCount_ = abi.decode(data, (uint40)); | |
| if (requestCount_ == 0) revert InvalidBid(); | |
| function endAuction(bytes memory data, bytes memory) external override onlyPromises { | |
| uint40 requestCount_ = abi.decode(data, (uint40)); | |
| if (requestCount_ == 0) revert InvalidBid(); |
🤖 Prompt for AI Agents
In contracts/evmx/AuctionManager.sol around lines 173 to 175, the endAuction
function is missing the required override specifier for the IAuctionManager
interface; update the function signature to include the override keyword (e.g.,
endAuction(bytes memory data, bytes memory) external override onlyPromises) so
the implementation explicitly overrides the interface declaration and compiles
cleanly.
| IERC20(address(feesManager__())).transferFrom(from_, to_, watcherFees); | ||
| } |
There was a problem hiding this comment.
Require success from transferFrom.
This should revert when the ERC20 returns false; otherwise schedule-fee collection can silently fail on non-reverting tokens (e.g., USDT-style). Add an explicit success check.
Apply this diff to enforce the check:
- IERC20(address(feesManager__())).transferFrom(from_, to_, watcherFees);
+ bool transferOk = IERC20(address(feesManager__())).transferFrom(from_, to_, watcherFees);
+ require(transferOk, "fees transfer failed");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IERC20(address(feesManager__())).transferFrom(from_, to_, watcherFees); | |
| } | |
| bool transferOk = IERC20(address(feesManager__())).transferFrom(from_, to_, watcherFees); | |
| require(transferOk, "fees transfer failed"); | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/AuctionManager.sol around lines 266-267, the call to
IERC20(address(feesManager__())).transferFrom(from_, to_, watcherFees) does not
check the return value which allows silent failure on tokens that return false;
update the code to require the transferFrom call succeeds (e.g.,
require(IERC20(...).transferFrom(...) == true, "fees transfer failed")) or,
preferably, switch to OpenZeppelin's SafeERC20 and call safeTransferFrom to
handle non-standard ERC20s; ensure the transfer is reverted on failure and add
an appropriate error message.
| feesManager__().approve(address(feesManager__()), true); | ||
| feesManager__().withdrawCredits(chainSlug_, token_, amount_, maxFees, receiver_); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Likely incorrect approval semantics before withdraw
Calling approve(address(feesManager__()), true) on FeesManager is suspicious:
- ERC20-style approvals take (spender, amount), not (spender, bool).
- Approving the FeesManager on the FeesManager itself is odd unless FeesManager is also the credit token and implements non‑ERC20 semantics.
This should approve allowances on the actual credit token for the FeesManager (spender) with a bounded amount, or invoke a dedicated authorization API that matches your credit model.
If FeesManager uses ERC20 semantics, the fix should look like:
- obtain the credit token (e.g., feesManager__().creditToken())
- call creditToken.approve(address(feesManager__()), amount_)
- then call withdrawCredits(...)
Please confirm the correct token/amount variables and I’ll provide an exact patch.
🏁 Script executed:
#!/bin/bash
# Verify the approve() signature on FeesManager and whether FeesManager is the token
rg -n -C2 -P 'interface\s+IFeesManager|contract\s+FeesManager' --type=sol
rg -n -C2 -P '\bfunction\s+approve\s*\(' --type=sol
rg -n -C2 -P '\bwithdrawCredits\s*\(' --type=solLength of output: 83
🏁 Script executed:
#!/bin/bash
set -e
# Locate FeesManager definition and its key methods in Solidity files
rg -n -C2 -P 'contract\s+FeesManager' -g '*.sol'
rg -n -C2 -P 'interface\s+IFeesManager' -g '*.sol'
rg -n -C2 -P 'function\s+approve\s*\(' -g '*.sol'
rg -n -C2 -P 'function\s+withdrawCredits\s*\(' -g '*.sol'
rg -n -C2 -P 'creditToken' -g '*.sol'Length of output: 4456
🏁 Script executed:
#!/bin/bash
set -e
# Show context around approve and withdrawCredits calls in AppGatewayBase.sol
rg -n -C5 'feesManager__|approve|withdrawCredits' -g 'contracts/evmx/base/AppGatewayBase.sol'Length of output: 2499
Incorrect approve overload before withdrawCredits
Calling feesManager__().approve(address(feesManager__()), true) invokes the bool‐based IFeesManager overload and sets the wrong allowance. This should call the ERC20 approve(spender, amount) on the FeesManager token for amount_ + maxFees (e.g. IERC20(address(feesManager__())).approve(address(feesManager__()), amount_ + maxFees)) before withdrawCredits.
🤖 Prompt for AI Agents
In contracts/evmx/base/AppGatewayBase.sol around lines 248-250, the current call
feesManager__().approve(address(feesManager__()), true) calls the wrong overload
(bool) and sets an incorrect allowance; replace it with an ERC20 approve on the
FeesManager token contract so the feesManager contract is allowed to transfer
amount_ + maxFees (e.g. call
IERC20(address(feesManager__())).approve(address(feesManager__()), amount_ +
maxFees)) before invoking withdrawCredits, ensuring you pass the summed amount
and the correct IERC20 approve signature.
| function setSusdcToken(uint32 chainSlug_, bytes32 susdcToken_) external onlyOwner { | ||
| forwarderAddresses[susdcToken][chainSlug_] = asyncDeployer__().getOrDeployForwarderContract( | ||
| susdcToken_, | ||
| chainSlug_ | ||
| ); | ||
| _setValidPlug(true, chainSlug_, susdcToken); | ||
| emit SusdcTokenSet(chainSlug_, susdcToken_); | ||
| } |
There was a problem hiding this comment.
Wrong variable used and missing assignment in setSusdcToken
Uses state susdcToken instead of the function arg and never assigns the state variable. Forwarder mapping and validity are set for zero token id.
- function setSusdcToken(uint32 chainSlug_, bytes32 susdcToken_) external onlyOwner {
- forwarderAddresses[susdcToken][chainSlug_] = asyncDeployer__().getOrDeployForwarderContract(
- susdcToken_,
- chainSlug_
- );
- _setValidPlug(true, chainSlug_, susdcToken);
- emit SusdcTokenSet(chainSlug_, susdcToken_);
- }
+ function setSusdcToken(uint32 chainSlug_, bytes32 susdcToken_) external onlyOwner {
+ susdcToken = susdcToken_;
+ forwarderAddresses[susdcToken][chainSlug_] =
+ asyncDeployer__().getOrDeployForwarderContract(susdcToken, chainSlug_);
+ _setValidPlug(true, chainSlug_, susdcToken);
+ emit SusdcTokenSet(chainSlug_, susdcToken);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function setSusdcToken(uint32 chainSlug_, bytes32 susdcToken_) external onlyOwner { | |
| forwarderAddresses[susdcToken][chainSlug_] = asyncDeployer__().getOrDeployForwarderContract( | |
| susdcToken_, | |
| chainSlug_ | |
| ); | |
| _setValidPlug(true, chainSlug_, susdcToken); | |
| emit SusdcTokenSet(chainSlug_, susdcToken_); | |
| } | |
| function setSusdcToken(uint32 chainSlug_, bytes32 susdcToken_) external onlyOwner { | |
| // assign the state variable so subsequent calls use the correct token | |
| susdcToken = susdcToken_; | |
| // now use the updated state variable as the key | |
| forwarderAddresses[susdcToken][chainSlug_] = | |
| asyncDeployer__().getOrDeployForwarderContract(susdcToken, chainSlug_); | |
| _setValidPlug(true, chainSlug_, susdcToken); | |
| emit SusdcTokenSet(chainSlug_, susdcToken); | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/fees/Credit.sol around lines 129 to 136, the function
setSusdcToken uses the state variable susdcToken instead of the function
argument susdcToken_ and never assigns the state variable, causing mappings and
validity to be set for the zero token id; fix by assigning the state variable
(susdcToken = susdcToken_) at the start, use susdcToken_ for the
forwarderAddresses and _setValidPlug calls, and emit SusdcTokenSet with
susdcToken_ (or the updated state variable) so the mapping, validity and event
reflect the intended token id.
contracts/evmx/fees/Credit.sol
Outdated
| // Mint tokens to the user | ||
| _mint(depositTo_, creditAmount_); | ||
|
|
||
| if (nativeAmount_ > 0) { | ||
| // if native transfer fails, add to credit | ||
| bool success = feesPool.withdraw(depositTo_, nativeAmount_); | ||
|
|
||
| if (!success) { | ||
| userCredit.totalCredits += nativeAmount_; | ||
| _mint(depositTo_, creditAmount_); | ||
| nativeAmount_ = 0; | ||
| creditAmount_ += nativeAmount_; | ||
| } |
There was a problem hiding this comment.
deposit mints wrong amount on native transfer failure
On failure it mints creditAmount_ again and then adds zero (after zeroing nativeAmount_). This double-mints and reports wrong amounts in onTransfer/event.
- if (nativeAmount_ > 0) {
+ if (nativeAmount_ > 0) {
// if native transfer fails, add to credit
bool success = feesPool.withdraw(depositTo_, nativeAmount_);
if (!success) {
- _mint(depositTo_, creditAmount_);
- nativeAmount_ = 0;
- creditAmount_ += nativeAmount_;
+ _mint(depositTo_, nativeAmount_);
+ creditAmount_ += nativeAmount_;
+ nativeAmount_ = 0;
}
}This ensures total minted credits equal creditAmount_ + nativeAmount_ when ETH transfer fails.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Mint tokens to the user | |
| _mint(depositTo_, creditAmount_); | |
| if (nativeAmount_ > 0) { | |
| // if native transfer fails, add to credit | |
| bool success = feesPool.withdraw(depositTo_, nativeAmount_); | |
| if (!success) { | |
| userCredit.totalCredits += nativeAmount_; | |
| _mint(depositTo_, creditAmount_); | |
| nativeAmount_ = 0; | |
| creditAmount_ += nativeAmount_; | |
| } | |
| // Mint tokens to the user | |
| _mint(depositTo_, creditAmount_); | |
| if (nativeAmount_ > 0) { | |
| // if native transfer fails, add to credit | |
| bool success = feesPool.withdraw(depositTo_, nativeAmount_); | |
| if (!success) { | |
| _mint(depositTo_, nativeAmount_); | |
| creditAmount_ += nativeAmount_; | |
| nativeAmount_ = 0; | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/fees/Credit.sol around lines 158-169, the failure branch
double-mints creditAmount_ because it calls _mint(depositTo_, creditAmount_)
again and then zeroes nativeAmount_, so the failed nativeAmount_ is never
actually added to credits; instead replace the second mint with minting the
failed nativeAmount_ and add that amount to creditAmount_ before zeroing
nativeAmount_. Concretely: on withdraw failure, call _mint(depositTo_,
nativeAmount_), then set creditAmount_ = creditAmount_ + nativeAmount_, then set
nativeAmount_ = 0 (do not mint creditAmount_ again).
| function testSchedulePrecompileSetExpiryTime() public { | ||
| uint256 expiryTime = expiryTime + 100; | ||
| hoax(watcherAddress); | ||
| vm.expectEmit(true, true, true, true); | ||
| emit SchedulePrecompile.ExpiryTimeSet(expiryTime); | ||
| schedulePrecompile.setExpiryTime(expiryTime); | ||
|
|
||
| hoax(nonOwner); | ||
| vm.expectRevert(abi.encodeWithSelector(OnlyWatcherAllowed.selector)); | ||
| schedulePrecompile.setExpiryTime(expiryTime); | ||
| } |
There was a problem hiding this comment.
Fix self-referential variable initialization; this won’t compile.
uint256 expiryTime = expiryTime + 100; shadows any outer expiryTime and reads the uninitialized local. Use a different local name to reference the intended outer/state variable.
Apply this diff:
- uint256 expiryTime = expiryTime + 100;
+ uint256 newExpiryTime = expiryTime + 100;
hoax(watcherAddress);
vm.expectEmit(true, true, true, true);
- emit SchedulePrecompile.ExpiryTimeSet(expiryTime);
- schedulePrecompile.setExpiryTime(expiryTime);
+ emit SchedulePrecompile.ExpiryTimeSet(newExpiryTime);
+ schedulePrecompile.setExpiryTime(newExpiryTime);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function testSchedulePrecompileSetExpiryTime() public { | |
| uint256 expiryTime = expiryTime + 100; | |
| hoax(watcherAddress); | |
| vm.expectEmit(true, true, true, true); | |
| emit SchedulePrecompile.ExpiryTimeSet(expiryTime); | |
| schedulePrecompile.setExpiryTime(expiryTime); | |
| hoax(nonOwner); | |
| vm.expectRevert(abi.encodeWithSelector(OnlyWatcherAllowed.selector)); | |
| schedulePrecompile.setExpiryTime(expiryTime); | |
| } | |
| function testSchedulePrecompileSetExpiryTime() public { | |
| uint256 newExpiryTime = expiryTime + 100; | |
| hoax(watcherAddress); | |
| vm.expectEmit(true, true, true, true); | |
| emit SchedulePrecompile.ExpiryTimeSet(newExpiryTime); | |
| schedulePrecompile.setExpiryTime(newExpiryTime); | |
| hoax(nonOwner); | |
| vm.expectRevert(abi.encodeWithSelector(OnlyWatcherAllowed.selector)); | |
| schedulePrecompile.setExpiryTime(expiryTime); | |
| } |
🤖 Prompt for AI Agents
In test/evmx/Watcher.t.sol around lines 786 to 796, the local variable
declaration uint256 expiryTime = expiryTime + 100; is self-referential and will
not compile; replace it with a new local name (e.g., uint256 newExpiry =
expiryTime + 100;) or otherwise reference the existing state/fixture expiryTime
and use that newLocal when emitting and calling setExpiryTime; ensure subsequent
uses in the test (emit expectation and setExpiryTime calls) use the new local
name and leave the original state variable untouched.
| /// @notice Mint a new token to `to` with a specific `tokenId` | ||
| function mint(address to, uint256 tokenId) public { | ||
| _mint(to, tokenId); | ||
| } | ||
|
|
||
| /// @notice Mint a new token to `to` with auto-incremented tokenId | ||
| function mint(address to) public returns (uint256 tokenId) { | ||
| tokenId = _nextTokenId++; | ||
| _mint(to, tokenId); | ||
| } |
There was a problem hiding this comment.
Fix auto-increment state after manual mints.
Manual mint(to, tokenId) leaves _nextTokenId unchanged. After minting token 1 manually, the auto-mint path still hands out 1 and reverts. This should advance _nextTokenId whenever a manual mint lands on or above the current cursor so the sequential mint keeps working.
/// @notice Mint a new token to `to` with a specific `tokenId`
function mint(address to, uint256 tokenId) public {
_mint(to, tokenId);
+ uint256 nextId = tokenId + 1;
+ if (nextId > _nextTokenId) {
+ _nextTokenId = nextId;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @notice Mint a new token to `to` with a specific `tokenId` | |
| function mint(address to, uint256 tokenId) public { | |
| _mint(to, tokenId); | |
| } | |
| /// @notice Mint a new token to `to` with auto-incremented tokenId | |
| function mint(address to) public returns (uint256 tokenId) { | |
| tokenId = _nextTokenId++; | |
| _mint(to, tokenId); | |
| } | |
| /// @notice Mint a new token to `to` with a specific `tokenId` | |
| function mint(address to, uint256 tokenId) public { | |
| _mint(to, tokenId); | |
| uint256 nextId = tokenId + 1; | |
| if (nextId > _nextTokenId) { | |
| _nextTokenId = nextId; | |
| } | |
| } | |
| /// @notice Mint a new token to `to` with auto-incremented tokenId | |
| function mint(address to) public returns (uint256 tokenId) { | |
| tokenId = _nextTokenId++; | |
| _mint(to, tokenId); | |
| } |
🤖 Prompt for AI Agents
In test/mock/MockERC721.sol around lines 21 to 30, the manual mint function does
not advance the auto-increment cursor, causing subsequent auto-mints to reuse an
existing tokenId; update the manual mint(path mint(address to, uint256 tokenId))
so that after a successful _mint call, if tokenId >= _nextTokenId then set
_nextTokenId = tokenId + 1 (ensuring the sequential mint path will continue from
the next unused id).
| function initSocket( | ||
| bytes32 appGatewayId_, | ||
| address socket_, | ||
| uint64 switchboardId_ | ||
| ) external override { | ||
| appGatewayId = appGatewayId_; | ||
| socket__ = ISocket(socket_); | ||
| initSocketCalled = true; | ||
|
|
||
| socket__.connect(appGatewayId_, switchboardId_); | ||
| } | ||
|
|
There was a problem hiding this comment.
Persist switchboardId in initSocket
State var switchboardId is never set. Tests depending on it will read zero.
function initSocket(
bytes32 appGatewayId_,
address socket_,
uint64 switchboardId_
) external override {
appGatewayId = appGatewayId_;
socket__ = ISocket(socket_);
+ switchboardId = switchboardId_;
initSocketCalled = true;
socket__.connect(appGatewayId_, switchboardId_);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function initSocket( | |
| bytes32 appGatewayId_, | |
| address socket_, | |
| uint64 switchboardId_ | |
| ) external override { | |
| appGatewayId = appGatewayId_; | |
| socket__ = ISocket(socket_); | |
| initSocketCalled = true; | |
| socket__.connect(appGatewayId_, switchboardId_); | |
| } | |
| function initSocket( | |
| bytes32 appGatewayId_, | |
| address socket_, | |
| uint64 switchboardId_ | |
| ) external override { | |
| appGatewayId = appGatewayId_; | |
| socket__ = ISocket(socket_); | |
| switchboardId = switchboardId_; | |
| initSocketCalled = true; | |
| socket__.connect(appGatewayId_, switchboardId_); | |
| } |
🤖 Prompt for AI Agents
In test/mock/MockPlug.sol around lines 20 to 31, the initSocket function calls
socket__.connect(...) but never assigns the incoming switchboardId_ to the
contract's state variable, so switchboardId remains zero for tests; fix this by
setting switchboardId = switchboardId_ (assign the state var before calling
connect) so the value is persisted for later reads.
| function depositNativeAndCreditsWithData( | ||
| uint32 chainSlug_, | ||
| uint256 credits_, | ||
| uint256 native_, | ||
| address user_, | ||
| address receiver_, | ||
| bytes memory data_ | ||
| ) internal { | ||
| SocketContracts memory socketConfig = getSocketConfig(chainSlug_); | ||
| TestUSDC token = socketConfig.testUSDC; | ||
|
|
||
| uint256 userBalance = token.balanceOf(user_); | ||
| uint256 feesPlugBalance = token.balanceOf(address(socketConfig.feesPlug)); | ||
| // uint256 userBalance = token.balanceOf(user_); | ||
| // uint256 feesPlugBalance = token.balanceOf(address(socketConfig.feesPlug)); | ||
|
|
||
| token.mint(address(user_), 100 ether); | ||
| assertEq( | ||
| token.balanceOf(user_), | ||
| userBalance + 100 ether, | ||
| "User should have 100 more test tokens" | ||
| ); | ||
| // assertEq( | ||
| // token.balanceOf(user_), | ||
| // userBalance + 100 ether, | ||
| // "User should have 100 more test tokens" | ||
| // ); | ||
|
|
||
| vm.startPrank(user_); | ||
| token.approve(address(socketConfig.feesPlug), 100 ether); | ||
| socketConfig.feesPlug.depositCreditAndNative(address(token), address(user_), 100 ether); | ||
| socketConfig.feesPlug.depositCreditAndNative(address(token), user_, 100 ether, data_); | ||
| vm.stopPrank(); | ||
|
|
||
| assertEq( | ||
| token.balanceOf(address(socketConfig.feesPlug)), | ||
| feesPlugBalance + 100 ether, | ||
| "Fees plug should have 100 more test tokens" | ||
| ); | ||
| // assertEq( | ||
| // token.balanceOf(address(socketConfig.feesPlug)), | ||
| // feesPlugBalance + 100 ether, | ||
| // "Fees plug should have 100 more test tokens" | ||
| // ); | ||
|
|
||
| uint256 currentCredits = feesManager.getAvailableCredits(user_); | ||
| uint256 currentNative = address(user_).balance; | ||
| // uint256 currentCredits = feesManager.balanceOf(user_); | ||
| // uint256 currentNative = address(user_).balance; | ||
|
|
||
| vm.expectEmit(true, true, true, false); | ||
| emit Deposited(chainSlug_, address(token), user_, credits_, native_); | ||
| emit Deposited(chainSlug_, address(token), receiver_, credits_, native_); | ||
|
|
||
| watcherMultiCall( | ||
| address(feesManager), | ||
| abi.encodeWithSelector( | ||
| Credit.deposit.selector, | ||
| chainSlug_, | ||
| address(token), | ||
| user_, | ||
| receiver_, | ||
| native_, | ||
| credits_ | ||
| credits_, | ||
| data_ | ||
| ) |
There was a problem hiding this comment.
Use the caller-provided credit/native amounts
This helper should honor the credits_, native_, and receiver_ arguments. Hard-coding 100 ether and omitting the native value means every caller gets the wrong balances and native escrow never gets funded. Fix the mint/approve/deposit amounts and forward the native value.
- token.mint(address(user_), 100 ether);
+ token.mint(address(user_), credits_);
@@
- token.approve(address(socketConfig.feesPlug), 100 ether);
- socketConfig.feesPlug.depositCreditAndNative(address(token), user_, 100 ether, data_);
+ token.approve(address(socketConfig.feesPlug), credits_);
+ socketConfig.feesPlug.depositCreditAndNative{value: native_}(
+ address(token),
+ receiver_,
+ credits_,
+ data_
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function depositNativeAndCreditsWithData( | |
| uint32 chainSlug_, | |
| uint256 credits_, | |
| uint256 native_, | |
| address user_, | |
| address receiver_, | |
| bytes memory data_ | |
| ) internal { | |
| SocketContracts memory socketConfig = getSocketConfig(chainSlug_); | |
| TestUSDC token = socketConfig.testUSDC; | |
| uint256 userBalance = token.balanceOf(user_); | |
| uint256 feesPlugBalance = token.balanceOf(address(socketConfig.feesPlug)); | |
| // uint256 userBalance = token.balanceOf(user_); | |
| // uint256 feesPlugBalance = token.balanceOf(address(socketConfig.feesPlug)); | |
| token.mint(address(user_), 100 ether); | |
| assertEq( | |
| token.balanceOf(user_), | |
| userBalance + 100 ether, | |
| "User should have 100 more test tokens" | |
| ); | |
| // assertEq( | |
| // token.balanceOf(user_), | |
| // userBalance + 100 ether, | |
| // "User should have 100 more test tokens" | |
| // ); | |
| vm.startPrank(user_); | |
| token.approve(address(socketConfig.feesPlug), 100 ether); | |
| socketConfig.feesPlug.depositCreditAndNative(address(token), address(user_), 100 ether); | |
| socketConfig.feesPlug.depositCreditAndNative(address(token), user_, 100 ether, data_); | |
| vm.stopPrank(); | |
| assertEq( | |
| token.balanceOf(address(socketConfig.feesPlug)), | |
| feesPlugBalance + 100 ether, | |
| "Fees plug should have 100 more test tokens" | |
| ); | |
| // assertEq( | |
| // token.balanceOf(address(socketConfig.feesPlug)), | |
| // feesPlugBalance + 100 ether, | |
| // "Fees plug should have 100 more test tokens" | |
| // ); | |
| uint256 currentCredits = feesManager.getAvailableCredits(user_); | |
| uint256 currentNative = address(user_).balance; | |
| // uint256 currentCredits = feesManager.balanceOf(user_); | |
| // uint256 currentNative = address(user_).balance; | |
| vm.expectEmit(true, true, true, false); | |
| emit Deposited(chainSlug_, address(token), user_, credits_, native_); | |
| emit Deposited(chainSlug_, address(token), receiver_, credits_, native_); | |
| watcherMultiCall( | |
| address(feesManager), | |
| abi.encodeWithSelector( | |
| Credit.deposit.selector, | |
| chainSlug_, | |
| address(token), | |
| user_, | |
| receiver_, | |
| native_, | |
| credits_ | |
| credits_, | |
| data_ | |
| ) | |
| function depositNativeAndCreditsWithData( | |
| uint32 chainSlug_, | |
| uint256 credits_, | |
| uint256 native_, | |
| address user_, | |
| address receiver_, | |
| bytes memory data_ | |
| ) internal { | |
| SocketContracts memory socketConfig = getSocketConfig(chainSlug_); | |
| TestUSDC token = socketConfig.testUSDC; | |
| // uint256 userBalance = token.balanceOf(user_); | |
| // uint256 feesPlugBalance = token.balanceOf(address(socketConfig.feesPlug)); | |
| // mint only what the caller asked for | |
| token.mint(address(user_), credits_); | |
| // assertEq( | |
| // token.balanceOf(user_), | |
| // userBalance + credits_, | |
| // "User should have credits_ more test tokens" | |
| // ); | |
| vm.startPrank(user_); | |
| // approve exactly credits_ | |
| token.approve(address(socketConfig.feesPlug), credits_); | |
| // forward the native value and use the provided receiver_ | |
| socketConfig.feesPlug.depositCreditAndNative{value: native_}( | |
| address(token), | |
| receiver_, | |
| credits_, | |
| data_ | |
| ); | |
| vm.stopPrank(); | |
| // assertEq( | |
| // token.balanceOf(address(socketConfig.feesPlug)), | |
| // feesPlugBalance + credits_, | |
| // "Fees plug should have credits_ more test tokens" | |
| // ); | |
| // uint256 currentCredits = feesManager.balanceOf(user_); | |
| // uint256 currentNative = address(user_).balance; | |
| vm.expectEmit(true, true, true, false); | |
| emit Deposited(chainSlug_, address(token), receiver_, credits_, native_); | |
| watcherMultiCall( | |
| address(feesManager), | |
| abi.encodeWithSelector( | |
| Credit.deposit.selector, | |
| chainSlug_, | |
| address(token), | |
| receiver_, | |
| native_, | |
| credits_, | |
| data_ | |
| ) |
| function testConvertersFuzz(address addr) public pure { | ||
| // Skip addresses that would cause overflow | ||
| vm.assume(uint256(uint160(addr)) <= type(uint160).max); | ||
|
|
||
| bytes32 bytes32Format = toBytes32Format(addr); | ||
| address convertedAddr = fromBytes32Format(bytes32Format); | ||
|
|
||
| assertEq(convertedAddr, addr, "Fuzz test should pass"); |
There was a problem hiding this comment.
Drop pure from testConvertersFuzz
This should compile, but vm.assume is a cheatcode call with non‑pure mutability. Declaring the test pure makes the compiler reject the file (TypeError: Function declared as pure but this expression (cheatcode call) could change the state). Remove pure (or mark the test virtual override etc.) so it builds.
Apply this diff:
- function testConvertersFuzz(address addr) public pure {
+ function testConvertersFuzz(address addr) public {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function testConvertersFuzz(address addr) public pure { | |
| // Skip addresses that would cause overflow | |
| vm.assume(uint256(uint160(addr)) <= type(uint160).max); | |
| bytes32 bytes32Format = toBytes32Format(addr); | |
| address convertedAddr = fromBytes32Format(bytes32Format); | |
| assertEq(convertedAddr, addr, "Fuzz test should pass"); | |
| function testConvertersFuzz(address addr) public { | |
| // Skip addresses that would cause overflow | |
| vm.assume(uint256(uint160(addr)) <= type(uint160).max); | |
| bytes32 bytes32Format = toBytes32Format(addr); | |
| address convertedAddr = fromBytes32Format(bytes32Format); | |
| assertEq(convertedAddr, addr, "Fuzz test should pass"); |
🤖 Prompt for AI Agents
In test/Utils.t.sol around lines 140 to 147, the test function is declared
`public pure` but calls the non‑pure cheatcode `vm.assume`, causing a compiler
error; remove the `pure` modifier from the function signature (make it `public`
or another appropriate visibility without `pure`) so the cheatcode call is
allowed and the file compiles.
| uint160 payloadPointer = (uint160(chainSlug) << 120) | | ||
| (uint160(uint64(uint256(triggerId_))) << 80) | | ||
| payloadCounter++; | ||
|
|
||
| bytes32 payloadId = createPayloadId(payloadPointer, switchboardId, dstChainSlug_); | ||
|
|
There was a problem hiding this comment.
Non-overlapping bit packing for payloadPointer
Current shifts overlap (chainSlug at bits 120–151 and trigger bits at 80–143) corrupting the pointer. Pack into disjoint ranges.
Apply:
- uint160 payloadPointer = (uint160(chainSlug) << 120) |
- (uint160(uint64(uint256(triggerId_))) << 80) |
- payloadCounter++;
+ // [159..128]=chainSlug(32b), [127..64]=trigger low(64b), [63..0]=counter(64b)
+ uint64 counter = payloadCounter++;
+ uint160 payloadPointer =
+ (uint160(chainSlug) << 128) |
+ (uint160(uint64(uint256(triggerId_))) << 64) |
+ uint160(counter);And widen the counter storage (Line 29):
- uint40 public payloadCounter;
+ uint64 public payloadCounter;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint160 payloadPointer = (uint160(chainSlug) << 120) | | |
| (uint160(uint64(uint256(triggerId_))) << 80) | | |
| payloadCounter++; | |
| bytes32 payloadId = createPayloadId(payloadPointer, switchboardId, dstChainSlug_); | |
| // [159..128]=chainSlug(32b), [127..64]=trigger low(64b), [63..0]=counter(64b) | |
| uint64 counter = payloadCounter++; | |
| uint160 payloadPointer = | |
| (uint160(chainSlug) << 128) | | |
| (uint160(uint64(uint256(triggerId_))) << 64) | | |
| uint160(counter); | |
| bytes32 payloadId = createPayloadId(payloadPointer, switchboardId, dstChainSlug_); |
🤖 Prompt for AI Agents
In contracts/protocol/switchboard/MessageSwitchboard.sol around lines 161 to
166, the bit shifts for building payloadPointer overlap (chainSlug and triggerId
ranges collide); fix by assigning non-overlapping bit offsets (e.g. keep the low
80 bits for payloadCounter, shift triggerId into the next 64-bit slot by 80
bits, and shift chainSlug into a higher slot by 144 bits) and update the
payloadCounter storage type (line 29) to a wider uint (uint80) so it fits the
allocated low bits; implement payloadPointer as: (uint160(chainSlug) << 144) |
(uint160(uint64(uint256(triggerId_))) << 80) | uint160(payloadCounter++) and
change the payloadCounter declaration on line 29 to uint80 payloadCounter.
| function attest(DigestParams calldata digest_, bytes calldata proof_) public { | ||
| (uint32 srcChainSlug, bytes32 srcPlug) = abi.decode(digest_.extraData, (uint32, bytes32)); | ||
| if (siblingPlugs[srcChainSlug][address(uint160(uint256(digest_.target)))] != srcPlug) { | ||
| revert InvalidTargetVerification(); | ||
| } | ||
| bytes32 digest = _createDigest(digest_); | ||
| address watcher = _recoverSigner( | ||
| keccak256(abi.encodePacked(toBytes32Format(address(this)), chainSlug, digest)), | ||
| proof_ | ||
| ); | ||
| if (!_hasRole(WATCHER_ROLE, watcher)) revert WatcherNotFound(); | ||
|
|
||
| if (isAttested[digest]) revert AlreadyAttested(); | ||
| isAttested[digest] = true; | ||
|
|
||
| emit Attested(digest_.payloadId, digest, watcher); | ||
| } |
There was a problem hiding this comment.
Enforce digest deadline in attest
attest ignores digest_.deadline. This should reject stale digests to prevent replay of expired intents.
function attest(DigestParams calldata digest_, bytes calldata proof_) public {
+ if (digest_.deadline < block.timestamp) revert("DigestExpired");If you have a shared error type, use it instead of a string.
🤖 Prompt for AI Agents
In contracts/protocol/switchboard/MessageSwitchboard.sol around lines 190 to
206, the attest function currently ignores digest_.deadline which allows replay
of expired digests; add a check early in the function (before any state changes
or signer recovery) to compare digest_.deadline with block.timestamp and revert
if the deadline has passed, using the project’s shared error type if one exists
(otherwise introduce a descriptive custom error), so stale digests are rejected
and execution halts before marking isAttested or emitting events.
| bool public payAndCheckFeesCalled = false; | ||
| ExecuteParams public lastExecuteParams; | ||
| TransmissionParams public lastTransmissionParams; | ||
|
|
||
| function payAndCheckFees( | ||
| ExecuteParams memory executeParams_, | ||
| TransmissionParams memory transmissionParams_ | ||
| ) external payable override { | ||
| payAndCheckFeesCalled = true; | ||
| lastExecuteParams = executeParams_; | ||
| lastTransmissionParams = transmissionParams_; | ||
| } | ||
|
|
||
| function getMinSocketFees() external pure override returns (uint256) { | ||
| return 0.001 ether; | ||
| } | ||
|
|
||
| function setSocketFees(uint256) external override { | ||
| // Mock implementation - do nothing | ||
| } | ||
|
|
||
| function socketFees() external pure override returns (uint256) { | ||
| return 0.001 ether; | ||
| } |
There was a problem hiding this comment.
Implement fee setter to persist the configured value
setSocketFees is currently a no-op so socketFees() keeps returning 0.001 ether. Any test that tries to adjust the fee through this mock will silently keep the default, defeating coverage of dynamic fee updates. This should store the provided value and have both getters read it.
- bool public payAndCheckFeesCalled = false;
- ExecuteParams public lastExecuteParams;
- TransmissionParams public lastTransmissionParams;
+ bool public payAndCheckFeesCalled = false;
+ ExecuteParams public lastExecuteParams;
+ TransmissionParams public lastTransmissionParams;
+ uint256 private _socketFees = 0.001 ether;
@@
- function getMinSocketFees() external pure override returns (uint256) {
- return 0.001 ether;
+ function getMinSocketFees() external view override returns (uint256) {
+ return _socketFees;
}
@@
- function setSocketFees(uint256) external override {
- // Mock implementation - do nothing
- }
-
- function socketFees() external pure override returns (uint256) {
- return 0.001 ether;
+ function setSocketFees(uint256 socketFees_) external override {
+ _socketFees = socketFees_;
+ }
+
+ function socketFees() external view override returns (uint256) {
+ return _socketFees;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool public payAndCheckFeesCalled = false; | |
| ExecuteParams public lastExecuteParams; | |
| TransmissionParams public lastTransmissionParams; | |
| function payAndCheckFees( | |
| ExecuteParams memory executeParams_, | |
| TransmissionParams memory transmissionParams_ | |
| ) external payable override { | |
| payAndCheckFeesCalled = true; | |
| lastExecuteParams = executeParams_; | |
| lastTransmissionParams = transmissionParams_; | |
| } | |
| function getMinSocketFees() external pure override returns (uint256) { | |
| return 0.001 ether; | |
| } | |
| function setSocketFees(uint256) external override { | |
| // Mock implementation - do nothing | |
| } | |
| function socketFees() external pure override returns (uint256) { | |
| return 0.001 ether; | |
| } | |
| bool public payAndCheckFeesCalled = false; | |
| ExecuteParams public lastExecuteParams; | |
| TransmissionParams public lastTransmissionParams; | |
| uint256 private _socketFees = 0.001 ether; | |
| function payAndCheckFees( | |
| ExecuteParams memory executeParams_, | |
| TransmissionParams memory transmissionParams_ | |
| ) external payable override { | |
| payAndCheckFeesCalled = true; | |
| lastExecuteParams = executeParams_; | |
| lastTransmissionParams = transmissionParams_; | |
| } | |
| function getMinSocketFees() external view override returns (uint256) { | |
| return _socketFees; | |
| } | |
| function setSocketFees(uint256 socketFees_) external override { | |
| _socketFees = socketFees_; | |
| } | |
| function socketFees() external view override returns (uint256) { | |
| return _socketFees; | |
| } |
🤖 Prompt for AI Agents
In test/mock/MockFeesManager.sol around lines 12 to 35, setSocketFees is a no-op
so socketFees() and getMinSocketFees() always return the hardcoded 0.001 ether;
add a uint256 state variable (e.g., socketFeesValue) initialized to 0.001 ether,
modify setSocketFees(uint256) to store the provided value into that variable,
and change socketFees() and getMinSocketFees() to return the stored variable
instead of the hardcoded literal so tests can update and observe dynamic fee
changes.
| function reset() external { | ||
| payAndCheckFeesCalled = false; | ||
| } |
There was a problem hiding this comment.
Reset should clear stored parameters
reset() only flips the flag, leaving lastExecuteParams and lastTransmissionParams carrying stale values. Tests that rely on a clean slate after reset() will still see the previous call’s data. This should delete the stored structs alongside resetting the flag.
function reset() external {
payAndCheckFeesCalled = false;
+ delete lastExecuteParams;
+ delete lastTransmissionParams;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function reset() external { | |
| payAndCheckFeesCalled = false; | |
| } | |
| function reset() external { | |
| payAndCheckFeesCalled = false; | |
| delete lastExecuteParams; | |
| delete lastTransmissionParams; | |
| } |
🤖 Prompt for AI Agents
In test/mock/MockFeesManager.sol around lines 41 to 43, reset() currently only
clears payAndCheckFeesCalled but leaves lastExecuteParams and
lastTransmissionParams populated; update reset() to also clear those stored
structs (e.g., delete lastExecuteParams; delete lastTransmissionParams; or
assign zeroed/default values for all their fields) so the mock returns to a
clean state for subsequent tests while still resetting the flag.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/evmx/watcher/precompiles/WritePrecompile.sol (1)
130-140: Switchboard struct is being encoded but decoded as uint64 — this will revertLine 136 encodes the entire
configurations__().switchboards(...)struct, yet Line 166 decodes that slot as auint64. Becauseconfigurations__().switchboardsreturns the fullSwitchboardstruct (see the configurations interface), ABI decoding will fail for every payload: the encoded struct spans multiple 32-byte words, while we attempt to interpret it as a single 8-byte scalar. This should only encode the switchboard ID (e.g..switchboardId) so that the decoded tuple matches. Fix by extracting the ID beforeabi.encode.- precompileData = abi.encode( - appGateway_, - queueParams_.transaction, - queueParams_.overrideParams.writeFinality, - queueParams_.overrideParams.gasLimit, - queueParams_.overrideParams.value, - configurations__().switchboards( - queueParams_.transaction.chainSlug, - queueParams_.switchboardType - ) - ); + uint64 switchboardId = configurations__() + .switchboards(queueParams_.transaction.chainSlug, queueParams_.switchboardType) + .switchboardId; + + precompileData = abi.encode( + appGateway_, + queueParams_.transaction, + queueParams_.overrideParams.writeFinality, + queueParams_.overrideParams.gasLimit, + queueParams_.overrideParams.value, + switchboardId + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contracts/evmx/fees/Credit.sol(11 hunks)contracts/evmx/interfaces/IConfigurations.sol(1 hunks)contracts/evmx/plugs/FeesPlug.sol(3 hunks)contracts/evmx/watcher/precompiles/WritePrecompile.sol(5 hunks)contracts/protocol/base/PlugBase.sol(3 hunks)contracts/protocol/switchboard/SwitchboardBase.sol(2 hunks)
🔇 Additional comments (6)
contracts/evmx/fees/Credit.sol (6)
372-380: Fix undefinedsbTypereference
Line 379 assignsqueueParams.switchboardType = sbType;butsbTypeno longer exists in this contract (the storage slot was renamed todeprecatedSbType, and the new flow is ID-based). This fails to compile today and, even once it compiles, would push the wrong selector into the queue. This should wire the correct identifier (e.g., the renameddeprecatedSbTypeor the newswitchboardId) consistently with the rest of the trigger refactor.
132-139: Fix susdcToken assignment before forwarding
Line 133 uses the persistentsusdcTokenslot before you ever assign it, so the forwarder deployment and_setValidPlugentries are written under the zero token and the actual arg is lost. This should store the argument into state and consistently use that value when wiring forwarders and emitting the event.+ susdcToken = susdcToken_; - forwarderAddresses[susdcToken][chainSlug_] = asyncDeployer__().getOrDeployForwarderContract( - susdcToken_, - chainSlug_ - ); - _setValidPlug(true, chainSlug_, susdcToken); - emit SusdcTokenSet(chainSlug_, susdcToken_); + forwarderAddresses[susdcToken][chainSlug_] = + asyncDeployer__().getOrDeployForwarderContract(susdcToken, chainSlug_); + _setValidPlug(true, chainSlug_, susdcToken); + emit SusdcTokenSet(chainSlug_, susdcToken);
173-177: Prevent double-mint on native withdrawal failure
Line 174 mintscreditAmount_a second time when the native transfer fails, so the user walks away with twice the intended credit.creditAmount_ += nativeAmount_;is also a no-op becausenativeAmount_has already been zeroed. This should mint the failed native amount and add it to the credit tallies before you zero the native leg.- _mint(depositTo_, creditAmount_); - nativeAmount_ = 0; - creditAmount_ += nativeAmount_; + _mint(depositTo_, nativeAmount_); + creditAmount_ += nativeAmount_; + nativeAmount_ = 0;
229-231: Guard balanceOf against blocked overflow
Line 230 subtractsuserBlockedCreditsdirectly from the raw ERC20 balance. If the blocked amount ever exceeds the raw balance (e.g., via partial burns) this underflows and bricks all reads. This should clamp at zero before subtracting.- return super.balanceOf(account) - userBlockedCredits[account]; + uint256 raw = super.balanceOf(account); + uint256 blocked = userBlockedCredits[account]; + return raw > blocked ? raw - blocked : 0;
288-290: Restore ERC20 approve semantics
Line 289 rewrites every non-zero allowance totype(uint256).max, which breaks ERC20 expectations and can leak infinite approvals to integrators. This should pass through the caller-provided amount unchanged.- return super.approve(spender, amount > 0 ? type(uint256).max : 0); + return super.approve(spender, amount);
424-434: Implement permit or revert explicitly
Lines 424-433 exposepermitbut leave it as a no-op. Callers will believe a signature succeeded while no allowance changes, breaking downstream flows and violating EIP-2612. This should either implement full permit (nonces, domain separator, allowance update) or revert so integrators don't get silent failures.
| function disconnectSocket() external onlyOwner { | ||
| socket__.disconnect(); | ||
| } |
There was a problem hiding this comment.
Emit the disconnect event
Calling socket__.disconnect() directly skips PlugBase._disconnectSocket(), so ConnectorPlugDisconnected never fires. Off-chain watchers lose the disconnect signal. This should delegate to the base helper.
- socket__.disconnect();
+ _disconnectSocket();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function disconnectSocket() external onlyOwner { | |
| socket__.disconnect(); | |
| } | |
| function disconnectSocket() external onlyOwner { | |
| _disconnectSocket(); | |
| } |
🤖 Prompt for AI Agents
In contracts/evmx/plugs/FeesPlug.sol around lines 136 to 138, the function
disconnectSocket() calls socket__.disconnect() directly which bypasses
PlugBase._disconnectSocket() and prevents the ConnectorPlugDisconnected event
from being emitted; replace the direct socket disconnect call with a call to the
base helper (i.e., invoke _disconnectSocket() from the function body) so the
base logic runs and the disconnect event is emitted.
| function initSocket( | ||
| bytes32 appGatewayId_, | ||
| address socket_, | ||
| address switchboard_ | ||
| uint64 switchboardId_ | ||
| ) external virtual socketInitializer { | ||
| _connectSocket(appGatewayId_, socket_, switchboard_); | ||
| _connectSocket(appGatewayId_, socket_, switchboardId_); | ||
| } |
There was a problem hiding this comment.
Lock down socket initialization
initSocket is callable by anyone until isSocketInitialized flips. An attacker can front-run the intended initializer, point socket__ at a malicious contract, and then invoke any onlySocket-gated flows to drain assets (e.g., withdrawFees). This should enforce authorization before wiring the socket.
- ) external virtual socketInitializer {
- _connectSocket(appGatewayId_, socket_, switchboardId_);
- }
+ ) external virtual socketInitializer {
+ _authorizeSocketInit();
+ _connectSocket(appGatewayId_, socket_, switchboardId_);
+ }
+
+ function _authorizeSocketInit() internal view virtual;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function initSocket( | |
| bytes32 appGatewayId_, | |
| address socket_, | |
| address switchboard_ | |
| uint64 switchboardId_ | |
| ) external virtual socketInitializer { | |
| _connectSocket(appGatewayId_, socket_, switchboard_); | |
| _connectSocket(appGatewayId_, socket_, switchboardId_); | |
| } | |
| function initSocket( | |
| bytes32 appGatewayId_, | |
| address socket_, | |
| uint64 switchboardId_ | |
| ) external virtual socketInitializer { | |
| _authorizeSocketInit(); | |
| _connectSocket(appGatewayId_, socket_, switchboardId_); | |
| } | |
| function _authorizeSocketInit() internal view virtual; |
4 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
Chores