Skip to content

Commit

Permalink
[KMS] Trim signature bytes for RLP encoding (#944)
Browse files Browse the repository at this point in the history
Fixes issues that sometimes invalid raw transaction bytes are generated
when using AWS. E.g.

```
0xf86b0285012a05f2008261a8948b733353ce21ebd8eb5ffd9f49d57830942e88158769ec95a3fa70008025a04f9dd75069b51d36ec47b5bf68e6c45f8b854cf17a661e734e7a7c651240eeaca00044280475c3d355c44829ac93118b4ebe044356185c1c08724c8bcfebbd4b3d
```

The issue seems to be that we use the web3 `Signature` type, which
encodes s and r as H256 (really they are just points on a curve). Later
in the code, we RLP encode those value to create the signed raw
transaction. Ethereum requires, at least in the context of signatures,
the minimal length RLP encoding for values (no leading 0s).
[H256's RLP
encoding](https://rust.velas.com/src/impl_rlp/lib.rs.html#50-72)
implementation does not trim leading 0s. U256's implementation however
does trim leading 0s
([source](https://rust.velas.com/src/impl_rlp/lib.rs.html#21-46)).

This PR addresses the issue by converting the H256 into U256 just before
RLP encoding it.

### Test Plan
Things still work:

```
cargo run --example kms
```

To ensure we can "just" trim the bytes of a leading 0 signature I used
this raw tx that our backend generated (cf above), loaded it into
https://toolkit.abdk.consulting/ethereum#rlp,transaction, removed the
leading 0s from the last element and then successfully decoded it in
https://tools.deth.net/tx-decoder
  • Loading branch information
fleupold authored Aug 2, 2023
1 parent d4f08ac commit 4b44725
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 5 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/pull-request.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ jobs:
strategy:
matrix:
include:
- rust: 1.65.0
- rust: 1.68.0
examples: false
continue-on-error: false
- rust: stable
Expand Down
4 changes: 3 additions & 1 deletion ethcontract/src/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,9 @@ async fn block_number(
}
let block_ = web3.eth().block(BlockId::Number(block)).await?;
let Some(block_) = block_ else {
return Err(Web3Error::InvalidResponse(format!("block {block:?} does not exist")));
return Err(Web3Error::InvalidResponse(format!(
"block {block:?} does not exist"
)));
};
Ok(block_.number.map(|n| n.as_u64()))
}
Expand Down
6 changes: 4 additions & 2 deletions ethcontract/src/transaction/kms.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,10 @@ impl Account {
encoder.append_raw(item.as_raw(), 1);
}
encoder.append(&v);
encoder.append(&signature.r);
encoder.append(&signature.s);
// RLP encoding doesn't allow leading zeros for s & r, yet default H256 RLP encoding preserves leading 0s
// By converting and encoding U256, we get rid of the leading zeros.
encoder.append(&U256::from(signature.r.as_bytes()));
encoder.append(&U256::from(signature.s.as_bytes()));

let raw_transaction = Bytes(match id {
Some(id) => [&[id], encoder.as_raw()].concat(),
Expand Down
2 changes: 1 addition & 1 deletion ethcontract/src/transport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ where

#[inline(always)]
fn send_batch_boxed(&self, requests: Vec<(RequestId, Call)>) -> BoxedBatch {
self.send_batch(requests.into_iter()).boxed()
self.send_batch(requests).boxed()
}

#[inline(always)]
Expand Down

0 comments on commit 4b44725

Please sign in to comment.