fix(crosschain): security hardening of the cross-chain module

[xavikh]

Security hardening of the WIP cross-chain module

This PR fixes the findings of a security review of src/common/crosschain/ at
e49e055, carried out as part of the Aragon / Makina guardian-integration
planning work. The module had no tests; this PR adds a Foundry suite covering
every critical and high finding as a regression test.

The motivating downstream use case is a mainnet OSx DAO passing a proposal that
must cancel a pending action on a remote chain before an on-chain timelock
expires. That shapes several decisions below: silent failures are
unacceptable, and a message stranded past its deadline is a real loss.

Finding 3 — the design fork: delegatecall → call

Decision: the controller now invokes adapters with a plain call.

Rationale:

1. delegatecall is not repairable here. Adapter storage reads land in the
   controller's slots: feeToken (adapter slot 1) read the controller's unset
   slot 1 and returned address(0), and _trustedRemotes (slot 0) collided
   with chainToAdapter. The suggested alternative — make all send-path state
   immutable — is impossible for finding 5: a bidirectional chain-id ⇄ CCIP
   selector map must be a mapping, and mappings cannot be immutable. The
   same applies to the updatable trusted-remote and fee-token setters in
   finding 10. Keeping delegatecall would mean redeploying every adapter to
   add a lane.
2. It defuses finding 7. Under delegatecall, UPDATE_CONFIG_PERMISSION
   was arbitrary code execution in the controller's context, and the
   controller holds EXECUTE_PERMISSION — i.e. root. Under call, a malicious
   adapter can no longer touch controller storage or spend beyond the fee it is
   handed. (It can still forge inbound receiveMessage payloads, so the
   permission remains highly privileged — now documented loudly in NatSpec.)
3. It makes finding 9 concrete. Under call, CCIP sees the adapter as
   the sender, so _trustedRemotes[chainId] holds the remote adapter
   address — which is exactly what the controller already stores as
   chainToAdapter[chainId].remoteAdapter. One address, one meaning, on both
   sides. BaseAdapter.assertTrustedRemotesMatchController(uint256[]) is a
   deployment-time check that the two sides agree.

Fee custody. Per the deployment model (one OSx DAO per chain, fees paid from
the cross-chain contract's own pre-funded balance rather than per-proposal from
the treasury), the CrossChainController custodies the fee funds, not the
adapters:

• It gained a receive() and a permissioned sweep(token, to, amount) (native
  and ERC20) so funds are never stranded and the fee token can be rotated.
• Per send it quotes getFee, checks its own balance, and hands the adapter
  exactly the quoted fee — native as msg.value, ERC20 by safeTransfer
  immediately before the call. The adapter never relies on a standing balance
  and returns any remainder to the controller in the same transaction.
• CCIP does not refund overpayment, hence quote-then-pay-exactly.
• quoteFee(destChainId, gasLimit, message) returns (feeToken, fee, available) so off-chain monitoring can answer "can we still afford a send?"
  and top up ahead of a deadline. This matters: with a 72h+ voting window, a
  fee quoted at proposal creation can be badly stale at execution. A dedicated
  INSUFFICIENT_FEE_BALANCE(feeToken, required, available) error exists
  precisely so ops can alert on that one condition.

Ops note: only the sending side needs a fee balance — destination gas is
bought by the source-side fee via extraArgs.gasLimit. In the target
deployment only the mainnet controller needs funding; spoke controllers are
receive-only and should not be over-funded.

Findings

#	Sev	Finding	Resolution
1	CRITICAL	receiveMessage() public, no access control → unauthenticated DAO takeover	onlyLocalAdapter, backed by a refcounted localAdapter → laneCount registry maintained by updateConfig. A refcount (not a bool) is required because one adapter serves many lanes: it stays authorised until its last lane is cleared, and a rotated-out adapter loses access immediately.
2	CRITICAL	BaseAdapter._forwardMessage() declared public → second takeover path	Now internal.
3	HIGH	Storage collision under delegatecall	Send path switched to call; see above.
4	HIGH	Silent no-op for unconfigured chains	_validatedAdapters() reverts ADAPTER_NOT_CONFIGURED when either adapter is unset and ADAPTER_HAS_NO_CODE when the local adapter is codeless. updateConfig additionally rejects half-configured lanes (INCOMPLETE_ADAPTER_CONFIG) and chain id 0.
5	HIGH	chain id vs CCIP selector (identity functions)	Real bidirectional chainId ⇄ uint64 selector maps, constructor-seeded and updatable via setChainSelectors. Both directions revert (UNKNOWN_CHAIN_ID / UNKNOWN_NATIVE_CHAIN_ID) instead of returning 0. Re-pointing a chain id clears the stale reverse entry.
6	MEDIUM	No router approval; no native-fee support	forceApprove(router, fee) before ccipSend, reset to 0 after. Native fees supported via feeToken == address(0) (the balanceOf check is now branch-correct); UNEXPECTED_NATIVE_VALUE if value is sent while an ERC20 fee token is configured. Fee is quoted with getFee and paid exactly.
7	MEDIUM	UPDATE_CONFIG_PERMISSION effectively root	Risk substantially reduced by the move to call (no arbitrary code in the controller's context). Residual risk — a malicious adapter can still forge inbound messages — is documented in explicit NatSpec on the permission constant: grant to the DAO only, never an EOA.
8	MEDIUM	No defensive receive; reverting action strands the message	Execution is wrapped in try this.executeActions(...) catch; the self-call also covers abi.decode, so a malformed payload is captured too. Failures are stored and emitted as MessageExecutionFailed(originChainId, messageId, callId, reason) and retried via retryFailedMessage(callId) under a new RETRY_MESSAGE_PERMISSION. Delivery therefore never reverts on the bridge, and recovery does not depend on CCIP's ~8h manual-execution window.
9	LOW	Trusted-remote semantics ambiguity	Resolved by finding 3: trusted remote == remote adapter == chainToAdapter[].remoteAdapter. Documented on IBaseAdapter, BaseAdapter and the AdapterByChain struct; assertTrustedRemotesMatchController() added as a deployment check.
10	LOW	Zero call id, empty event, constructor-only config	callId = keccak256(originChainId, messageId) (also the failed-message key). Real event payloads: MessageForwarded, MessageReceived, MessageExecutionFailed, MessageRetried, ConfigUpdated, TrustedRemoteSet, FeeTokenSet, ChainSelectorSet, Swept. Permissioned setters setTrustedRemotes, setFeeToken, setChainSelectors under a new UPDATE_ADAPTER_CONFIG_PERMISSION.

Interface changes

• IBaseAdapter.sendMessage is now payable and returns bytes32 (the bridge
  message id) instead of uint256; quoteFee added; the view functions are
  marked view.
• CrossChainController.receiveMessage(bytes32 messageId, bytes payload, uint256 originChainId) — gained the message id, for traceability and for
  call-id derivation.
• BaseAdapter is now DaoAuthorizable, adopting the DAO of its controller, so
  adapter administration reuses the OSx permission system rather than
  introducing a second ownership model.
• CCIPAdapter's constructor takes the chain-id ⇄ selector seed arrays.

For human review

• UPDATE_CONFIG_PERMISSION / UPDATE_ADAPTER_CONFIG_PERMISSION grants.
  Both must go to the DAO only. Consider an allowlist of vetted adapter
  implementations if adapter deployment is ever delegated.
• RETRY_MESSAGE_PERMISSION grantee. The payload was already authenticated
  by the bridge, so an ops multisig is defensible and much faster than a second
  governance round before a timelock deadline — but it is a policy call.
• Defensive receive vs. fail-loud. A reverting payload is now swallowed into
  storage with a loud event rather than reverting the bridge delivery. This is
  Chainlink's recommended pattern and the right trade-off for a deadline-bound
  use case, but it does mean monitoring must watch MessageExecutionFailed.
• Fee funding levels and top-up alerting are operational decisions; the
  contract only provides quoteFee and INSUFFICIENT_FEE_BALANCE.
• Chain selectors are configuration, not constants. The values must be
  verified against Chainlink's directory at deployment.
• No reentrancy guard was added on forwardMessage; the adapter is a
  DAO-configured trusted component and the controller performs no state
  mutation around the call. Worth a second opinion.

---

Attribution: review and fixes produced during the Aragon/Makina
guardian-integration planning work.

🤖 Generated with Claude Code

[xavikh]

Opened #696 as the requested A/B counterpart to this PR: same ten security fixes, delegatecall retained, storage collision resolved by moving all send-path config into the controller ("Option 1") so the adapter's send path reads zero storage.

The PR body includes an explicit "What Option 1 is WORSE at than #695" section — most importantly that finding 7's residual risk returns and is not mitigated there.

[xavikh]

Superseded by #696, which contains both commits from this branch plus the delegatecall send-path resolution. Consolidating into a single PR against feat/cross-chain.