Skip to content

Conversation

@byeongsu-hong
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/lib/LibSecp256k1.sol 50.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Member

@taeguk taeguk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uncompressPubkey already has some verification logic itself. Isn't it not enough?

@byeongsu-hong
Copy link
Member Author

@taeguk no it does check only for the format, not the public key itself. we need to check whether the pubKey is on curve

@byeongsu-hong
Copy link
Member Author

According to the context. Probably it is safe to not to apply this fix.

  1. ValidatorManager -> verifying pubKey with msg.sender. we can assume that the pubKey is on curve if the derived address is the same.
  2. ConsensusValidatorEntrypoint -> Only called by ValidatorManager, so we don't need to think about it.

@taeguk
Copy link
Member

taeguk commented Apr 24, 2025

@byeongsu-hong oh right. we should check.

@taeguk
Copy link
Member

taeguk commented Apr 24, 2025

ValidatorManager -> verifying pubKey with msg.sender. we can assume that the pubKey is on curve if the derived address is the same.

check ValidatorManager's Initialize(). It only has pubkey.

@byeongsu-hong byeongsu-hong requested a review from taeguk April 24, 2025 13:44
@byeongsu-hong
Copy link
Member Author

byeongsu-hong commented Apr 24, 2025

@taeguk given our security assumption, we don't need to verify the pubKey. Please check
Oh actually, i added isOnCurve inside of uncompressPubkey, so it verifies it every time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about keeping existing codes for potential usage in future?
And actually it was already included in audit scope.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm okay, i will keep it

@byeongsu-hong byeongsu-hong merged commit f8d4e51 into main Apr 25, 2025
@byeongsu-hong byeongsu-hong deleted the fix/verify-cmp-pubkey branch April 25, 2025 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants