Impossibility to interact with other EIP-1271 smartcontract

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/interfaces/ISignatureValidator.sol#L19
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342

Vulnerability details

Impact

Impossibility of SCW smartaccount to sign messages, and to accept signed messages, with EIP-1271 smartcontracts :

SmartAccount.sol#L342 - ISignatureValidator.sol#L5-L6 - ISignatureValidator.sol#L19

Proof of Concept

Signature validation is not conformant with EIP-1271 :

• signature validation is checked against a wrong value : 0x20c13b0b instead of 0x1626ba7e == EIP1271_MAGIC_VALUE , so it's impossible for EIP-1271 conformant smartcontract to sign a transaction (via execTransaction) on SCW smartaccount

• signature by SCW smartaccount is not implemented: missing both some signature function (like signMessage in GnosisSafe) and EIP-1271 ìsvalidSignature implementation

These 2 lines ISignatureValidator.sol#L5-L6 are not EIP1271 conformant (interface and sighash) :

    // bytes4(keccak256("isValidSignature(bytes,bytes)")
    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;

should be :

    // bytes4(keccak256("isValidSignature(bytes32,bytes)")
    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x1626ba7e;

Tools Used

Manual review

Recommended Mitigation Steps

Current Biconomy signature validation is nethertheless compatible with GnosisSafe accounts, but as said by GnosisSafe themselves, they do not conform to standards :
The current version of the Safe contracts (1.3.0 8) is stable, but is not following all standards.

Anyway Biconomy could propose compatibility with both GnosisSafe and EIP-1271 , that can be done with :

A1/ check of other smartcontracts, with EIP1271_MAGIC_VALUE 0x1626ba7e and to be compatible with GnosisSafe also with other value 0x20c13b0b, something like that :

    // dataHash has bytes32 type (and should be keccak256(data))
    try ISignatureValidator(_signer).isValidSignature(dataHash, contractSignature) returns (bytes4 res){
        require( res == EIP1271_MAGIC_VALUE,  "BSA024");
        return;
    } catch {}
    // data has bytes type 
    try ISignatureValidator(_signer).isValidSignature(data, contractSignature) returns (bytes4 res){
        require( res == EIP1271_MAGIC_VALUE_OLD,  "BSA024g");
    } catch {
        revert("BSA024z");
    }

A2/ with two interfaces functions in ISignatureValidator.sol :

contract ISignatureValidatorConstants {
    // bytes4(keccak256("isValidSignature(bytes32,bytes)")
    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x1626ba7e;

    // bytes4(keccak256("isValidSignature(bytes,bytes)")
    bytes4 internal constant EIP1271_MAGIC_VALUE_OLD = 0x20c13b0b;
}

abstract contract ISignatureValidator is ISignatureValidatorConstants {
    function isValidSignature(bytes32 memory _dataHash, bytes memory _signature) public view virtual returns (bytes4);

    function isValidSignature(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4);
}

And Biconomy should also propose signature of message, like this :

B1/ Implementation of message signature, something like GnosisSafe for example :

    mapping(bytes32 => uint256) public signedMessages;
    event SignMsg(bytes32 indexed msgHash);

    function signMessage(bytes calldata _data) external authorized {
        bytes32 msgHash = getMessageHash(_data);
        signedMessages[msgHash] = 1;
        emit SignMsg(msgHash);
    }

B2/ implementation of EIP1271 isValidSignature (inside SmartAccount.sol) with the help of ECDSA OpenZeppelin library :

    import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
    
    function isValidSignature(bytes32 memory _hash, bytes memory _signature) public view override returns (bytes4){
        bool validSignatureEOA = ECDSA.recover(_hash, _signature) == owner  ;
        bool validSignatureSCW = _signature.length == 0 && signedMessages[_hash] == 1;
        
        return bytes4( (validSignatureEOA || validSignatureSCW) ? 0x1626ba7e : 0xffffffff);
    }

[c4-judge]

gzeon-c4 marked the issue as duplicate of #370

[c4-sponsor]

livingrockrises marked the issue as sponsor acknowledged

[c4-judge]

gzeon-c4 marked the issue as satisfactory