Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

No check for signature malleability #38

Open
code423n4 opened this issue Oct 18, 2021 · 2 comments
Open

No check for signature malleability #38

code423n4 opened this issue Oct 18, 2021 · 2 comments
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Warden finding resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Handle

cmichel

Vulnerability details

The SignatureValidator.recoverAddrImpl function does not check for malleable signatures.
Without this check, anyone can derive a second, different but valid signature (for the same message).

Impact

As nonces are used and the signature bytes are never used themselves, not checking for malleability does not lead to issues.

Recommended Mitigation Steps

You might or might not want to implement these additional checks, depending on gas costs.
@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Warden finding labels Oct 18, 2021
code423n4 added a commit that referenced this issue Oct 18, 2021
@code423n4
cmichel issue #38
4962e2f
@Ivshti Ivshti added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons and removed sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Oct 19, 2021
@Ivshti
Copy link
Member

Ivshti commented Oct 19, 2021

As you said, because of the nonces, as well as not using the sig bytes as identifying data, this is a non-issue. Marking as resolved

@GalloDaSballo
Copy link
Collaborator

The finding is valid
There is no impact
Will downgrade to Non Critical

@GalloDaSballo GalloDaSballo added 0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation and removed 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments labels Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Warden finding resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Ivshti @GalloDaSballo @code423n4