Low-level ecrecover in Solidity contract accepts malleable signatures

[andrey-kuprianov]

Surfaced from @informalsystems audit of Althea Gravity Bridge at commit 19a4cfe

severity: Low
type: Implementation
difficulty: Low

Involved artifacts

• Solidity contract signature verification

Description

The utility function verifySig uses the low-level Solidity function ecrecover for recovering the address of the signer. This function accepts malleable signatures: the signature stays the same, but the signing address can be altered (albeit to an unpredictable one). Though this is not considered a serious security flow, it was still disallowed for Ethereum transaction signing via EIP-2; see point 2 there. For more details on the problem see the post on ECDSA Malleability.

Problem Scenarios

Given a valid call to the Gravity Bridge Solidity contract, involving a validator signature, it is possible to obtain another random public key such that the original signature remains valid for a different message and the new public key. This is not a vulnerability per se, but may lead to some yet unforeseen consequences related e.g. to slashing.

Recommendation

We recommend for the purposes of implementing verifySig to switch to the OpenZeppelin ECDSA contract, which provides the function recover with additional checks on top of ecrecover, as well as the convenience function toEthSignedMessageHash for forming Geth-style signatures.

This will have the added benefit of simplifying many function signatures and the corresponding implementations by switching frow accepting low-level signature components v, r, s, to a single signature; e.g. by changing the signature of updateValset from

function updateValset(
		// The new version of the validator set
		ValsetArgs memory _newValset,
		// The current validators that approve the change
		ValsetArgs memory _currentValset,
		// These are arrays of the parts of the current validator's signatures
		uint8[] memory _v,
		bytes32[] memory _r,
		bytes32[] memory _s
	)

to

function updateValset(
		// The new version of the validator set
		ValsetArgs memory _newValset,
		// The current validators that approve the change
		ValsetArgs memory _currentValset,
		// These is the array of validator's signatures
		bytes[] memory signatures
	)

It should also reduce the cost of executing the contract, because many checks like below will be simplified from

    // Check that current validators, powers, and signatures (v,r,s) set is well-formed
    require(
        _currentValset.validators.length == _currentValset.powers.length &&
            _currentValset.validators.length == _v.length &&
            _currentValset.validators.length == _r.length &&
            _currentValset.validators.length == _s.length,
        "Malformed current validator set"
    );

to

    // Check that current validators, powers, and signatures set is well-formed
    require(
        _currentValset.validators.length == _currentValset.powers.length &&
            _currentValset.validators.length == _signatures.length,
        "Malformed current validator set"
    );