howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/libraries/Permit.sol#L5

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/libraries/Permit.sol#L5

import "@openzeppelin/contracts-upgradeable/utils/cryptography/SignatureCheckerUpgradeable.sol";

    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 {

        ..snip

        }

Evidently, EIP 1271 is being implemented in protocol, albeit via the SignatureCheckerUpgradeable inherited from openzeppelin which allows contracts to sign messages and works great in tandem with EIP 4337 (account abstraction).

For ERC-4337, the account is not deployed until the first UserOp is mined, previous to this, the account exists "counterfactually" — it has an address even though it's not really deployed. Usually, this is great since we can use the counterfactual address to receive assets without deploying the account first.

Now, not being able to sign messages from counterfactual contracts/accounts has always been a limitation of ERC-1271 since one can't call the _isValidSignature() function on them.

Here is the current method of validation: https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/libraries/Permit.sol#L18-L22

        if (AddressUpgradeable.isContract(owner)) {
            require(
                IERC1271Upgradeable(owner).isValidSignature(hash, abi.encodePacked(r, s, v)) ==
                    0x1626ba7e,
                "ERC1271: Unauthorized"
            );

Which calls the below via SignatureChecker


    function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) {
        (address recovered, ECDSA.RecoverError error, ) = ECDSA.tryRecover(hash, signature);
        return
            (error == ECDSA.RecoverError.NoError && recovered == signer) ||
            isValidERC1271SignatureNow(signer, hash, signature);
    }

    function isValidERC1271SignatureNow(
        address signer,
        bytes32 hash,
        bytes memory signature
    ) internal view returns (bool) {
        (bool success, bytes memory result) = signer.staticcall(
            abi.encodeCall(IERC1271.isValidSignature, (hash, signature))
        );
        return (success &&
            result.length >= 32 &&
            abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector));
    }

Which practically means that Reserve will fail to validate signatures for users of notorious wallets/projects, including Safe, Sequence, and Argent supporting ERC1271, but also ERC4337 wallets, even though a clear intention has been made to support signatures by EIP1271 compliant wallets, as confirmed by using the Eip-1271 method of validating signatures.

Crux of the issue is the fact that protocol is taking responsibility to check the validity of signatures, but partially failing to trigger signature validation signatures for a group of wallets from a provider since (the validation will succeed if the ERC4337 wallet is deployed) and given that the protocol decided to support contract-based wallets (that support ERC4337) and implement ERC1271, one could assume that they "inherit" the possibility from the wallet providers to support ERC4337 too.

Impact

This vulnerability easily impacts Reserve's ability to validate signatures for counterfactual ERC-4337 accounts, limiting the usability for users of certain wallets that rely on AA, leading to the limitation of functionalities in the protocol, since all operations that need the signatures attached to the typehashes wouldn't work for some set of users, i.e the availability of these functions is impacted.

Considering this has been clearly documented: https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/libraries/Permit.sol#L7

/// Internal library for verifying metatx sigs for EOAs and smart contract wallets

This then means that some of the protocol would be unable to validate real signatures even from users who are expected to integrate with the protocol since the revert from PermitLib#requireSignature() would bubble back up to all instances where it's been used across protocol, for e.g in StRSR.sol#L937.

Recommended Mitigation Steps

EIP-6492 solves this issue. The author of the EIP already implemented ERC-6492 in a universal library which verifies 6492, 712, and 1271 sigs with this pull request.

ERC6492 assumes that the signing contract will normally be a contract wallet, but it could be any contract that implements ERC-1271 and is deployed counterfactually.

If the contract is deployed, produce a normal ERC-1271 signature
If the contract is not deployed yet, wrap the signature as follows: concat(abi.encode((create2Factory, factoryCalldata, originalERC1271Signature), (address, bytes, bytes)), magicBytes)

Reserve could adopt the reference-implementation as stated in the EIP and delegate the responsibility of supporting counterfactual signatures to the wallets themselves, and this works because, as defined in the EIP, the wallet should return the magic value in both cases.