Clarify the ECDSA security notice#6560
Conversation
🦋 Changeset detectedLatest commit: 98f3f39 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR clarifies the ECDSA library's security notice regarding hash input requirements in 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.changeset/quiet-radios-protect.md (1)
1-2: ⚡ Quick winReconsider this “empty changeset” if the ECDSA notice should reach
CHANGELOG.md.
.changeset/quiet-radios-protect.mduses empty front matter (--- ---), which is the Changesets “empty changeset” form (no packages are bumped). This repo has no other files using that form, so this entry won’t generate a user-facing changelog note. If the corrected ECDSA security guidance is intended to be discoverable inCHANGELOG.md, switch to a normal changeset that specifies the relevant package and semver type; otherwise the current “no changelog entry needed” approach is consistent with an empty changeset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/quiet-radios-protect.md around lines 1 - 2, The changeset file .changeset/quiet-radios-protect.md currently uses empty front matter (the "no changelog" form) so it won't produce a CHANGELOG.md entry; to make the ECDSA guidance appear in the changelog, replace the empty front matter with a standard changeset header that lists the target package names and a semver bump type (patch/minor/major) and include a short summary body; update the file so the front matter contains the package identifiers and bump level and keep the explanation text below it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/quiet-radios-protect.md:
- Line 4: Add a changelog entry to the existing changeset
"quiet-radios-protect.md" summarizing the ECDSA security notice clarification:
state that the ECDSA signature verification guidance was corrected to clarify
hash input requirements, reference issue `#5143`, include a brief one-line impact
summary for developers (e.g., potential implementation changes or
incompatibility risk), and mark the entry as a documentation/security note so it
appears in release notes.
In `@contracts/utils/cryptography/ECDSA.sol`:
- Around line 50-57: Update the two IMPORTANT paragraphs in ECDSA.sol to clarify
that signature ambiguity/malleability is an inherent concern regardless of
whether data is pre-hashed, and explain briefly that hashing helps by providing
preimage resistance which prevents certain signature-forging encodings; keep the
OpenZeppelin docs xref style for {MessageHashUtils-toEthSignedMessageHash} and
{MessageHashUtils-toTypedDataHash} as-is. Locate the text blocks around the
ECDSA helper docs (the existing IMPORTANT paragraphs) and replace wording that
implies the issue only affects “non-hashed data” with language stating the
ambiguity exists generally and that using
MessageHashUtils-toEthSignedMessageHash or MessageHashUtils-toTypedDataHash and
proper hashing mitigates the risk by binding the signature to a fixed-length
hash (preimage resistance). Ensure both occurrences are updated consistently.
---
Nitpick comments:
In @.changeset/quiet-radios-protect.md:
- Around line 1-2: The changeset file .changeset/quiet-radios-protect.md
currently uses empty front matter (the "no changelog" form) so it won't produce
a CHANGELOG.md entry; to make the ECDSA guidance appear in the changelog,
replace the empty front matter with a standard changeset header that lists the
target package names and a semver bump type (patch/minor/major) and include a
short summary body; update the file so the front matter contains the package
identifiers and bump level and keep the explanation text below it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b1f4c60b-3344-4a76-af74-27e9ed6b7493
📒 Files selected for processing (2)
.changeset/quiet-radios-protect.mdcontracts/utils/cryptography/ECDSA.sol
| --- | ||
| --- | ||
|
|
||
| No changelog entry needed for clarifying the ECDSA security notice. |
There was a problem hiding this comment.
Consider adding a changelog entry for this security documentation clarification.
While this is a documentation-only change, it corrects what issue #5143 describes as an "inaccurate" security notice regarding hash input requirements in ECDSA signature verification. Security-related documentation clarifications typically benefit from changelog visibility because:
- Developers may have implemented signature verification based on the previous unclear guidance
- Users reviewing security-related updates in changelogs would benefit from knowing about this clarification
- The correction affects how developers understand a critical security requirement
Consider changing this to document the clarification, for example:
---
---
-No changelog entry needed for clarifying the ECDSA security notice.
+Clarified the security notice in `ECDSA` regarding the requirement that the `hash` parameter must be the result of a hash operation. The notice now better explains the rationale for this requirement and cross-references `MessageHashUtils.toEthSignedMessageHash` and `MessageHashUtils.toTypedDataHash` for proper message hashing.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| No changelog entry needed for clarifying the ECDSA security notice. | |
| Clarified the security notice in `ECDSA` regarding the requirement that the `hash` parameter must be the result of a hash operation. The notice now better explains the rationale for this requirement and cross-references `MessageHashUtils.toEthSignedMessageHash` and `MessageHashUtils.toTypedDataHash` for proper message hashing. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/quiet-radios-protect.md at line 4, Add a changelog entry to the
existing changeset "quiet-radios-protect.md" summarizing the ECDSA security
notice clarification: state that the ECDSA signature verification guidance was
corrected to clarify hash input requirements, reference issue `#5143`, include a
brief one-line impact summary for developers (e.g., potential implementation
changes or incompatibility risk), and mark the entry as a documentation/security
note so it appears in release notes.
| * IMPORTANT: `hash` _must_ be the result of a hash operation for the | ||
| * verification to be secure: it is possible to craft signatures that | ||
| * recover to arbitrary addresses for non-hashed data. A safe way to ensure | ||
| * this is by receiving a hash of the original message (which may otherwise | ||
| * be too long), and then calling {MessageHashUtils-toEthSignedMessageHash} on it. | ||
| * verification to be secure. It is possible to craft signatures that recover | ||
| * to arbitrary addresses for non-hashed data. Most wallet providers append | ||
| * https://eips.ethereum.org/EIPS/eip-191[ERC-191] prefixes to signed | ||
| * messages to avoid encoding collisions with signed transactions. When | ||
| * verifying signatures, consider using | ||
| * {MessageHashUtils-toEthSignedMessageHash} for Ethereum-signed messages or | ||
| * {MessageHashUtils-toTypedDataHash} for typed data. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify NatSpec cross-reference syntax and check compilation
# Expected: Compilation succeeds and documentation is properly generated
echo "Checking if compilation succeeds with current NatSpec syntax..."
npm run compile -- --quiet 2>&1 | head -20
echo -e "\n---\nSearching for NatSpec cross-reference examples in the codebase..."
rg -n '\{[A-Za-z]+[.-][A-Za-z]+\}' --type sol -C 2Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 352
🌐 Web query:
Solidity NatSpec cross-reference syntax for linking to functions in other contracts
💡 Result:
Solidity NatSpec does not have a dedicated “cross-reference/link” syntax for linking to functions in other contracts. What you can do instead (supported by the official NatSpec rules): 1) Write normal NatSpec tags (@notice, @dev, @param, @return, etc.). These are emitted by the Solidity compiler into the NatSpec JSON; consumers (frontends/tools) display them as documentation, not as hyperlinks to other contracts’ functions.[1][2] 2) For cross-contract documentation inheritance, use @inheritdoc, which copies missing NatSpec tags from a base function and must specify the contract name. Example pattern: @inheritdoc BaseContractName (and it can apply to overridden functions in derived contracts). This is cross-contract, but it’s inheritance of documentation tags—not a link syntax.[2] 3) If you want “links” in human-readable docs, you’d typically include plain text (or whatever formatting your documentation consumer supports) inside @notice/@dev yourself; Solidity’s NatSpec format itself doesn’t define a structured link tag for referencing a particular function signature in another contract.[1][2] If you tell me what your tooling target is (e.g., Hardhat/Foundry-generated docs, a block explorer, or custom front-end consuming solc’s NatSpec JSON), I can suggest the most compatible way to represent references (often by including the canonical function signature like functionName(type1,type2) in the text, since solc’s NatSpec “methods” keys use canonical signatures).
Citations:
- 1: https://docs.solidity.org/en/latest/natspec-format.html
- 2: https://docs.soliditylang.org/en/develop/natspec-format.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="contracts/utils/cryptography/ECDSA.sol"
echo "== File exists? =="
ls -la "$FILE"
echo "== Context around ECDSA.sol lines ~40-140 =="
nl -ba "$FILE" | sed -n '35,140p'
echo "== Find NatSpec brace-link patterns in ECDSA.sol =="
rg -n '\{[^}]*[.-][^}]*\}' "$FILE" || true
echo "== Find whether dot or hyphen variants are used elsewhere =="
rg -n '\{[A-Za-z0-9_]+\.[A-Za-z0-9_]+\}' contracts || true
rg -n '\{[A-Za-z0-9_]+-[A-Za-z0-9_]+\}' contracts || true
echo "== Specifically check MessageHashUtils variants =="
rg -n '\{MessageHashUtils[^}]*\}' "$FILE" || true
rg -n '\{MessageHashUtils[^}]*\}' contracts/utils || trueRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 293
🌐 Web query:
Solidity NatSpec braces link syntax {ContractName.functionName} or {ContractName-functionName} hyphen
💡 Result:
Solidity NatSpec does not support a native linking syntax such as {ContractName.functionName} or {ContractName-functionName} [1][2][3]. The official Solidity NatSpec format, as defined in the Solidity documentation, uses Doxygen-style tags like @title, @author, @notice, @dev, @param, @return, and @inheritdoc [1][4]. While these tags allow for descriptive documentation, the language does not include a special markup syntax to create hyperlinked references to other contracts or functions within the comment strings themselves [1][2][3]. References to other functions in Solidity documentation are typically handled by developers manually typing the name of the function or contract within the text of the comment [1][5]. Unlike documentation systems such as Javadoc or TSDoc—which utilize tags like {@link} to generate cross-references—the Solidity compiler extracts NatSpec comments into structured JSON files (user and dev documentation) without processing or resolving internal link markup [1][3]. Consequently, neither the {dot} nor {hyphen} syntax is a recognized or functional feature of the Solidity NatSpec format [1][3].
Citations:
- 1: https://docs.solidity.org/en/latest/natspec-format.html
- 2: https://github.com/ethereum/solidity/blob/breaking/docs/natspec-format.rst
- 3: https://docs.soliditylang.org/en/develop/natspec-format.html
- 4: https://docs.soliditylang.org/en/latest/natspec-format.html
- 5: https://medium.com/coinmonks/natspec-comments-in-solidity-the-key-to-writing-readable-and-maintainable-code-7022e51a12e7
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="contracts/utils/cryptography/ECDSA.sol"
echo "== Context around ECDSA.sol (lines 35-140) =="
sed -n '35,140p' "$FILE" | cat
echo "== Search for brace reference patterns in ECDSA.sol =="
rg -n '\{[^}]*[.-][^}]*\}' "$FILE" || true
echo "== Search for dot/hyphen brace references across repo =="
rg -n '\{[A-Za-z0-9_]+\.[A-Za-z0-9_]+\}' contracts || true
rg -n '\{[A-Za-z0-9_]+-[A-Za-z0-9_]+\}' contracts || true
echo "== Search for MessageHashUtils brace references =="
rg -n '\{MessageHashUtils[^}]*\}' contracts || true
echo "== Show a couple matches near MessageHashUtils brace references =="
rg -n '\{MessageHashUtils[^}]*\}' contracts | head -n 20 || true
echo "== Look for doc tooling that might parse these braces =="
# common OZ doc tooling files/dirs
ls -la .github . || true
find . -maxdepth 3 -type f -iname "*docs*" -o -iname "*doxygen*" -o -iname "*natspec*" -o -iname "*solhint*" | head -n 50 || true
find . -maxdepth 4 -type f -iname "*.js" -o -iname "*.ts" | rg -n "documentation|doxygen|natspec|link|brace" -S || trueRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for exact phrase 'non-hashed data' =="
rg -n "non-hashed data" contracts/utils/cryptography || true
echo "== Inspect MessageHashUtils.sol around toEthSignedMessageHash and toTypedDataHash =="
FILE="contracts/utils/cryptography/MessageHashUtils.sol"
sed -n '1,220p' "$FILE" | cat
echo "== Search for how OZ processes { ... } xrefs =="
# check package.json scripts/tools for doc generation
ls -ლა package.json >/dev/null 2>&1 && sed -n '1,220p' package.json | cat || true
ls -la docs >/dev/null 2>&1 && find docs -maxdepth 3 -type f | head -n 50 || true
rg -n "solidity-docgen|natSpec|natspec|doxygen|xref|asciidoc|docgen|javadoc|markdown" package.json docs hardhat -S || true
echo "== Check for mention of 'toEthSignedMessageHash' in docs tooling scripts =="
rg -n "MessageHashUtils-toEthSignedMessageHash" -S . || trueRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for exact phrase 'non-hashed data' =="
rg -n "non-hashed data" contracts/utils/cryptography || true
echo "== Inspect MessageHashUtils.sol (relevant sections) =="
FILE="contracts/utils/cryptography/MessageHashUtils.sol"
sed -n '1,240p' "$FILE" | cat
echo "== Inspect package.json scripts/tools for doc generation =="
if [ -f package.json ]; then
sed -n '1,240p' package.json | cat
else
echo "package.json not found"
fi
echo "== Search for docgen/xref tooling =="
rg -n "docgen|solidity-docgen|asciidoc|adoc|doxygen|natspec|NatSpec|xref|cross[- ]ref|linking|xrefs" package.json docs hardhat lib .github -S || true
echo "== Confirm brace xref syntax patterns in repo (small sample) =="
rg -n '\{[A-Za-z0-9_]+\-[A-Za-z0-9_]+\}' contracts/utils contracts/interfaces -S | head -n 20
rg -n '\{[A-Za-z0-9_]+\.[A-Za-z0-9_]+\}' contracts/utils contracts/interfaces -S | head -n 20Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 50391
🌐 Web query:
solidity-docgen brace xref syntax {Contract-function} {Contract.function}
💡 Result:
The solidity-docgen “brace xref syntax” you’re asking about (e.g. {Contract-function} or {Contract.function}) is not a built-in, standardized syntax in solidity-docgen itself. What solidity-docgen actually supports is that its templates are Handlebars-based, and it ships an optional “prelude” template that can create cross-references. The prelude was removed from the default templates, but you can still copy contract.hbs and prelude.sample.hbs from the repository, rename it to prelude.hbs, and use it via the -t <templates-directory> option to enable the cross-reference behavior. [1] In other words: the {...} brace syntax (and the exact meaning of {Contract-function} vs {Contract.function}) is template-defined. You need to inspect your project’s prelude.hbs (or prelude.sample.hbs) / Handlebars helpers to see the exact delimiter/format that will be recognized. Evidence/starting points: 1) solidity-docgen output is fully configurable through Handlebars templates. [2] 2) The repo’s changelog states the prelude can be used “to create cross-references” and shows how to enable it by copying prelude.sample.hbs -> prelude.hbs. [1] 3) The OpenZeppelin docs repo also describes copying its docgen templates and customizing template helpers (e.g., templates-md/helpers.js) and contract.hbs, which indicates cross-reference formatting is part of template logic rather than a compiler feature. [3] If you paste your prelude.hbs (or prelude.sample.hbs) and whichever helpers file you’re using, I can tell you the exact correct syntax for cross-referencing contracts/functions in your setup.
Citations:
- 1: https://github.com/OpenZeppelin/solidity-docgen/blob/master/CHANGELOG.md
- 2: https://github.com/OpenZeppelin/solidity-docgen
- 3: https://github.com/OpenZeppelin/docs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find template files related to brace/xref handling
echo "== Locate prelude templates (prelude*.hbs) =="
find . -type f -iname "prelude*.hbs" -o -iname "*prelude*" | head -n 50
echo "== Locate solidity-docgen template/handlers usage =="
find docs -maxdepth 4 -type f \( -iname "*.js" -o -iname "*.hbs" \) | head -n 200
echo "== Show docs/config.js (docgen config) =="
sed -n '1,220p' docs/config.js | cat
echo "== Show docs/templates/helpers.js (xref helper) =="
sed -n '1,220p' docs/templates/helpers.js | cat
echo "== Search repo for prelude.sample.hbs / contract.hbs references =="
rg -n "prelude\.sample\.hbs|prelude\.hbs|contract\.hbs|solidity-docgen.*-t|templates-directory" -S docs scripts .github || true
echo "== Search for brace-xref parsing logic =="
rg -n "xref-|brace|\\{\\[A-Za-z]|MessageHashUtils-toEthSignedMessageHash" -S docs scripts templates lib contracts || true
echo "== Inspect docs/modules/api/cryptography adoc presence for MessageHashUtils =="
find docs/modules -maxdepth 4 -type f -name "*cryptography*.adoc" | head -n 50
echo "== Search for generated xrefs to MessageHashUtils-toEthSignedMessageHash =="
rg -n "MessageHashUtils-toEthSignedMessageHash" docs || trueRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 8081
Adjust the ECDSA “non-hashed data” security wording (brace xrefs are fine)
{MessageHashUtils-toEthSignedMessageHash}/{MessageHashUtils-toTypedDataHash}use OpenZeppelin’s docs-generation{...}xref convention (parsed bydocs/templates/helpers.js), so the hyphen brace style isn’t a NatSpec/compilation problem.- Update the IMPORTANT text (both occurrences) to avoid implying the concern only applies to “non-hashed data”; clarify that the underlying signature ambiguity/malleability exists regardless, and that hashing helps via preimage resistance (add a short “why hashing helps” sentence).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/utils/cryptography/ECDSA.sol` around lines 50 - 57, Update the two
IMPORTANT paragraphs in ECDSA.sol to clarify that signature
ambiguity/malleability is an inherent concern regardless of whether data is
pre-hashed, and explain briefly that hashing helps by providing preimage
resistance which prevents certain signature-forging encodings; keep the
OpenZeppelin docs xref style for {MessageHashUtils-toEthSignedMessageHash} and
{MessageHashUtils-toTypedDataHash} as-is. Locate the text blocks around the
ECDSA helper docs (the existing IMPORTANT paragraphs) and replace wording that
implies the issue only affects “non-hashed data” with language stating the
ambiguity exists generally and that using
MessageHashUtils-toEthSignedMessageHash or MessageHashUtils-toTypedDataHash and
proper hashing mitigates the risk by binding the signature to a fixed-length
hash (preimage resistance). Ensure both occurrences are updated consistently.
1 participant
Summary
hashmust come from a hash operationMessageHashUtils.toEthSignedMessageHashandMessageHashUtils.toTypedDataHashTesting
./node_modules/.bin/prettier --check contracts/utils/cryptography/ECDSA.sol./node_modules/.bin/solhint --config solhint.config.js --noPoster contracts/utils/cryptography/ECDSA.solnpm run compile -- --quietCloses #5143