Smart wallets or contarcts violates EIP1271 on `SavingsXDai.permit` action

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xfcb06a10abcef38d141af4d0dabf5881aa4ff7cc4eee906dc7170dc41c7fa42e Severity: low

Description: Description\ Root cause : not checking if signer.codlength > 0 when caling IERC1271.isValidSignature, instead validating with signature.length. Likelihood : All protcols check isValidSignature based on signer's code length, instead of the supplied signature's length, so the regular AA wallets cannot do SavingsXDai.permit

According to EIP1271, the signature length should be == 65, and if not the call on IERC1271.isValidSignature of contract to validate the sig will revert due to the strict check of signature length check == 65 there also. So smart wallets cannot do permit by sig, because in order to do it, the sig length should be > 0 on line 54 below. But the isValidSignature function implemeted on contarct/wallet is strictly checking if sig length == 65. And it is doing it according to reference implementation section of eip-1271. And all wallet providers follow this check.

From, reference implementation section of eip-1271

    function isValidSignature( bytes32 _hash, bytes calldata _signature ) external override view returns (bytes4) {
        if (recoverSigner(_hash, _signature) == owner) {
            return 0x1626ba7e;
        } else {
            return 0xffffffff;
        }
    }

    function recoverSigner( bytes32 _hash, bytes memory _signature) internal pure returns (address signer) {
>>>     require(_signature.length == 65, "SignatureValidator#recoverSigner: invalid signature length");
        ---- SNIP ----

    }

Attack Scenario\

A user with contract/smart wallet is doing SavingsXDai.permit and he calls with signature.length == 65, and it will go into the if flow on line 54 below and try to ecrecover the signer. But all the Account abstracted wallets implement isValidSignature with signature arametervalidated as length == 65 on the first line itself. That is according to the reference implementation section of eip-1271

SavingsXDai.sol

53:     function _isValidSignature(address signer, bytes32 digest, bytes memory signature) internal view returns (bool) {
54:   >>>   if (signature.length == 65) {
    ---- SNIP ----
63:             if (signer == ecrecover(digest, v, r, s)) {
65:                 return true;
66:             }
67:         }
69: 
70:         (bool success, bytes memory result) =
71:   >>>       signer.staticcall(abi.encodeWithSelector(IERC1271.isValidSignature.selector, digest, signature));
72:         return (success && result.length == 32 && abi.decode(result, (bytes4)) == IERC1271.isValidSignature.selector);
73:     }

75:     function permit(address owner, address spender, uint256 value, uint256 deadline, bytes memory signature) public {
    ---- SNIP ----

81:         bytes32 digest = keccak256( abi.encodePacked( "\x19\x01",
84:                 block.chainid == deploymentChainId ? _DOMAIN_SEPARATOR : _calculateDomainSeparator(block.chainid),
85:                 keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonce, deadline))
86:             )
87:         );
88: 
89:   >>>   require(_isValidSignature(owner, digest, signature), "SavingsXDai/invalid-permit");
    ---- SNIP ----

93:     }

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[greenlucid]

This is an issue in sDAI, not seer

[clesaege]

Yeah, this has no impact on Seer. Per competition rules, are excluded:

• Issues in third party contracts (those in the interaction folder + Kleros + Curate + DAI) which do not lead to issues with Seer (if you find those, we'll help you report them to their respective projects).

I don't have a particular dev contact for sDAI, but you can report it to the maker team.