Ecrecover vulnerability

[wafardev]

Issue Template

Checklist

• I have searched the existing issues and pull requests for duplicates.

Type of Issue

• New vulnerability addition
• Feature request
• Update existing vulnerability

Description

Vulnerability in unexpected-ecrecover-null-address.md

The following function is presented as a solution to the ecrecover signature recovery problem:

function setOwner(bytes32 newOwner, uint8 v, bytes32 r, bytes32 s) external {
    address signer = ecrecover(newOwner, v, r, s);
    require(signer == owner && signer != address(0));
    owner = address(newOwner);
}

Issue

While it's impossible to create a fake signature from scratch, if the owner has already made a transaction on-chain, the transaction data (including the signature) can be retrieved. The corresponding hash that was signed (the pre-image) can then be re-computed. The ecrecover function will find the signer address and modify the owner to the address converted from newOwner (assuming implicit conversion from bytes32). This introduces a Denial-of-Service (DoS) vulnerability, as the newOwner can be set to an address without an existing private key linked to it.

Proof of Concept

async function main() {
  const provider = new ethers.providers.JsonRpcProvider(
    // insert your rpc here
  );
  const transactionHash = "" // insert any tx hash from the signer
  const tx = await provider.getTransaction(transactionHash);
  console.log(tx);
  const baseTx = {
    to: tx.to,
    nonce: tx.nonce,
    data: tx.data,
    value: tx.value,
    gasLimit: tx.gasLimit,
    gasPrice: tx.gasPrice,
    chainId: tx.chainId,
  };
  const sig = {
    r: tx.r,
    s: tx.s,
    v: tx.v,
  };
  // Serialize the unsigned transaction
  const unsignedTx = ethers.utils.serializeTransaction(baseTx);
  console.log("Unsigned Tx:", unsignedTx);
  // Get the transaction pre-image
  const preimage = ethers.utils.keccak256(unsignedTx);
  console.log("Preimage:", preimage);
  // ecrecover based on the signature and the preimage
  const from = ethers.utils.recoverAddress(preimage, sig); // same as ecrecover
  console.log(from); // the signer address found with pre-image and signature
}

Proposed Solution

To mitigate this vulnerability, the hash should be computed within the function:

function setOwner(address newOwner, uint8 v, bytes32 r, bytes32 s) external {
    bytes32 newOwnerHash = keccak256(abi.encodePacked(newOwner));
    address signer = ecrecover(newOwnerHash, v, r, s);
    require(signer == owner && signer != address(0));
    owner = newOwner;
}

This approach prevents the re-use of a pre-image with a signature to recover a signer. Only the owner would be able to sign the newOwnerHash offline before calling the function.

Additional Information

This critical vulnerability is very uncommon and must be removed, as it can have varying consequences, depending on what the function does after "verifying" that the owner "signed" the message. The severity and impact of this vulnerability can differ significantly based on the specific implementation and context of the smart contract.

Potential consequences may include:

1. Unauthorized access to privileged functions
2. Manipulation of contract state
3. Theft of funds or assets
4. Permanent loss of contract control

Given its rarity and potentially severe implications, it's crucial to document this vulnerability thoroughly in a separate, dedicated file. This will allow for a more comprehensive exploration of various scenarios, mitigation strategies, and best practices for secure signature verification in smart contracts. Also, using such code, which is not EIP-712 compliant, also introduces cross-chain signature replayability, also needs to be taken into account.

[kadenzipfel]

Thanks for submitting this. Want to make a PR?

[wafardev]

Sure, I'd be happy to contribute to this open-source project.