Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens validation around HD-expanded Ed25519 scalar usage in packages/crypto to prevent inconsistent scalar handling between public key derivation and signing.
Changes:
- Added a scalar validation helper to assert the scalar’s top bit (bit 255) is not set.
- Enforced this validation in both HD-based signing (
rawSign) and public key derivation (rawPubkey). - Removed the previous “clear top bit” behavior in pubkey derivation in favor of rejecting invalid inputs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
In the xHD lib, the signing function uses [crypto_scalarmult_ed25519_base_noclamp](https://github.com/algorandfoundation/xHD-Wallet-API-ts/blob/96e7a4be6bca67a4f77252206811f7676e59e5ec/src/x.hd.wallet.api.crypto.ts#L144-L144) to get the public key which [clears the top bit](https://github.com/algorandfoundation/xHD-Wallet-API-ts/blob/9849fb3e90cecfb6348e188ff445b55806bfde00/src/sumo.facade.ts#L106-L106). Then for the signing, the [raw scalar](https://github.com/algorandfoundation/xHD-Wallet-API-ts/blob/96e7a4be6bca67a4f77252206811f7676e59e5ec/src/x.hd.wallet.api.crypto.ts#L156-L156) is used without clearing the top bit. Since this is not an exported function and the keys used are always from the known derivation function (which ensure the top bit is clear), then this is not an issue. In AlgoKit, however, we have no guarantees about where the scalar comes from. As such, it's possible for someone to pass a scalar that does not have the top bit cleared. The two options are to either clear it automatically or error, but since a scalar without the top bit cleared is invalid ed255519 scalar it seems preferable to just throw an error.
joe-p
force-pushed
the
decoupling-chore/assert_top_bit
branch
from
472678a to
4207ed3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🎉 This PR is included in version 10.0.0-alpha.45 🎉 The release is available on: Your semantic-release bot 📦🚀 |
In the xHD lib, the signing function uses crypto_scalarmult_ed25519_base_noclamp to get the public key which clears the top bit. Then for the signing, the raw scalar is used without clearing the top bit. Since this is not an exported function and the keys used are always from the known derivation function (which ensure the top bit is clear), then this is not an issue. In AlgoKit, however, we have no guarantees about where the scalar comes from. As such, it's possible for someone to pass a scalar that does not have the top bit cleared. The two options are to either clear it automatically or error, but since a scalar without the top bit cleared is invalid ed255519 scalar it seems preferable to just throw an error.