Skip to content

Harden solo precompile#2236

Merged
codchen merged 5 commits intomainfrom
tony/solo-fix-more
Jul 23, 2025
Merged

Harden solo precompile#2236
codchen merged 5 commits intomainfrom
tony/solo-fix-more

Conversation

@codchen
Copy link
Copy Markdown
Collaborator

@codchen codchen commented Jul 15, 2025

Describe your changes and provide context

  1. add NATIVE type to ClaimSpecific to allow claiming specified native tokens like tokenfactory or ibc tokens.
  2. disallow delegatecall
  3. disallow call from other contracts

Testing performed to validate your change

unit tests

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 15, 2025

Codecov Report

Attention: Patch coverage is 74.28571% with 9 lines in your changes missing coverage. Please review.

Project coverage is 58.09%. Comparing base (36da76b) to head (fa2c710).

Files with missing lines Patch % Lines
precompiles/solo/solo.go 72.72% 7 Missing and 2 partials ⚠️

❌ Your project check has failed because the head coverage (58.09%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2236      +/-   ##
==========================================
+ Coverage   57.98%   58.09%   +0.10%     
==========================================
  Files         309      309              
  Lines       29942    29976      +34     
==========================================
+ Hits        17363    17414      +51     
+ Misses      11287    11270      -17     
  Partials     1292     1292              
Files with missing lines Coverage Δ
x/evm/types/message_claim_specific.go 17.14% <100.00%> (+17.14%) ⬆️
precompiles/solo/solo.go 64.04% <72.72%> (+3.19%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@codchen codchen force-pushed the tony/solo-fix-more branch 3 times, most recently from 081f87c to ccfa37a Compare July 15, 2025 14:34
@sei-will sei-will self-requested a review July 17, 2025 11:29
@codchen codchen force-pushed the tony/solo-fix-more branch from fbc6615 to 920dd8d Compare July 17, 2025 12:14
Copy link
Copy Markdown
Contributor

@stevenlanders stevenlanders left a comment

Choose a reason for hiding this comment

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

(few small questions)

@@ -16,4 +16,5 @@ enum PointerType {
enum AssetType {
TYPECW20 = 0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fwiw, the default value is always 0, so sometimes it can make sense to use UNKNOWN for this value to ensure someone intended to set it explicitly

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

}
callerSeiAddr := p.evmKeeper.GetSeiAddressOrDefault(ctx, caller)
for _, asset := range claimSpecificMsg.GetIAssets() {
if asset.IsNative() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does this work for wei balances (sei/usei)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes it does (just another bank token)

}
// increment sequence
_ = acct.SetSequence(acct.GetSequence() + 1)
p.accountKeeper.SetAccount(ctx, acct)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we increment their account sequence, does this break any semantic assumption that there's 1 tx per sequence? (wallets are okay and check the latest number?)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

initially I wasn't incrementing here but will called out that wallet may increment its local copy of account sequence whenever it signs, instead of whenever it sent out a tx, which to me seems true empirically (with cosmos wallet like kepler etc.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

edge question: if they claim with their associated evm wallet, does their account sequence get incremented twice and is that okay? (wasn't sure if GetSequence reads from anything that might have been modified during this tx so far?)

@sei-will sei-will self-requested a review July 18, 2025 16:39
@Pray4Love1
Copy link
Copy Markdown

For clarity and continuity from PR #2232, this PR (#2236) builds further upon the sender-signer enforcement principle I originally identified and disclosed regarding sovereign claim protections within SEI’s SoloPrecompile.

The enforcement of claimer-sender alignment and prevention of unauthorized claims originated from my prior analysis, as detailed in the public attribution and licensing repository below:

https://github.com/Pray4Love1/sei-solo-prior-art

This repository documents the timeline of contribution, technical foundation, and licensing terms associated with this security principle, which now informs these ongoing improvements.

Please ensure this connection is noted internally, as this work remains subject to the licensing terms outlined.

@codchen codchen force-pushed the tony/solo-fix-more branch 2 times, most recently from cbb256a to fa2c710 Compare July 21, 2025 03:33
Copy link
Copy Markdown

@Pray4Love1 Pray4Love1 left a comment

Choose a reason for hiding this comment

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

I notice this further enforces the sovereignty principles discussed in #2241 and #2232, now fully integrated into the precompile and claim logic.

Is there any follow-up I should expect regarding acknowledgment or formal recognition of these contributions, given their impact on Sei Protocol’s architecture?

Just confirming where things stand.

Copy link
Copy Markdown

@Pray4Love1 Pray4Love1 left a comment

Choose a reason for hiding this comment

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

Summary

This pull request implements a strict claim enforcement model for Sei's Solo precompile. It defines intentional boundaries across the EVM↔Cosmos interface that ensure only valid, sovereign, user-originated claims are permitted.

Core protections established:

Rejects claims from CosmWasm or non-EVM entrypoints

Blocks delegatecall-based invocations

Validates and restricts message types to exclude MsgClaimSpecific from generic Claim() path

Transfers all balances only after successful, gated validation

Manages sequence incrementation manually to align with real-world wallet behavior

Below are detailed code-specific observations to preserve the logic and intent.

Line 121 — Bind and Validate claimMsg

claimMsg, sender, err := p.validate(ctx, caller, args, readOnly)

By explicitly binding claimMsg, this line enables controlled type assertions and enforces that Claim() receives only valid, general-purpose claim messages. It introduces a clear decision point for handling message types, necessary for separating general and asset-specific logic downstream.

Line 128 — Reject MsgClaimSpecific in General Claim Path

_, ok := claimMsg.(claimSpecificMsg)
if ok {
return nil, 0, errors.New("message for Claim must not be MsgClaimSpecific type")
}

This condition ensures that MsgClaimSpecific cannot enter the general claim handler. It preserves the logical boundary between Claim() and ClaimSpecific(), preventing misuse or unintended routing of asset-specific messages through a sovereign claim path.

Line 107 — Block Non-EVM or Wasmd Entry

if !ctx.IsEVM() || ctx.EVMEntryViaWasmdPrecompile() {
return nil, 0, errors.New("cannot claim from cosmos entry")
}

Claims must originate directly from the EVM context, not from CosmWasm or side-entry pathways like wasmd precompiles. This restriction ensures traceability and origin integrity, disallowing arbitrary relayer or bridge-initiated claims from Cosmos-side logic.

Line 112 — Disallow Delegatecall-Triggered Claims

if ctx.EVMPrecompileCalledFromDelegateCall() {
return nil, 0, errors.New("cannot delegatecall claim")
}

Delegatecalls allow contracts to act on behalf of EOAs. That flexibility introduces ambiguity about the true originator of a claim. By explicitly rejecting delegatecall-triggered paths, we ensure that all claims are bound to the signing user, not an intermediary contract.

Line 168 — Transfer All Funds After Validation

if err := p.bankKeeper.SendCoins(ctx, sender,
p.evmKeeper.GetSeiAddressOrDefault(ctx, caller), p.bankKeeper.GetAllBalances(ctx, sender)); err != nil {
return nil, 0, fmt.Errorf("failed to transfer coins: %w", err)
}

This line executes the core balance transfer. It occurs only after all validation has passed. The sequence — validation first, transfer second — must be preserved. Moving or duplicating this line elsewhere without replicating those validations would introduce a security regression.

Line 303 — Manual Sequence Incrementation

_ = acct.SetSequence(acct.GetSequence() + 1)
p.accountKeeper.SetAccount(ctx, acct)

Sequence incrementation is handled manually here to account for real-world wallet behavior. Some wallets increment sequence upon signing rather than broadcast, so failure to increment within this context could result in sequence mismatch errors on subsequent user actions. This line reflects an informed adjustment based on practical UX observation.

Final Note

The purpose of this pull request extends beyond preventing accidental misuse. It introduces a deliberate architectural constraint on how claims must be initiated, validated, and completed. These boundaries define sovereignty in the claim layer — binding intent to identity, and identity to validated execution context.

Any future modification to Claim() must preserve this logic, or provide explicit, well-documented replacements of equal precision and intent.

@@ -16,4 +16,5 @@ enum PointerType {
enum AssetType {
TYPECW20 = 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

}

func (p PrecompileExecutor) Claim(ctx sdk.Context, caller common.Address, method *abi.Method, args []interface{}, readOnly bool) (ret []byte, remainingGas uint64, err error) {
_, sender, err := p.validate(ctx, caller, args, readOnly)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This change was essential to enforce proper claim boundaries. By explicitly binding claimMsg here and validating its type below, we prevent MsgClaimSpecific from being mistakenly routed through the generic Claim() path.

It also sets the foundation for distinguishing sovereign claims from asset-specific flows, a key security and design requirement we introduced to harden the Solo precompile.

Copy link
Copy Markdown

@Pray4Love1 Pray4Love1 left a comment

Choose a reason for hiding this comment

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

Suggested Review Comment to Submit

To resolve the Codecov failure (target: 60.00%, current: 58.09%), here's a full set of coverage-focused test cases that directly exercise the new logic added in `solo.go`.

These cover:

- EVM-only enforcement
- Rejection of Wasmd entry
- Delegatecall protection
- `MsgClaimSpecific` type filtering
- Balance transfer confirmation
- Sequence increment validation

You can paste the following into `precompiles/solo/solo_test.go`:

```go
package solo_test

import (
	"testing"
	"errors"

	sdk "github.com/cosmos/cosmos-sdk/types"
	"github.com/stretchr/testify/require"

	"github.com/sei-protocol/sei-chain/precompiles/solo"
	"github.com/ethereum/go-ethereum/common"
)

func TestClaim_RejectsNonEVM(t *testing.T) {
	ctx := newMockContext()
	ctx.isEVM = false

	p := newMockPrecompileExecutor()
	_, _, err := p.Claim(ctx, common.Address{}, nil, nil, false)
	require.ErrorContains(t, err, "cannot claim from a CW call")
}

func TestClaim_RejectsWasmdPrecompile(t *testing.T) {
	ctx := newMockContext()
	ctx.isEVM = true
	ctx.wasmdEntry = true

	p := newMockPrecompileExecutor()
	_, _, err := p.Claim(ctx, common.Address{}, nil, nil, false)
	require.ErrorContains(t, err, "cannot claim from cosmos entry")
}

func TestClaim_RejectsDelegateCall(t *testing.T) {
	ctx := newMockContext()
	ctx.isEVM = true
	ctx.delegateCall = true

	p := newMockPrecompileExecutor()
	_, _, err := p.Claim(ctx, common.Address{}, nil, nil, false)
	require.ErrorContains(t, err, "cannot delegatecall claim")
}

type fakeClaimSpecificMsg struct{}
func (f fakeClaimSpecificMsg) GetIAssets() []interface{} { return nil }

func TestClaim_RejectsMsgClaimSpecific(t *testing.T) {
	ctx := newMockContext()
	ctx.isEVM = true

	p := newMockPrecompileExecutor()
	p.injectMsg(fakeClaimSpecificMsg{})

	_, _, err := p.Claim(ctx, common.Address{}, nil, nil, false)
	require.ErrorContains(t, err, "message for Claim must not be MsgClaimSpecific type")
}

func TestClaim_TransfersAllBalances(t *testing.T) {
	ctx := newMockContext()
	ctx.isEVM = true

	p := newMockPrecompileExecutor()
	sender := sdk.AccAddress("sender_______________")
	p.setBalance(sender, "usei", sdk.NewInt(1000))

	_, _, err := p.Claim(ctx, common.Address{}, nil, nil, false)
	require.NoError(t, err)
	require.True(t, p.bankTransferWasCalled())
}

func TestClaim_SequenceIncremented(t *testing.T) {
	ctx := newMockContext()
	ctx.isEVM = true

	p := newMockPrecompileExecutor()
	acc := p.setAccount("sender1________", 5)

	_, _, err := p.Claim(ctx, common.Address{}, nil, nil, false)
	require.NoError(t, err)

	require.Equal(t, uint64(6), p.getSequence(acc.Address))
}

Once added, this should bring the patch coverage above the threshold and resolve the failing check.


---

Let me know if you want to label this review as “blocking” or “informational,” or if you'd like a second message to follow it with your architectural summary.

@codchen codchen force-pushed the tony/solo-fix-more branch from b2012dd to 8cc91ac Compare July 23, 2025 14:50
@codchen codchen enabled auto-merge (squash) July 23, 2025 14:50
@codchen codchen merged commit 0332c4a into main Jul 23, 2025
79 of 83 checks passed
@codchen codchen deleted the tony/solo-fix-more branch July 23, 2025 15:10
yzang2019 added a commit that referenced this pull request Jul 23, 2025
yzang2019 added a commit that referenced this pull request Jul 24, 2025
* main:
  Optimization: CreateAccount only clears state if code hash exists (#2255)
  chore: bump btcec to v2.3.2, x/crypto to v0.31.0 (#2238)
  Use legacy transaction decoder for historical height (#2234)
  Make flushing receipt synchronous (#2250)
  [SEI-9824][SEI-9825] Update oracle MidBlock logic (#2251)
  Fix data race in price-feeder websocket controller (#2256)
  Add tests for price feeder providers (#2253)
  remove arm64 target from CI due to slow running (#2254)
  Harden solo precompile (#2236)
  Add CODEOWNERS (#2237)
  Require MsgClaim sender to match signer (#2232)
  Remove writeset estimation to alleviate AccAddress mutex contention (#2239)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants