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

Incorrect value from serializeParams for array fields #279

Closed
defunctzombie opened this issue Sep 8, 2020 · 2 comments · Fixed by #282
Closed

Incorrect value from serializeParams for array fields #279

defunctzombie opened this issue Sep 8, 2020 · 2 comments · Fixed by #282

Comments

@defunctzombie
Copy link

I discovered this issue when working with the multisig approve signing flow. The message parameters generated from an approve message were not accepted by the network. When I compared the message parameter base64 from these signing tools to the one produced by lotus I discovered the discrepancy.

The serializeParams function is not properly serializing array data - it seems to be injecting separators when the Uint8 array values are >= 24. Below is an example script to reproduce the issue.

const signer = require("@zondax/filecoin-signing-tools");
const blake = require("blakejs");

function blake2b256(message) {
  const blakeCtx = blake.blake2bInit(32);
  blake.blake2bUpdate(blakeCtx, message);
  return blake.blake2bFinal(blakeCtx);
}

const params = {
  requester: "t018788",
  to: "t1b3kvd3e7i4rwtzehvyh6vojpixdq7ntemlg66dy",
  value: "1000000000000000000",
  method: 0,
  params: "",
};

const hash = blake2b256(signer.serializeParams(params));
console.log("Hash:", hash);

const txnParams = {
  TxnID: 4,
  ProposalHashData: Buffer.from(hash).toString("base64"),
};

console.log("Txn Params:", txnParams);

const serialized = signer.serializeParams(txnParams);

// The serialized value here is incorrect
// Instead of containing the 32 bytes from the proposal hash it contains 64 bytes
// Each of the original 32 bytes is broken up with another byte - decimal value 24
console.log("Serialized:", serialized);
console.log(Buffer.from(serialized).toString("base64"));

Running the above script will show a discrepancy between the 'Hash' value and the 'Serialized' value at the end. The correct output should have the serialized value without filler values - but instead the output is filled with decimal '24' value between each value. This occurs whenever the value of the ProposalHashData array is >= 24 as noted in the code comment. If for example I feed in ProposalHashData equivalent of 32 0's the serialization works as expected.

This bug in serializing the ProposalHashData leads to incorrect message parameters on the multisig approval message.

@defunctzombie defunctzombie changed the title Incorrect value from serializeParams when using base64 input. Incorrect value from serializeParams for array fields Sep 8, 2020
@defunctzombie
Copy link
Author

After digging into this, I've traced the issue to how the proposal_hash field of TxnIDParams is serialized (https://github.com/Zondax/filecoin-signing-tools/blob/master/extras/src/lib.rs#L68).

The rust serde_cbor library wants to serialize each element of the array as a separate item (for some reason only when the value is <24 - I suspect having to do with cbor encoding). The lotus (golang) version serializes these as a continuous set of bytes.

While I don't know if this is the most appropriate fix - the following does result in serializing the bytes as a continuous set rather than individually.

diff --git a/extras/src/lib.rs b/extras/src/lib.rs
index ca19845..75856cb 100644
--- a/extras/src/lib.rs
+++ b/extras/src/lib.rs
@@ -65,9 +65,17 @@ pub struct ProposalHashData {
 #[derive(Serialize_tuple, Deserialize_tuple)]
 pub struct TxnIDParams {
     pub id: TxnID,
+    #[serde(serialize_with = "serde_byte_slice")]
     pub proposal_hash: [u8; 32],
 }
+pub fn serde_byte_slice<S>(v: &[u8; 32], serializer: S) -> Result<S::Ok, S::Error>
+where
+    S: serde::Serializer,
+{
+    serializer.serialize_bytes(v)
+}
+

This issue (pyfisch/cbor#78) from the serde cbor library hinted at potential fixes that I was able to apply as above.

Depending on the needs of the library it might be necessary to create a custom deserialization routine - and add the appropriate tests. I'm not familiar with rust to know where the appropriate pieces for all of this should go; I was able to build the library to wasm so I could test my fix and would need some guidance on how best to land the fix.

@rllola
Copy link
Contributor

rllola commented Sep 10, 2020

Yeah thanks indeed it wasn't the correct serialization. Created a PR that should fix it.

@jleni jleni linked a pull request Sep 15, 2020 that will close this issue
@jleni jleni closed this as completed Sep 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants