pallet_revive: Change address derivation to use hashing#7662
Conversation
| let eve = [5u8; 20]; | ||
| api::terminate(&eve); | ||
| input!(beneficiary: &[u8; 20],); | ||
| api::terminate(&beneficiary); |
There was a problem hiding this comment.
This code made assumptions about how the address is derived. So it had to be changed.
| } else { | ||
| // this is an (ed|sr)25510 derived address | ||
| // avoid truncating the public key by hashing it first | ||
| let account_hash = blake2_256(account_bytes); |
There was a problem hiding this comment.
blake2 is faster. Eth code will never need to deal with this hash so it is fine to not use keccak.
There was a problem hiding this comment.
Auditors recommended to move to keccak to be consistent with other protocols. While not strictly necessary it is just out of caution. Performance should not matter as we are just hashing a few bytes.
|
/cmd prdoc --audience runtime_dev --bump major |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
pgherveou
left a comment
There was a problem hiding this comment.
Looks great, glad we don't even need a migration
| ethabi = { workspace = true } | ||
| ethereum-types = { workspace = true, features = ["codec", "rlp", "serialize"] } | ||
| hex = { workspace = true } | ||
| hex-literal = { workspace = true } |
There was a problem hiding this comment.
Could you please use array-bytes as used in the rest of the code base?
There was a problem hiding this comment.
Both seem to be widely used in the code base. Should we transition away from hex-literal?
alexander:~/Developer/polkadot-sdk% cargo tree -i hex-literal --depth 1 | wc -l
22
alexander:~/Developer/polkadot-sdk% cargo tree -i array-bytes --depth 1 | wc -l
34
There was a problem hiding this comment.
At some point we tried to push for array-bytes, but all the other are coming back over time :P
There was a problem hiding this comment.
Just looked into array-bytes. It is not a replacement since it does the conversion at runtime. But we often need it at compile time as literal. Or do you mean the hex crate? I removed the direct dependency but we are still indirectly depending on it via ethereum-types. We will move away eventually to the newer alloy crates.
|
Moved back to keccak as hash after consultation with auditors. |
Fixes #6723 Internal auditors recommended to not truncate Polkadot Addresses when deriving Ethereum addresses from it. Reasoning is that they are raw public keys where truncating could lead to collisions when weaknesses in those curves are discovered in the future. Additionally, some pallets generate account addresses in a way where only the suffix we were truncating contains any entropy. The changes in this PR act as a safe guard against those two points. We change the `to_address` function to first hash the AccountId32 and then use trailing 20 bytes as `AccountId20`. If the `AccountId32` ends with 12x 0xEE we keep our current behaviour of just truncating those trailing bytes. This will allow us to still recover the original `AccountId20` because those are constructed by just adding those 12 bytes. Please note that generating an ed25519 key pair where the trailing 12 bytes are 0xEE is theoretically possible as 96bits is not a huge search space. However, this cannot be used as an attack vector. It will merely allow this address to interact with `pallet_revive` without registering as the fallback account is the same as the actual address. The ultimate vanity address. In practice, this is not relevant since the 0xEE addresses are not valid public keys for sr25519 which is used almost everywhere. tl:dr: We keep truncating in case of an Ethereum address derived account id. This is safe as those are already derived via keccak. In every other case where we have to assume that the account id might be a public key. Therefore we first hash and then take the trailing bytes. No. We changed the name of the mapping. This means the runtime will not try to read the old data. Ethereum keys are unaffected by this change. We just advise people to re-register their AccountId32 in case they need to use it as it is a very small circle of users (just 3 addresses registered). This will not cause disturbance on Westend. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #6723
Motivation
Internal auditors recommended to not truncate Polkadot Addresses when deriving Ethereum addresses from it. Reasoning is that they are raw public keys where truncating could lead to collisions when weaknesses in those curves are discovered in the future. Additionally, some pallets generate account addresses in a way where only the suffix we were truncating contains any entropy. The changes in this PR act as a safe guard against those two points.
Changes made
We change the
to_addressfunction to first hash the AccountId32 and then use trailing 20 bytes asAccountId20. If theAccountId32ends with 12x 0xEE we keep our current behaviour of just truncating those trailing bytes.Security Discussion
This will allow us to still recover the original
AccountId20because those are constructed by just adding those 12 bytes. Please note that generating an ed25519 key pair where the trailing 12 bytes are 0xEE is theoretically possible as 96bits is not a huge search space. However, this cannot be used as an attack vector. It will merely allow this address to interact withpallet_revivewithout registering as the fallback account is the same as the actual address. The ultimate vanity address. In practice, this is not relevant since the 0xEE addresses are not valid public keys for sr25519 which is used almost everywhere.tl:dr: We keep truncating in case of an Ethereum address derived account id. This is safe as those are already derived via keccak. In every other case where we have to assume that the account id might be a public key. Therefore we first hash and then take the trailing bytes.
Do we need a Migration for Westend
No. We changed the name of the mapping. This means the runtime will not try to read the old data. Ethereum keys are unaffected by this change. We just advise people to re-register their AccountId32 in case they need to use it as it is a very small circle of users (just 3 addresses registered). This will not cause disturbance on Westend.