Incorrect Use of secp256k1 Methods

[tosynthegeek]

There are incorrect usage of secp256k1 methods instead of the go-ethereum crypto package (ethCrypto) methods in the implementation of Secp256k1PrivateKey and Secp256k1PublicKey within the Aptos Go SDK here: https://github.com/aptos-labs/aptos-go-sdk/blob/main/crypto/secp256k1.go

These issues affect the correct handling of private keys and signature verification. Additionally, there are opportunities to improve the code readability and performance by addressing type assertion warnings and other minor issues.

I have identified the following problems and have a proposed solution. I will submit a pull request to address these issues.

Problem Details

File: https://github.com/aptos-labs/aptos-go-sdk/blob/main/crypto/secp256k1.go

Function: SignMessage

Current code:

func (key *Secp256k1PrivateKey) SignMessage(msg []byte) (sig Signature, err error) {
    hash := util.Sha3256Hash([][]byte{msg})
    // TODO: The eth library doesn't protect against malleability issues, so we need to handle those
    signature, err := secp256k1.Sign(hash, key.Bytes())
    if err != nil {
        return nil, err
    }

    // Strip the recovery bit
    return &Secp256k1Signature{
        signature[0:64],
    }, nil
}

• Issue: TheSign method from secp256k1 is used instead of ethCrypto

Function: Verify

Current Code:

func (key *Secp256k1PublicKey) Verify(msg []byte, sig Signature) bool {
    switch sig.(type) {
    case *Secp256k1Signature:
        typedSig := sig.(*Secp256k1Signature)

        return secp256k1.VerifySignature(key.Bytes(), msg, typedSig.Bytes())
    default:
        return false
    }
}

• Issue: The VerifySignature method from secp256k1 is used instead of ethCrypto.

  Proposed solution

  SignMessage function

  func (key *Secp256k1PrivateKey) SignMessage(msg []byte) (sig Signature, err error) {
  hash := util.Sha3256Hash([][]byte{msg})
  // TODO: The eth library doesn't protect against malleability issues, so we need to handle those
  signature, err := ethCrypto.Sign(hash, key.Inner)
  if err != nil {
      return nil, err
  }
  
  // Strip the recovery bit
  return &Secp256k1Signature{
      signature[0:64],
  }, nil
  }

  VerifySignature function

  func (key *Secp256k1PublicKey) Verify(msg []byte, sig Signature) bool {
  switch sig.(type) {
  case *Secp256k1Signature:
      typedSig := sig.(*Secp256k1Signature)
  
      return ethCrypto.VerifySignature(key.Bytes(), msg, typedSig.Bytes())
  default:
      return false
  }
  }

Additional improvement

Type assertion warning:

• Current:

  switch sig.(type) {

• Improved:

  switch sig := sig.(type) {

[gregnazario]

I'm fairly new to Go, would be great to put the additional improvement everywhere. Thanks for the contribution!

[tosynthegeek]

Sure, would do that. Just to know, do you have any idea when the new version of the code be published?