Weak ERC1271 Signature Validation in Permit.sol

[c4-bot-1]

Lines of code

https://github.com/reserve-protocol/protocol/blob/master/contracts/libraries/Permit.sol#L10-L33

Vulnerability details

Impact

The code has a flawed implementation of ERC1271 signature validation.

When checking an ERC1271 signature, the function requireSignature treats any return value of 0x1626ba7e from isValidSignature as a valid signature.

However, according to the ERC1271 standard, the function isValidSignature should return magicValue (which equals to bytes4(keccak256("isValidSignature(bytes32,bytes)")) but not necessarily 0x1626ba7e).

This incorrect check can lead to potential vulnerabilities. For example, a malicious contract could easily fake an ERC1271 signature since it knows the expected return value.

Proof of Concept

https://github.com/reserve-protocol/protocol/blob/master/contracts/libraries/Permit.sol#L10-L33

function requireSignature( address owner, bytes32 hash, uint8 v, bytes32 r, bytes32 s ) internal view {
        if (AddressUpgradeable.isContract(owner)) {
            require(
                IERC1271Upgradeable(owner).isValidSignature(hash, abi.encodePacked(r, s, v)) == 0x1626ba7e,
                "ERC1271: Unauthorized"
            );
        } else {
            require(
                SignatureCheckerUpgradeable.isValidSignatureNow(
                    owner,
                    hash,
                    abi.encodePacked(r, s, v)
                ),
                "ERC20Permit: invalid signature"
            );
        }
} 

Tools Used

Manual Review

Recommended Mitigation Steps

Check the response from isValidSignature against the correct magicValue from ERC1271 standard.

Consider making and using a constant that's assigned the correct magicValue to avoid possible errors.

Additionally, as good practice, the contract interacting with the ERC1271 signer should be audited to ensure that it behaves correctly.

This can help prevent security risks from an 'evil' contract providing a valid response without actually checking the signature.

bytes4 constant internal MAGICVALUE = 0x20c13b0b;
require(
    IERC1271Upgradeable(owner).isValidSignature(hash, abi.encodePacked(r, s, v)) == MAGICVALUE,
    "ERC1271: Unauthorized"
);

Assessed type

Other