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

SHARD-2007: Sign multisig payload directly instead of its hash #169

Merged
merged 2 commits into from
Mar 20, 2025

Conversation

jintukumardas
Copy link
Contributor

@jintukumardas jintukumardas commented Mar 20, 2025

PR Type

Enhancement, Tests


Description

  • Sign multisig payload directly instead of hashing

  • Update signature verification to use direct payload

  • Modify unit tests to reflect signing changes

  • Adjust file path for reading configuration


Changes walkthrough 📝

Relevant files
Enhancement
archiver_config_sign.ts
Directly sign payload and update signature output               

scripts/archiver_config_sign.ts

  • Sign payload directly instead of its hash
  • Output signature in a new format
  • Adjust file path for configuration file
  • +11/-7   
    ticketVerification.ts
    Update signature verification to use direct payload           

    src/services/ticketVerification.ts

  • Verify signatures using direct payload
  • Replace payload hash with direct message
  • +2/-2     
    Tests
    ticketVerification.test.ts
    Modify tests for direct payload signing                                   

    test/unit/src/services/ticketVerification.test.ts

  • Update test to sign direct payload message
  • Remove hashing step in signature creation
  • +1/-2     
    archiverWhitelist.test.ts
    Update archiver whitelist test for direct signing               

    test/unit/src/shardeum/archiverWhitelist.test.ts

  • Change test signature to use direct message
  • Remove payload hashing in test setup
  • +2/-2     
    Miscellaneous
    allowed-archivers.json
    Update signature in configuration JSON                                     

    allowed-archivers.json

    • Update signature value in JSON
    +2/-2     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🏅 Score: 85
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    Ensure that the error handling logic in the generateSignature function is robust, especially when reading the configuration file and signing the message.

    async function generateSignature(): Promise<void> {
      try {
        // Get private key from env or command line
        const privateKey = process.env.PRIVATE_KEY || process.argv[2]
        if (!privateKey) {
          throw new Error('Private key not provided. Set PRIVATE_KEY in .env or provide as command line argument')
        }
    
        // Read and parse config file
        const configData: ConfigData = StringUtils.safeJsonParse(fs.readFileSync('../allowed-archivers.json', 'utf8'))
    
        // Create payload
        const rawPayload: SignaturePayload = {
          allowedArchivers: configData.allowedArchivers,
        }
    
        // Sign the stringified payload directly
        const signedMessage = StringUtils.safeStringify(rawPayload)
    
        // Initialize wallet and sign
        const wallet = new ethers.Wallet(privateKey)
        const signature = await wallet.signMessage(signedMessage)
        const address = wallet.address
        // Output signature in format needed for verification
        console.log('Signature object:', {
          owner: address,
          sig: signature
        })
    
      } catch (error) {
        console.error('Error generating signature:', error)
        process.exit(1)
      }
    Signature Verification Logic

    Verify that the logic for checking the validity of signatures is correctly implemented and that it handles edge cases, such as duplicate signatures or invalid public keys.

    // no reason to allow more signatures than allowedPubkeys exist
    // this also prevent loop exhaustion
    if (sigs.length > Object.keys(allowedPubkeys).length) return { isValid: false, validCount: 0 }
    
    let validSigs = 0
    const messageToSign = Utils.safeStringify(rawPayload)
    const seen = new Set()
    
    for (let i = 0; i < sigs.length; i++) {
      /* eslint-disable security/detect-object-injection */
      // The sig owner has not been seen before
      // The sig owner is listed on the server
      // The sig owner has enough security clearance
      // The signature is valid
      // The signature is not a duplicate
      if (
        !seen.has(sigs[i].owner.toLowerCase()) &&
        allowedPubkeys[sigs[i].owner] &&
        allowedPubkeys[sigs[i].owner] >= requiredSecurityLevel &&
        ethers.verifyMessage(messageToSign, sigs[i].sig).toLowerCase() === sigs[i].owner.toLowerCase()
      ) {
        validSigs++
        seen.add(sigs[i].owner.toLowerCase())
      }

    aniketdivekar
    aniketdivekar previously approved these changes Mar 20, 2025
    urnotsam
    urnotsam previously approved these changes Mar 20, 2025
    @aniketdivekar aniketdivekar dismissed stale reviews from urnotsam and themself via d3f243d March 20, 2025 14:45
    @mhanson-github mhanson-github merged commit f97f93d into mainnet-launch Mar 20, 2025
    5 checks passed
    @mhanson-github mhanson-github deleted the SHARD-2007 branch March 20, 2025 14:51
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants