mhw0 / libethc

Open-source Ethereum C library
https://mhw0.github.io/libethc/
MIT License
46 stars 8 forks source link

example: legacy transaction example encodes `r` and `s` in a wrong way #7

Closed mhw0 closed 8 months ago

mhw0 commented 11 months ago

These: https://github.com/mhw0/libethc/blob/60a518fa7a9f0830112f483ae3159b528b71e42c/examples/legacy-transaction.c#L62-L63 must be replaced with ok(eth_rlp_uint8(&rlp1, r)) and ok(eth_rlp_uint8(&rlp1, s))

DerXanRam commented 8 months ago

Hello bro. actually encoding r and s using bytes data type is a correct way. This change was made after i rise this https://github.com/mhw0/libethc/issues/6#issuecomment-1682669194. At that time i didn't understand why the problem happens and continue using uint8 to encode both r and s. then using uint8 for r and s brings another problem, which is encoding from address incorrectly(https://github.com/mhw0/libethc/issues/31#issue-1909374976). If u remember, the fix was changing ok(eth_keccak256(keccak, rlp0bytes, rlp0len)); to ok(eth_keccak256p(keccak, rlp0bytes, rlp0len));

this fix works temporarily until i notice something. In random conditions, signing using the eth_keccak256p results in undefined behavior and incorrect encoding of from address. I took lot of tests and i can't catch the reason. So i returned to the old example and start testing. Finally got stable and correct result.

the test i took is approve transaction and the abi data is

0x095ea7b30000000000000000000000005302086a3a25d473aabbd0356eff8dd811a4d89b0000000000000000000000000000000000000000000000000de0b6b3a7640000

the code i used to sign is

string rawTransaction(string inputData, string gasLimit,string gasPrice, string to,string val="0x00", string from="0xb438D71c1E6E8E53637e1F1deC37c2E635e335fc")
{
    struct eth_rlp rlp0, rlp1;
    struct eth_ecdsa_signature sign;

    uint8_t privkey[] = {0x80, 0x3e, 0x53, 0x83, 0x51, 0x93, 0xdd, 0x6e,
                  0xad, 0x61, 0xa2, 0x5a, 0x74, 0x5f, 0xa6, 0x35,
                  0x86, 0xb4, 0xb2, 0xdd, 0x7b, 0x6a, 0xbe, 0x4a,
                  0x06, 0xfa, 0x7f, 0x33, 0xc1, 0x1a, 0xed, 0x81}; // real privare key of 0xa9d0a7dC416f586491f2fb596731598F937617b5
  uint8_t nonce=0x3D, zero=0x00, keccak[32], *rlp0bytes, *r, *s;
  char *gasprice = strdup(gasPrice.c_str()), *gaslimit = strdup(gasLimit.c_str()), *value = strdup(val.c_str()), *input=strdup(inputData.c_str());
  char *toaddr = strdup(to.c_str()), *txn;
  uint8_t chainid = 0x38, v;
  size_t rlp0len, rlp1len, siglen=32;

  ok(eth_rlp_init(&rlp0, ETH_RLP_ENCODE));

  ok(eth_rlp_array(&rlp0));                  //[
    ok(eth_rlp_uint8(&rlp0, &nonce));        //  
    ok(eth_rlp_hex(&rlp0, &gasprice, NULL)); //  
    ok(eth_rlp_hex(&rlp0, &gaslimit, NULL)); //   
    ok(eth_rlp_address(&rlp0, &toaddr));     //   
    ok(eth_rlp_hex(&rlp0, &value, NULL));    //   value
    ok(eth_rlp_hex(&rlp0, &input,NULL));     //   abi data
    ok(eth_rlp_uint8(&rlp0, &chainid));     //   0x7a69,
    ok(eth_rlp_uint8(&rlp0, &zero));         //   0x,
    ok(eth_rlp_uint8(&rlp0, &zero));         //   0x,
  ok(eth_rlp_array_end(&rlp0));              // ]

  ok(eth_rlp_to_bytes(&rlp0bytes, &rlp0len, &rlp0));
  ok(eth_rlp_free(&rlp0));

  // compute the keccak hash of the encoded rlp elements
  ok(eth_keccak256(keccak, rlp0bytes, rlp0len));
  free(rlp0bytes);

  // sign the transaction
  ok(eth_ecdsa_sign(&sign, privkey, keccak));

  // calculate v
  v = sign.recid + chainid * 2 + 35;
  r = sign.r;
  s = sign.s;

  ok(eth_rlp_init(&rlp1, ETH_RLP_ENCODE));

  ok(eth_rlp_array(&rlp1));                 //[
    ok(eth_rlp_uint8(&rlp1, &nonce));
    ok(eth_rlp_hex(&rlp1, &gasprice, NULL));
    ok(eth_rlp_hex(&rlp1, &gaslimit, NULL));
    ok(eth_rlp_address(&rlp1, &toaddr));
    ok(eth_rlp_hex(&rlp1, &value, NULL));     //value
    ok(eth_rlp_hex(&rlp1, &input,NULL));      //abi data
    ok(eth_rlp_uint8(&rlp1, &v));
    ok(eth_rlp_bytes(&rlp1, &r, &siglen));
    ok(eth_rlp_bytes(&rlp1, &s, &siglen));
  ok(eth_rlp_array_end(&rlp1));             //]

  ok(eth_rlp_to_hex(&txn, &rlp1));
  ok(eth_rlp_free(&rlp1));
  string raw(txn);
  free(txn);
  return raw;
}

when i send to json rpc server, final result is transaction hash and it worked successfully.

The above code using your old example works perfectly but only if i use uint8 for chainid and v. ➡️ If i use uint16 for chainid and uint8 for v, it results incorrect from address.

➡️ and if i use uint8 for chainid and uint16 for v it result

{"error":{"code":-32000,"message":"read V: rlp: non-canonical integer format"},"id":1,"jsonrpc":"2.0"}

➡️ if i encode both chainid and v in uint16, it results incorrect from address

➡️ if i encode r and s using uint8, it results incorrect from address.

So encoding r and s using bytes data type is correct and the problem is with data types of chainid and v. And i'm very sorry to create the misconception on u and this repo 🙏 . Also i updated the example code in the discussion section.

Thanks a lot brother

mhw0 commented 8 months ago

Hello @DerXanRam Have you tried encoding v, r and s using eth_abi_bytes? And, did it fix the issue?

DerXanRam commented 8 months ago

Hello @DerXanRam Have you tried encoding v, r and s using eth_abi_bytes? And, did it fixed the issue?

Yes bro.

mhw0 commented 8 months ago

Hello @DerXanRam Have you tried encoding v, r and s using eth_abi_bytes? And, did it fixed the issue?

Yes bro.

Okay. Can you please tell me the value of v, r and s? I think your v doesn't fit in uint8

DerXanRam commented 8 months ago

Ok wait pls

DerXanRam commented 8 months ago

I use the above ABI data and code and gasPrice=0xEE6B2800 gasLimit=0x186A0 nonce=0x3D toaddr=0x55d398326f99059ff775485246999027b3197955 value=0x00 chainid=0x38

the raw tnx generated is 0xf8aa3d84ee6b2800830186a09455d398326f99059ff775485246999027b319795580b844095ea7b30000000000000000000000005302086a3a25d473aabbd0356eff8dd811a4d89b0000000000000000000000000000000000000000000000000de0b6b3a76400008194a05d7bf43a7b0e2f72cfb045a88ea92063fe6e60f2ac72cd4880af751a7114079ca0508891d1c779abd72700ba34588082507b027a1f205845c8b7051cf975c4de6c

R 0x5d7bf43a7b0e2f72cfb045a88ea92063fe6e60f2ac72cd4880af751a7114079c

S 0x508891d1c779abd72700ba34588082507b027a1f205845c8b7051cf975c4de6c V 148

and the tnx works perfectly

mhw0 commented 8 months ago

Nice, but, what's the problem?)

DerXanRam commented 8 months ago

what i

Nice, but, what's the problem?)

what if chainid and v exceeds size of uint8?

Hello bro. actually encoding r and s using bytes data type is a correct way. This change was made after i rise this #6 (comment). At that time i didn't understand why the problem happens and continue using uint8 to encode both r and s. then using uint8 for r and s brings another problem, which is encoding from address incorrectly(#31 (comment)). If u remember, the fix was changing ok(eth_keccak256(keccak, rlp0bytes, rlp0len)); to ok(eth_keccak256p(keccak, rlp0bytes, rlp0len));

this fix works temporarily until i notice something. In random conditions, signing using the eth_keccak256p results in undefined behavior and incorrect encoding of from address. I took lot of tests and i can't catch the reason. So i returned to the old example and start testing. Finally got stable and correct result.

the test i took is approve transaction and the abi data is

0x095ea7b30000000000000000000000005302086a3a25d473aabbd0356eff8dd811a4d89b0000000000000000000000000000000000000000000000000de0b6b3a7640000

the code i used to sign is

string rawTransaction(string inputData, string gasLimit,string gasPrice, string to,string val="0x00", string from="0xb438D71c1E6E8E53637e1F1deC37c2E635e335fc")
{
  struct eth_rlp rlp0, rlp1;
  struct eth_ecdsa_signature sign;

  uint8_t privkey[] = {0x80, 0x3e, 0x53, 0x83, 0x51, 0x93, 0xdd, 0x6e,
                0xad, 0x61, 0xa2, 0x5a, 0x74, 0x5f, 0xa6, 0x35,
                0x86, 0xb4, 0xb2, 0xdd, 0x7b, 0x6a, 0xbe, 0x4a,
                0x06, 0xfa, 0x7f, 0x33, 0xc1, 0x1a, 0xed, 0x81}; // real privare key of 0xa9d0a7dC416f586491f2fb596731598F937617b5
  uint8_t nonce=0x3D, zero=0x00, keccak[32], *rlp0bytes, *r, *s;
  char *gasprice = strdup(gasPrice.c_str()), *gaslimit = strdup(gasLimit.c_str()), *value = strdup(val.c_str()), *input=strdup(inputData.c_str());
  char *toaddr = strdup(to.c_str()), *txn;
  uint8_t chainid = 0x38, v;
  size_t rlp0len, rlp1len, siglen=32;

  ok(eth_rlp_init(&rlp0, ETH_RLP_ENCODE));

  ok(eth_rlp_array(&rlp0));                  //[
    ok(eth_rlp_uint8(&rlp0, &nonce));        //  
    ok(eth_rlp_hex(&rlp0, &gasprice, NULL)); //  
    ok(eth_rlp_hex(&rlp0, &gaslimit, NULL)); //   
    ok(eth_rlp_address(&rlp0, &toaddr));     //   
    ok(eth_rlp_hex(&rlp0, &value, NULL));    //   value
    ok(eth_rlp_hex(&rlp0, &input,NULL));     //   abi data
    ok(eth_rlp_uint8(&rlp0, &chainid));     //   0x7a69,
    ok(eth_rlp_uint8(&rlp0, &zero));         //   0x,
    ok(eth_rlp_uint8(&rlp0, &zero));         //   0x,
  ok(eth_rlp_array_end(&rlp0));              // ]

  ok(eth_rlp_to_bytes(&rlp0bytes, &rlp0len, &rlp0));
  ok(eth_rlp_free(&rlp0));

  // compute the keccak hash of the encoded rlp elements
  ok(eth_keccak256(keccak, rlp0bytes, rlp0len));
  free(rlp0bytes);

  // sign the transaction
  ok(eth_ecdsa_sign(&sign, privkey, keccak));

  // calculate v
  v = sign.recid + chainid * 2 + 35;
  r = sign.r;
  s = sign.s;

  ok(eth_rlp_init(&rlp1, ETH_RLP_ENCODE));

  ok(eth_rlp_array(&rlp1));                 //[
    ok(eth_rlp_uint8(&rlp1, &nonce));
    ok(eth_rlp_hex(&rlp1, &gasprice, NULL));
    ok(eth_rlp_hex(&rlp1, &gaslimit, NULL));
    ok(eth_rlp_address(&rlp1, &toaddr));
    ok(eth_rlp_hex(&rlp1, &value, NULL));     //value
    ok(eth_rlp_hex(&rlp1, &input,NULL));      //abi data
    ok(eth_rlp_uint8(&rlp1, &v));
    ok(eth_rlp_bytes(&rlp1, &r, &siglen));
    ok(eth_rlp_bytes(&rlp1, &s, &siglen));
  ok(eth_rlp_array_end(&rlp1));             //]

  ok(eth_rlp_to_hex(&txn, &rlp1));
  ok(eth_rlp_free(&rlp1));
  string raw(txn);
  free(txn);
  return raw;
}

when i send to json rpc server, final result is transaction hash and it worked successfully.

The above code using your old example works perfectly but only if i use uint8 for chainid and v. ➡️ If i use uint16 for chainid and uint8 for v, it results incorrect from address.

➡️ and if i use uint8 for chainid and uint16 for v it result

{"error":{"code":-32000,"message":"read V: rlp: non-canonical integer format"},"id":1,"jsonrpc":"2.0"}

➡️ if i encode both chainid and v in uint16, it results incorrect from address

➡️ if i encode r and s using uint8, it results incorrect from address.

So encoding r and s using bytes data type is correct and the problem is with data types of chainid and v. And i'm very sorry to create the misconception on u and this repo 🙏 . Also i updated the example code in the discussion section.

Thanks a lot brother

as i mentioned here, if i use uint16 for chain id and v, it doesn't work.

mhw0 commented 8 months ago

Thank you, I saw that. If v exceeds uint8, you should be able to use uint16. Because it takes 2 bytes and it's same thing if you encoded 2 byte integer.

DerXanRam commented 8 months ago

Thank you, I saw that. If v exceeds uint8, you should be able to use uint16. Because it takes 2 bytes and it's same thing if you encoded 2 byte integer.

ok wait ... let me try it.

DerXanRam commented 8 months ago

Yeaa it works. I used 400 for chainid(hyperonChain TestNet) and encoded using uint16, it worked successfully. I have one question. Does chainid and v grow proportnaly. I mean when i use uint16 for chainid, do i must use also for v?

DerXanRam commented 8 months ago

when i calculate byte of the data, i don't have to count the 0x part righ? like in data 0x190 there is only 3 bytes.

mhw0 commented 8 months ago

Yeaa it works. I used 400 for chainid(hyperonChain TestNet) and encoded using uint16, it worked successfully. I have one question. Does chainid and v grow proportnaly. I mean when i use uint16 for chainid, do i must use also for v?

Sorry. I didn’t get the question.

DerXanRam commented 8 months ago

Yeaa it works. I used 400 for chainid(hyperonChain TestNet) and encoded using uint16, it worked successfully. I have one question. Does chainid and v grow proportnaly. I mean when i use uint16 for chainid, do i must use also for v?

Sorry. I didn’t get the question.

Do i must set same data type for chainid and v?

mhw0 commented 8 months ago

Well, If your chainid and v are > 255, then yes, you have to.

DerXanRam commented 8 months ago

Ok thanks a lot brother 🙏🙏

mhw0 commented 8 months ago

Closing the issue again.