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.

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

Signature malleability in permit() and permitPositionManagerApproval() #94

Closed
c4-submissions opened this issue Nov 9, 2023 · 6 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working insufficient quality report This report is not of sufficient quality primary issue Highest quality submission among a set of duplicates sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/EBTCToken.sol#L199-L221
https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/BorrowerOperations.sol#L706

Vulnerability details

Impact

Both permit() and permitPositionManagerApproval() are vulnerable to signature malleability, which allows replay attacks.

During the previous Code4rena contest, the similar attack vector has been evaluated as Medium/High

Based on that, I've decided to evaluate the risk of this issue as Medium. It's worth to note, that in most of the previous Code4rena contests, the signature malleability vulnerabilities had been detected during the bot race, so there weren't reported as a separate issue. Thus, there aren't many contests which could be used as a reference for proper severity categorization. In the current contest - the bot race did not report signature malleability issue, so I'm reporting it as a separate issue in this report.

Proof of Concept

  • In BorrowerOperations.sol, there is a function permitPositionManagerApproval() which sets given _approval for specified _borrower and _positionManager

FIle: BorrowerOperations

address recoveredAddress = ecrecover(digest, v, r, s);

In EBTCToken.sol, there's a function permit(), which approves given amount for specified owner and spender.

File: EBTCToken.sol

 address recoveredAddress = ecrecover(digest, v, r, s);

In both cases, there's no verification of v, r, s parameters.

https://swcregistry.io/docs/SWC-117/

 The EVM specification defines several so-called ‘precompiled’ contracts one of them being ecrecover which executes the elliptic curve public key recovery. A malicious user can slightly modify the three values v, r and s to create other valid signatures.

Tools Used

Manual code review

Recommended Mitigation Steps

Consider using OpenZeppelin's ECDSA library: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol

Assessed type

Invalid Validation

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 9, 2023
c4-submissions added a commit that referenced this issue Nov 9, 2023
@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Nov 15, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Nov 16, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as primary issue

@c4-sponsor
Copy link

GalloDaSballo (sponsor) disputed

@c4-sponsor c4-sponsor added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Nov 20, 2023
@GalloDaSballo
Copy link

Known + we use nonces

@c4-judge
Copy link

jhsagd76 marked the issue as unsatisfactory:
Out of scope

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working insufficient quality report This report is not of sufficient quality primary issue Highest quality submission among a set of duplicates sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

6 participants