Biconomy - Smart Contract Wallet contest - Koolex's results

Lines of code

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

Vulnerability details

Impact

Functions from the logic contract (SmartAccount) shouldn't be called directly. a bad actor can take the ownership by calling init directly then destroy it making all proxies (Smart Contract Wallets) pointing to a non-existing contract.

One could argue that Biconomy's admin can call init after deploying it. However, there is still a risk of front-runing it.

Proof of Concept

1. SmartAccount is left uninitialized. there is no code that's initializing it nor disabling initializing it.

2. The attacker initializes it to gets the ownership.

3. Since SmartAccount can delegate calls, the attacker can delegate a call to a malcious contract calling selfdestruct which results in destroying SmartAccount.

Tools Used

Manual analysis

Recommended Mitigation Steps

As per openzeppelin's doc:

To prevent the implementation contract from being used, you should invoke the _disableInitializers function in the constructor to automatically lock it when it is deployed

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    _disableInitializers();
}

https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializing_the_implementation_contract

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/interfaces/ISignatureValidator.sol#L4-L20 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342

Vulnerability details

Impact

1. The EIP1271_MAGIC_VALUE is incorrect. it has 0x20c13b0b which is different than what EIP-2171 specified 0x1626ba7e. It results in wrong contract signature always when validating.
2. isValidSignature should have hashData (bytes32) parameter and not data (bytes). Therefore, the hashData should be passed instead of data (in checkSignatures function) . This also results in wrong contract signature always since the signer contract expects dataHash and not the data its self.

Proof of Concept

Check:

• The EIP-2171 specs https://eips.ethereum.org/EIPS/eip-1271#specification

• Line 342 in SmartAccount

require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024");

• Line 19 in ISignatureValidator

    function isValidSignature(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4);

Tools Used

Manual analysis

Recommended Mitigation Steps

1. Change the implementation of ISignatureValidator to adhere to EIP-1271 specs.
2. In SmartAccount's checkSignatures function, pass the dataHash instead of data.

Lines of code

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

Vulnerability details

Impact

Any transaction that was already executed through execTransaction function can be executed again (many times not only once).

Proof of Concept

Explanation

checkSignatures function takes into account nonces to prevent a signature from being valid again. The nonces increases by one everytime a transaction gets executed which protects from replaying. However, Biconomy uses a 2D nonces to support batches, and the caller has to pass the batchId when calling execTransaction. A malicious actor can use a batchId that was never used (within the uint256 range which is big) and replay all transactions in the same order in a batchId that was used by a user.

Coded PoC

1. Go to line 491 in the file './test/smart-wallet/onboardings-test.ts' and duplicate the last transaction with a different batchId (e.g. 77) as folllows:

const txs: MetaTransaction[] = [
      buildContractCall(
        walletFactory,
        "deployCounterFactualWallet",
        [owner, entryPoint.address, handler.address, 1],
        0
      ),
      buildContractCall(
        userSCW,
        "execTransaction",
        [transaction, 1, refundInfo, signature],
        0
      ),
      buildContractCall(
        userSCW,
        "execTransaction",
        [transaction, 77, refundInfo, signature],
        0
      ),
    ];

2. At the end of the test case, change the expectation from 10 ether to 20 ether as follows:

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

3. Run the test file and it will pass.

npx hardhat test ./test/smart-wallet/onboardings-test.ts

Tools Used

Manual analysis

Recommended Mitigation Steps

Include the batchId as a part of the signed hashed data.

Lines of code

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

Vulnerability details

Impact

According to the Biconomy's docs (check contest overview),

Biconomy Smart Account is a smart contract wallet that builds on core concepts of Gnosis / Argent safes and implements an interface to support calls from account abstraction Entry Point contract

As per Biconomy, it doesn't support signature aggregation at the moment, and according to Account Abstraction specs EIP-4337

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

However, the SmartAccount reverts rather than returns SIG_VALIDATION_FAILED. This migh lead to incompatibility since any EntryPoint that would be used in future will expect a returned value in case of invalid signature instead of a revert.

Proof of Concept

validateUserOp function (in BaseSmartAccount) calls _validateSignature to validate the userOp (UserOperation). However, _validateSignature reverts if the signature is invalid rather than returning SIG_VALIDATION_FAILED.

Check SmartAccount contract for

    function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash, address)
    internal override virtual returns (uint256 deadline) {
        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");
        return 0;
    }

Tools Used

Manual analysis

Recommended Mitigation Steps

If the signature is invalid return SIG_VALIDATION_FAILED. for any other error, revert.

Note: adjust the EntryPoint contract according to the suggested change above.

Lines of code

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

Vulnerability details

Impact

If the implementation contract (SmartAccount) was upgraded to a contract which doesn't have upgrading mechanism (e.g. updateImplementation), then the proxy (i.e. Smart Contract Wallet) is locked forever with the new implementation.

Proof of Concept

Check updateImplementation in SmartAccount.

Tools Used

Manual analysis

Recommended Mitigation Steps

Use a security mechanism similiar to what's used in UUPS (openzeppelin).

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/ERC1967/ERC1967Upgrade.sol#L84-L88