Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Some fields in transaction are not authenticated by signature #426

Closed
yihuang opened this issue Aug 10, 2021 · 7 comments · Fixed by #703
Closed

Some fields in transaction are not authenticated by signature #426

yihuang opened this issue Aug 10, 2021 · 7 comments · Fixed by #703
Assignees

Comments

@yihuang
Copy link
Contributor

yihuang commented Aug 10, 2021

System info: ethermint main

Steps to reproduce:

Some fields in transaction are not signed:

  • the fields in the wrapping cosmos tx
  • the redundant From field in the MsgEthereumTx

Attacker is free to change these fields and tx is still valid.

Expected behavior: Can't modify tx without resigning.

Actual behavior: Can modify tx without resigning.

Additional info:

We should verify those fields against a constant value.

@leejw51crypto
Copy link
Contributor

leejw51crypto commented Aug 20, 2021

it's overwritten
in func (esvd EthSigVerificationDecorator) AnteHandle

// set up the sender to the transaction field if not already
 msgEthTx.From = sender.Hex()

how about adding in rpc client side?

@yihuang
Copy link
Contributor Author

yihuang commented Aug 20, 2021

The issue is it don't verify the content before override it.

@yihuang yihuang assigned yihuang and unassigned yihuang Sep 10, 2021
@tomtau
Copy link
Contributor

tomtau commented Oct 6, 2021

@Muggle-Du is looking into this issue

@yihuang yihuang removed their assignment Oct 6, 2021
@JayT106
Copy link
Contributor

JayT106 commented Oct 15, 2021

@Muggle-Du is looking into this issue

is @Muggle-Du still looking into it?

@adu-web3
Copy link
Contributor

@Muggle-Du is looking into this issue

is @Muggle-Du still looking into it?

Yes, I'm still looking into this.
And I need some help about this issue.
Currently, the tx on wire is a private type, so we can’t simply inspect the internals and do validation.
So why don't we directly broadcast MsgEthereumTx instead of using a wrapper, since MsgEthereumTx had already implemented sdk.tx?
Or can you propose any other workarounds?

@tomtau
Copy link
Contributor

tomtau commented Oct 18, 2021

@fedekunze

@leejw51crypto
Copy link
Contributor

leejw51crypto commented Oct 21, 2021

adding "from" null as part of consensus.
and other tx fields (not included in eth) as null
so keeping eth verification compatibility.

currently, it's ignored in ante-handler level. so any data is accepted.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants