Skip to content

fix: validate local signer in initiateTx#56

Closed
mattsu6666 wants to merge 5 commits into
mainfrom
fix-initaite-tx
Closed

fix: validate local signer in initiateTx#56
mattsu6666 wants to merge 5 commits into
mainfrom
fix-initaite-tx

Conversation

@mattsu6666

@mattsu6666 mattsu6666 commented Jun 18, 2026

Copy link
Copy Markdown
Member

Overview

There was a bug in initiateTx that allowed authentication to be bypassed.

This PR fixes it by authenticating AUTH_MODE_LOCAL signers in initiateTx based on msg.sender, in the same way as signTx.

Unlike the Go implementation, cross-solidity now enforces a constraint for initiateTx: exactly one signer must be provided and authenticated.

ethereum-cross-demo will be updated in a separate PR after this PR is merged.


2026-06-19 Update:

  • Added support for AUTH_MODE_EXTENSION in initiateTx to ensure consistency with Cross in Go.
  • As a result, cross-solidity now supports both AUTH_MODE_LOCAL and AUTH_MODE_EXTENSION.

2026-06-20 Update:

  • Improved initiateTx to support both AUTH_MODE_LOCAL and AUTH_MODE_EXTENSION within a single transaction.

@github-actions

github-actions Bot commented Jun 18, 2026

Copy link
Copy Markdown

⛽ Gas Usage Changes

Comparison against main branch:

Click to view gas diff
test_initAuthState_DelegatesToAuthManager() (gas: +6 (0.008%)) 
test_sign_DelegatesToAuthManager() (gas: +6 (0.012%)) 
test_sign_IgnoresUnknownSigners() (gas: +27 (0.013%)) 
test_getAuthState_ReturnsCorrectRemainingSigners() (gas: +54 (0.015%)) 
test_sign_IgnoresDuplicateSignatures() (gas: +54 (0.019%)) 
test_initAuthState_HandlesDuplicateSigners() (gas: +54 (0.019%)) 
test_initAuthState_Succeeds() (gas: +54 (0.020%)) 
test_setStateFromRemainingList_HandlesDuplicatesAndReturnsRemains() (gas: +54 (0.021%)) 
test_getRemainingSigners_TracksSigningProgress() (gas: +81 (0.023%)) 
test_initiateTx_RevertWhen_TimeoutHeightExpired() (gas: +74 (0.126%)) 
test_initiateTx_RevertWhen_ChainIDMismatch() (gas: +85 (0.139%)) 
test_initiateTx_RevertWhen_TimeoutTimestampExpired() (gas: +206 (0.347%)) 
test_getRequiredAccounts_ReturnsEmptyArrayWhenNoSigners() (gas: +328 (0.470%)) 
test_getRequiredAccounts_ReturnsEmptyArrayWhenNoTxs() (gas: +204 (0.498%)) 
test_constructor_SetsChainIDHash() (gas: +43 (0.517%)) 
test_getRequiredAccounts_AggregatesSigners() (gas: +3148 (1.180%)) 
test_initiateTx_RevertWhen_txIDAlreadyExists() (gas: +2753 (1.770%)) 
test_selfXCC_ReturnsCorrectChannelInfo() (gas: +308 (1.806%)) 
test_constructor_GrantsIbcRoleWhenDebugModeTrue() (gas: +86619 (2.655%)) 
test_constructor_DoesNotGrantIbcRoleWhenDebugModeFalse() (gas: +86619 (2.673%)) 
test_extSignTx_SucceedsAsCompletedAndEmitsEvent() (gas: +2267 (2.693%)) 
test_initialize_SucceedsAndEmitsEvents() (gas: -55941 (-3.573%)) 
test_initialize_RevertWhen_ArrayLengthMismatch() (gas: -55897 (-3.678%)) 
test_initialize_RevertWhen_AlreadyInitialized() (gas: -55891 (-3.767%)) 
test_verifySignatures_DelegatesToAuthManager() (gas: +1860 (3.789%)) 
test_initialize_RevertWhen_EmptyTypeUrl() (gas: -55902 (-3.835%)) 
test_initialize_RevertWhen_ZeroAddressVerifier() (gas: -55903 (-4.006%)) 
test_initialize_SucceedsWithEmptyArrays() (gas: -55865 (-4.013%)) 
test_extSignTx_RevertsIf_VerificationFails() (gas: +2268 (4.628%)) 
test_extSignTx_SucceedsAsPending() (gas: +2267 (5.117%)) 
test_verifySignatures_RevertWhen_StaticCallFails() (gas: +1806 (5.746%)) 
test_verifySignatures_RevertWhen_VerifierReturnsFalse() (gas: +1914 (5.905%)) 
test_verifySignatures_SucceedsSingleSigner() (gas: +1792 (6.357%)) 
test_verifySignatures_SucceedsMultipleSigners() (gas: +3243 (7.624%)) 
test_verifySignatures_RevertWhen_VerifierNotFound() (gas: +2195 (8.005%)) 
test_verifySignatures_RevertWhen_AuthModeMismatch() (gas: +2264 (9.573%)) 
test_verifySignatures_RevertWhen_TooManySigners() (gas: +65609 (44.755%)) 
test_initiateTx_SucceedsAsVerifiedWhenSignersMet() (gas: +203879 (65.921%)) 
test_initiateTx_SucceedsAsPendingWhenSignersNotMet() (gas: +196974 (72.340%)) 
Overall gas change: 333716 (0.031%)

Calculated by Foundry Gas Snapshot Action

@github-actions

github-actions Bot commented Jun 18, 2026

Copy link
Copy Markdown

LCOV of commit fd99c04 during Coverage Report #285

Summary coverage rate:
  lines......: 96.6% (648 of 671 lines)
  functions..: 95.6% (109 of 114 functions)
  branches...: no data found

Files changed coverage rate:
                                         |Lines       |Functions  |Branches    
  Filename                               |Rate     Num|Rate    Num|Rate     Num
  =============================================================================
  src/core/DelegatedLogicHandler.sol     |32.4%     68| 0.0%    22|    -      0
  src/core/Initiator.sol                 | 9.7%     72| 0.0%     7|    -      0
  src/core/TxAuthManager.sol             |17.1%     82| 0.0%    14|    -      0

Full coverage report

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR closes an authentication bypass in initiateTx by enforcing that AUTH_MODE_LOCAL initiation is tied to msg.sender, aligning initiateTx authentication semantics with signTx and tightening the initiation signer constraints.

Changes:

  • Enforce initiateTx to require exactly one signer, with auth_type.mode == AUTH_MODE_LOCAL.
  • Validate that the provided signer ID matches abi.encodePacked(msg.sender) during initiateTx.
  • Update Solidity and Go tests (and gas snapshot) to reflect the new initiation signer requirements.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/Initiator.t.sol Updates Initiator tests to use a local signer derived from address(this) and adds revert tests for invalid signer configurations.
src/core/Initiator.sol Adds _validateLocalSigner and calls it from initiateTx to prevent local-auth initiation bypass.
pkg/testing/cross_test.go Adjusts Go-side initiateTx test input to use opts.From.Bytes() as the local signer ID.
package-lock.json Lockfile metadata adjustment (removes peer: true flags in a couple entries).
.gas-snapshot Updates gas baselines reflecting added validation and updated test inputs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Comment thread src/core/Initiator.sol Outdated
Comment thread test/Initiator.t.sol Outdated
Comment thread test/Initiator.t.sol

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Comment thread src/core/Initiator.sol Outdated
@mattsu6666 mattsu6666 marked this pull request as draft June 19, 2026 07:06
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown

Slither report

Summary

uninitialized-local

Impact: Medium
Confidence: Medium

Account.Data memory localSigner;

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Comment thread src/core/Initiator.sol
Comment thread src/core/TxAuthManagerBase.sol
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants