Skip to content
This repository has been archived by the owner on May 28, 2019. It is now read-only.

Stellar: invalid signature for the changeTrustOp #421

Closed
tsusanka opened this issue Nov 23, 2018 · 26 comments
Closed

Stellar: invalid signature for the changeTrustOp #421

tsusanka opened this issue Nov 23, 2018 · 26 comments
Assignees
Milestone

Comments

@tsusanka
Copy link
Contributor

This transaction supposedly creates an invalid transaction.

    transaction: {
        "source": "GDNAQREX5VOHMYZ2QDHDY6A7XR2OYIE67EETL5MFCT6TCGHYA7YHACEW",
        "fee": 100,
        "sequence": "89703140756029442",
        "operations": [
        {
          "type": "changeTrust",
          "line": {
            "code": "XRP",
            "issuer": "GBVOL67TMUQBGL4TZYNMY3ZQ5WGQYFPFD5VJRWXR72VA33VFNL225PL5",
			"type": 1
          },
          "limit": "922337203685.4775"
        }
        ],
        "memo": {
        	"type": 0
        }
    }
@tsusanka
Copy link
Contributor Author

@istrau2 writes:

... does indeed allow the trezor to complete the signing of the operation successfully. However, the trezor returns an invalid signature. The stellar network rejects the transaction when submitted with the error that signature is invalid.

@tsusanka
Copy link
Contributor Author

@zulucrypto the signature is the same for Trezor One as for Trezor T. Would you be interested in looking into this?

@istrau2
Copy link

istrau2 commented Nov 27, 2018

@tsusanka Any updates on this? We are trying to include Trezor support in our next release...

@tsusanka
Copy link
Contributor Author

Few questions:

  • Are you actually using the "limit" with a decimal number? Could you try with an integer?
  • Any chance you could provide what the serialization should look like before signing? (i.e. the exact bytes that should be signed for this transaction)

Answers for those would speed up things, otherwise I'll have to dive into the stellar eco system, which will take a while.

@istrau2
Copy link

istrau2 commented Nov 29, 2018

@tsusanka
Copy link
Contributor Author

Ok, thank you. One more question, how do you proceed? Using connect or python's trezorlib?

@istrau2
Copy link

istrau2 commented Nov 29, 2018

@tsusanka using trezor-connect (js library).

@zulucrypto
Copy link
Contributor

@tsusanka - Would you still like help looking into this?

@tsusanka
Copy link
Contributor Author

tsusanka commented Dec 2, 2018

@zulucrypto if you have a bit of time, it'd be great! I haven't been able to figure out much more than that the digest to be signed in Trezor differs from the ones I'm getting from the offical JS SDK, see stellar/js-stellar-sdk#211 or at least that's what it seems like.

But at the same time, that applies for the Payment operation, which I have tested myself and it worked, so I'm a bit confused here.

@zulucrypto
Copy link
Contributor

@tsusanka - It looks like this is a bug somewhere in Connect. tl;dr is that if you run the transaction that istrau2 provided you're asked to confirm an amount of 92233.7203685 when it should be 922337203685.4775. Notes are below since I didn't figure that out until I did a lot of other stuff!

I checked out the master branch of python trezor, trezor-mcu, and trezor core and then found the account used by the default emulator setups:

$ ./trezorctl stellar-get-address
GAK5MSF74TJW6GLM7NLTL76YZJKM2S4CGP3UH4REJHPHZ4YBZW2GSBPW

I used that to recreate istrau2's transaction (on the testnet):

https://www.stellar.org/laboratory/#xdr-viewer?input=AAAAABXWSL%2Fk028ZbPtXNf%2FYylTNS4Iz90PyJEnefPMBzbRpAAAAZAE%2BsIgAAAAFAAAAAAAAAAAAAAABAAAAAAAAAAYAAAABWFJQAAAAAABq5fvzZSATL5POGsxvMO2NDBXlH2qY2vH%2Bqg3upWr1rn%2F%2F%2F%2F%2F%2F%2F%2FzYAAAAAAAAAAEBzbRpAAAAQN0a0joKQB4uDD1aS4hYOcR5Gz9xIksLnWGfi8GV5%2Fm%2BryoPXc67lyFDNkmvmIwJM6lgKJK2tLIiSjrVSBjvbgo%3D&type=TransactionEnvelope&network=test

Then I ran it through both emulators (Trezor T and One):

$ ./trezorctl stellar-sign-transaction --network-passphrase 'Test SDF Network ; September 2015' AAAAABXWSL/k028ZbPtXNf/YylTNS4Iz90PyJEnefPMBzbRpAAAAZAE+sIgAAAAFAAAAAAAAAAAAAAABAAAAAAAAAAYAAAABWFJQAAAAAABq5fvzZSATL5POGsxvMO2NDBXlH2qY2vH+qg3upWr1rn////////zYAAAAAAAAAAEBzbRpAAAAQN0a0joKQB4uDD1aS4hYOcR5Gz9xIksLnWGfi8GV5/m+ryoPXc67lyFDNkmvmIwJM6lgKJK2tLIiSjrVSBjvbgo=
Please confirm action on your Trezor device
3RrSOgpAHi4MPVpLiFg5xHkbP3EiSwudYZ+LwZXn+b6vKg9dzruXIUM2Sa+YjAkzqWAokra0siJKOtVIGO9uCg==

They both return the expected signature.

Using https://trezor.github.io/connect-explorer/#/stellar-signtx I set up the following JSON (default path and on the testnet):

