Rigor Protocol contest - rotcivegaf's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/libraries/SignatureDecoder.sol#L11-L41

Vulnerability details

Impact

When recoverKey receive a messageSignatures with invalid length, the it's v != 27 && v != 28 or ecrecover return a address(0), the call don't revert instead of that return the address(0)

Proof of Concept

As SignatureDecoder.sol is a library, it will be used for several contracts, these could not take into account that an invalid signature does not revert but returns address(0) as signer Basically, you can make an arbitrary signature with an arbitrary message, with address(0) as signer

Tools Used

Manual review

Recommended Mitigation Steps

A solution it's use @openzeppelin-contracts/contracts/utils/cryptography/ECDSA.sol and use recover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) of OZ to recover the key of each position

function recoverKey(
    bytes32 messageHash,
    bytes memory messageSignatures,
    uint256 pos
) internal pure returns (address) {
    if (messageSignatures.length % 65 != 0) {
        return (address(0));
    }

    (uint8 v, bytes32 r, bytes32 s) = signatureSplit(messageSignatures, pos);
    ECDSA.recover(messageHash, v, r, s)
}

QA report

Low Risk

[L-01] The recoverKey function don't out of it's bound

In SignatureDecoder.sol, recoverKey function if call this function with a high pos, this overflow the messageSignatures, this would contain bytes (0) or maybe dirty bytes

Check if the position does not overflow the array:

function recoverKey(
    bytes32 messageHash,
    bytes memory messageSignatures,
    uint256 pos
) internal pure returns (address) {
    uint256 messageSignaturesLength = messageSignatures.length;
    if (messageSignaturesLength % 65 != 0) {
        return (address(0));
    }
    if (pos < messageSignaturesLength / 65) {
        return (address(0));
    }

    uint8 v;
    bytes32 r;
    bytes32 s;
    (v, r, s) = signatureSplit(messageSignatures, pos);

    // If the version is correct return the signer address
    if (v != 27 && v != 28) {
        return (address(0));
    } else {
        // solium-disable-next-line arg-overflow
        return ecrecover(toEthSignedMessageHash(messageHash), v, r, s);
    }
}