Lack of owner verification in EIP-1271 signature check

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L218
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342

Vulnerability details

Description

In the checkSignatures there are checks that the signer is the account owner, but in the case of EIP-1271 signature check there are no such checks:

// If v is 0 then it is a contract signature
// When handling contract signatures the address of the contract is encoded into r
_signer = address(uint160(uint256(r)));

// Check that signature data pointer (s) is not pointing inside the static part of the signatures bytes
    // This check is not completely accurate, since it is possible that more signatures than the threshold are send.
    // Here we only check that the pointer is not pointing inside the part that is being processed
    require(uint256(s) >= uint256(1) * 65, "BSA021");

    // Check that signature data pointer (s) is in bounds (points to the length of data -> 32 bytes)
    require(uint256(s) + 32 <= signatures.length, "BSA022");

    // Check if the contract signature is in bounds: start of data is s + 32 and end is start + signature length
    uint256 contractSignatureLen;
    // solhint-disable-next-line no-inline-assembly
    assembly {
        contractSignatureLen := mload(add(add(signatures, s), 0x20))
    }
    require(uint256(s) + 32 + contractSignatureLen <= signatures.length, "BSA023");

    // Check signature
    bytes memory contractSignature;
    // solhint-disable-next-line no-inline-assembly
    assembly {
        // The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s
        contractSignature := add(add(signatures, s), 0x20)
    }
    require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024");

So everyone can sign any transaction using the EIP-1271 signature validation method and convince the wallet that the valid signature was verified.

Impact

The complete absence of signature verification, and as a result, the possibility of performing any transaction by a third party.

Recommended Mitigation Steps

Add the following check into the EIP-1271 signature check logic:

require(_signer == owner, "INVALID_SIGNATURE");

[c4-judge]

gzeon-c4 marked the issue as duplicate of #175

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[livingrockrises]

confirmed duplicate of #175

[c4-judge]

gzeon-c4 marked the issue as satisfactory