{
    "source": "GAK5MSF74TJW6GLM7NLTL76YZJKM2S4CGP3UH4REJHPHZ4YBZW2GSBPW",
        "fee": 100,
        "sequence": "89703140756029445",
        "operations": [
        {
          "type": "changeTrust",
          "line": {
            "code": "XRP",
            "issuer": "GBVOL67TMUQBGL4TZYNMY3ZQ5WGQYFPFD5VJRWXR72VA33VFNL225PL5",
			"type": 1
          },
          "limit": "922337203685.4775"
        }
        ],
        "memo": {
        	"type": 0
        }
}

When I ran that through the emulator, I noticed that it's not parsing the limit correctly and you're asked to confirm the wrong value.

@tsusanka
Copy link
Contributor Author

tsusanka commented Dec 4, 2018

That's great I believe we're finally getting somewhere.

Also thanks to the issue on Stellar's SDK I've finally realized why the data before signing differed. The reason is that both trezor firmware and Connect work with stroops rather than lumens. This is a design choice which is used for every coin to avoid floats. That means that value 922337203685 should indeed get parsed on Trezor as 92233.7203685 lumens. This also implies that only integers make sense here, float is an invalid value, which Connect did not check unfortunately.

We have agreed with @szymonlesisz that Connect will check if the amount is float and if so raise an error (trezor/connect#278). Only integers are valid.


@istrau2 could you try the same transaction but with the limit specified in stroops? If I am not mistaken that should be 9223372036854775000. And please confirm the Trezor shows 922337203685.4775 XLM.

@istrau2
Copy link

istrau2 commented Dec 4, 2018

@tsusanka @zulucrypto Ok, I tried this tx:

{"source":"GDNAQREX5VOHMYZ2QDHDY6A7XR2OYIE67EETL5MFCT6TCGHYA7YHACEW","fee":100,"sequence":"89703140756029445","operations":[{"type":"changeTrust","line":{"code":"SLT","issuer":"GCKA6K5PCQ6PNF5RQBF7PQDJWRHO6UOGFMRLK3DYHDOI244V47XKQ4GP","type":1},"limit":"9223372036854775000"}],"memo":{"type":0}}

The emulator shows 922337203685.4774784 on the confirmation screen.

Once the transaction is signed, I still get an invalid signature (that is rejected by the stellar network).

@prusnak prusnak added this to the v2.0.11 milestone Dec 5, 2018
@istrau2
Copy link

istrau2 commented Dec 31, 2018

@prusnak @tsusanka Any updates on this?

@tsusanka
Copy link
Contributor Author

tsusanka commented Jan 1, 2019

I'll look into this next week at the latest

@istrau2
Copy link

istrau2 commented Jan 1, 2019

@tsusanka great, looking forward. We are waiting on this to release Trezor support to all Stellarport users.

@reqlez

This comment has been minimized.

@tsusanka

This comment has been minimized.

@tsusanka
Copy link
Contributor Author

@istrau2 do you by any chance have any public beta where I could try trezor with the ChangeTrustOp easily? Our trezor.io/stellar currently does not support this op, so it might speed things a bit.

@tsusanka
Copy link
Contributor Author

So I've tried this using the Stellar's Python SDK and trezor generates the same pre-signature hash as the SDK does for the ChangeTrustOp. So I doubt this is a firmware bug.

So the only other option is that this is a Connect issue, but the fields are defined correctly. @istrau2 could you please try if you're still having this issue? Please use the newest Connect and don't forget we use lumens instead of XLMs.

If you still experience the issue, please provide an example script where you use Connect with Trezor and the network rejects the transaction.

@istrau2
Copy link

istrau2 commented Jan 16, 2019

@tsusanka Hi, how can I get in touch with you (I tried gitter and telegram)?

@tsusanka
Copy link
Contributor Author

@istrau2 I'll be on gitter

@tsusanka
Copy link
Contributor Author

So the problem is Connect currently does not support numbers larger then 2^53 (trezor/connect#302). There was a good reason Satoshi chose 21 mil cap for Bitcoin, because it fits in the IEEE-754 floating point standard.

This is not the case for Stellar which works with numbers up to 2^63 and recommends using "big number" librariers, which we are currently not using in Connect.

Although we are aware of this issue as tracked in trezor/connect#302, it will take a while to fix this because it means a significant rework of our stack.


In the meantime I believe the most straightforward solution to this is not to set the limit to the max value but to 2^53 - 1, which is the largest safe integer. I understand this is not ideal, but 2^53 - 1 is still a "big" number and when we fix this, users will be able to upgrade this limit.

Does this sound reasonable? Could you try to send 2^53 - 1 in your Stellarport instead of the max value?

@tsusanka
Copy link
Contributor Author

Also note, I've fixed the memo issue I believe you've reported earlier. It now defaults to memo type none if not defined. ea775c2#diff-53ecaee988a138eb845ea2e0df5e94dfR83

@istrau2
Copy link

istrau2 commented Jan 18, 2019

@tsusanka Ok, thanks I will check it out later today.

@istrau2
Copy link

istrau2 commented Jan 22, 2019

@tsusanka I just tried the proposed solution. Unfortunately, it has not solved the issue. Can you please reopen?

@tsusanka tsusanka reopened this Jan 22, 2019
@istrau2
Copy link

istrau2 commented Jan 22, 2019

@tsusanka My apologies. Looking again at the code, I realized that I changed the number in the json representation being sent to the trezor to sign but I hadn't changed the number in the actual transaction being submitted to the network. Obviously then, this resulted in an incorrect signature.

I will try again today.

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

No branches or pull requests

6 participants