Signature Malleability in `contracts/core/VotingEscrowUpgradeable.sol::delegateBySig`.

[hats-bug-reporter[bot]]

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x64f5a807cbfc4a5161a0b36f5eb14a6bbc07c55bedf9d9b7eb38402cb661d3ab Severity: medium

Description: Description

Using built in EVM's ecrecover is prone to signature malleability, therefore it is recommended to use ECDSA.recover from openzeppelin library which will validate the values of v and s in signatures.

Attack Scenario

Openzeppelin states the following bug from ecrecover and how to counter it with further validation of v and s value of signature used in ECDSA.tryrecover of openzeppelin library.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9e3f4d60c581010c4a3979480e07cc7752f124cc/contracts/utils/cryptography/ECDSA.sol#L119-L145

// EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature
// unique. Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines
// the valid range for s in (301): 0 < s < secp256k1n ÷ 2 + 1, and for v in (302): v ∈ {27, 28}. Most
// signatures from current libraries generate a unique signature with an s-value in the lower half order.
//
// If your library generates malleable signatures, such as s-values in the upper range, calculate a new s-value
// with 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1 and flip v from 27 to 28 or
// vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept
// these malleable signatures as well.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9e3f4d60c581010c4a3979480e07cc7752f124cc/contracts/utils/cryptography/ECDSA.sol#L39-L70

/* The `ecrecover` EVM precompile allows for malleable (non-unique) signatures:
this function rejects them by requiring the `s` value to be in the lower
half order, and the `v` value to be either 27 or 28. */

Attachments

NA

1. Proof of Concept (PoC) File

In contracts/core/VotingEscrowUpgradeable.sol::delegateBySig:

 function delegateBySig(address delegatee, uint nonce, uint expiry, uint8 v, bytes32 r, bytes32 s) public {
        require(delegatee != msg.sender);
        require(delegatee != address(0));

        bytes32 domainSeparator = keccak256(
            abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), keccak256(bytes(version)), block.chainid, address(this))
        );
        bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));
        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
        address signatory = ecrecover(digest, v, r, s); //erictee-issue: Signature Malleability when using built-in ecrecover.
        require(signatory != address(0), "VotingEscrow::delegateBySig: invalid signature");
        require(nonce == nonces[signatory]++, "VotingEscrow::delegateBySig: invalid nonce");
        require(block.timestamp <= expiry, "VotingEscrow::delegateBySig: signature expired");
        return _delegate(signatory, delegatee);
    }

2. Revised Code File (Optional)

It is recommended to use the recover() function from OpenZeppelin’s ECDSA library for signature verification

[0xmahdirostami]

for now, I will judge these submissions as well

[0xmahdirostami]

Security best practice