Biconomy - Smart Contract Wallet contest - V_B's results

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L166 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L192 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L229 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/base/Executor.sol#L23

Vulnerability details

Description

If the SmartAccount implementation contract is not initialized, it can be destroyed using the following attack scenario:

• Initialize the SmartAccount implementation contract using the init function.
• Execute a transaction that contains a single delegatecall to a contract that executes the selfdestruct opcode on any incoming call, such as:

contract Destructor {
    fallback() external {
        selfdestruct(payable(0));
    }
}

The destruction of the implementation contract would result in the freezing of all functionality of the wallets that point to such an implementation. It would also be impossible to change the implementation address, as the Singleton functionality and the entire contract would be destroyed, leaving only the functionality from the Proxy contract accessible.

---

In the deploy script there is the following logic:

const SmartWallet = await ethers.getContractFactory("SmartAccount");
const baseImpl = await SmartWallet.deploy();
await baseImpl.deployed();
console.log("base wallet impl deployed at: ", baseImpl.address);

So, in the deploy script there is no enforce that the SmartAccount contract implementation was initialized.

The same situation in scw-contracts/scripts/wallet-factory.deploy.ts script.

---

Please note, that in case only the possibility of initialization of the SmartAccount implementation will be banned it will be possible to use this attack. This is so because in such a case owner variable will be equal to zero and it will be easy to pass a check inside of checkSignatures function using the fact that for incorrect input parameters ecrecover returns a zero address.

Impact

Complete freezing of all functionality of all wallets (including complete funds freezing).

Recommended Mitigation Steps

Add to the deploy script initialization of the SmartAccount implementation, or add to the SmartAccount contract the following constructor that will prevent implementation contract from the initialization:

// Constructor ensures that this implementation contract can not be initialized
constructor() public {
    owner = address(1);
}

Lines of code

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

Vulnerability details

Description

The execTransaction function is designed to accept a relayed transaction with a transaction cost refund. At the beginning of the function, the startGas value is calculated as the amount of gas that the relayer will approximately spend on the transaction initialization, including the base cost of 21000 gas and the cost per calldata byte of msg.data.length * 8 gas. At the end of the function, the total consumed gas is calculated as gasleft() - startGas and the appropriate refund is sent to the relayer.

An attacker could manipulate the calldata to increase the refund amount while spending less gas than what is calculated by the contract. To do this, the attacker could provide calldata with zero padded bytes of arbitrary length. This would only cost 4 gas per zero byte, but the refund would be calculated as 8 gas per calldata byte. As a result, the refund amount would be higher than the gas amount spent by the relayer.

Attack scenario

Let’s a smart wallet user signs a transaction. Some of the relayers trying to execute this transaction and send a transaction to the SmartAccount contract. Then, an attacker can frontrun the transaction, changing the transaction calldata by adding the zeroes bytes at the end.

So, the original transaction has such calldata:

abi.encodeWithSignature(RelayerManager.execute.selector, (...))

The modified (frontrun) transaction calldata:

// Basically, just add zero bytes at the end
abi.encodeWithSignature(RelayerManager.execute.selector, (...)) || 0x00[]

PoC

The PoC shows that the function may accept the data with redundant zeroes at the end. At the code above, an attacker send a 100_000 meaningless zeroes bytes, that gives a 100_000 * 4 = 400_000 additional gas refund. Technically, it is possible to pass even more zero bytes.

pragma solidity ^0.8.12;

contract DummySmartWallet {
    function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) external {
        // Do nothing, just test that data with appended zero bytes are accepted by Solidity
    }
}

contract PoC {
    address immutable smartWallet;

    constructor() {
        smartWallet = address(new DummySmartWallet());
    }

    // Successfully call with original data
    function testWithOriginalData() external {
        bytes memory txCalldata = _getOriginalTxCalldata();

        (bool success, ) = smartWallet.call(txCalldata);
        require(success);
    }

    // Successfully call with original data + padded zero bytes
    function testWithModifiedData() external {
        bytes memory originalTxCalldata = _getOriginalTxCalldata();
        bytes memory zeroBytes = new bytes(100000);

        bytes memory txCalldata = abi.encodePacked(originalTxCalldata, zeroBytes);

        (bool success, ) = smartWallet.call(txCalldata);
        require(success);
    }

    function _getOriginalTxCalldata() internal pure returns(bytes memory) {
        Transaction memory transaction;
        FeeRefund memory refundInfo;
        bytes memory signatures;
        return abi.encodeWithSelector(DummySmartWallet.execTransaction.selector, transaction, uint256(0), refundInfo, signatures);
    }
}

Impact

An attacker to manipulate the gas refund amount to be higher than the gas amount spent, potentially leading to arbitrary big ether loses by a smart wallet.

Recommended Mitigation Steps

You can calculate the number of bytes used by the relayer as a sum per input parameter. Then an attacker won't have the advantage of providing non-standard ABI encoding for the PoC calldata.

// Sum the length of each  bynamic and static length parameters.
uint256 expectedNumberOfBytes = _tx.data.length + signatures.length + 12 * 32;
uint256 dataLen = Math.min(msg.data.length, expectedNumberOfBytes);

Please note, the length of the signature must also be bounded to eliminate the possibility to put meaningless zeroes there.

Lines of code

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

Vulnerability details

Description

The deployCounterFactualWallet function deploys a smart wallet using the create2 function with a salt value that depends on the _owner and _index parameters. However, the address derivation for the deployed wallet does not depend on the _entryPoint and _handler parameters, which are used during initialization. This allows an attacker to front run the transaction and deploy a smart contract with their own _handler and _entryPoint, but same address. The attacker can then call the execFromEntryPoint function on the smart wallet, allowing them to execute any desired logic, such as changing the owner. As a result, users cannot rely on the address derivation in advance or deploy the wallet with the same address on different EVM chains.

Impact

If users rely on the address derivation in advance or try to deploy the wallet with the same address on different EVM chains, any funds sent to the wallet could potentially be withdrawn by anyone else. All in all, it could lead to the theft of user funds.

Recommended Mitigation Steps

Change formula with which salt is calculated to depends _entryPoint and _handler.

Lines of code

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

Vulnerability details

Description

In the checkSignatures there are checks that the signer is the account owner, but in the case of EIP-1271 signature check there are no such checks:

// 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");

So everyone can sign any transaction using the EIP-1271 signature validation method and convince the wallet that the valid signature was verified.

Impact

The complete absence of signature verification, and as a result, the possibility of performing any transaction by a third party.

Recommended Mitigation Steps

Add the following check into the EIP-1271 signature check logic:

require(_signer == owner, "INVALID_SIGNATURE");

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L239 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L264 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L424

Vulnerability details

Description

For the calculation of the amount of the token to be paid to the relayer tokenGasPriceFactor value is used. The corresponding logic is the following:

payment = (gasUsed + baseGas) * (gasPrice) / (tokenGasPriceFactor);
require(transferToken(gasToken, receiver, payment), "BSA012");

So, the number of tokens that the relayer should receive is inversely proportional to the value of this variable. But the tokenGasPriceFactor parameter is not signed by the owner:

bytes32 safeTxHash =
    keccak256(
        abi.encode(
            ACCOUNT_TX_TYPEHASH,
            _tx.to,
            _tx.value,
            keccak256(_tx.data),
            _tx.operation,
            _tx.targetTxGas,
            refundInfo.baseGas,
            refundInfo.gasPrice,
            refundInfo.gasToken,
            refundInfo.refundReceiver,
            _nonce
        )
    );

Using this fact the relayer can pass any value of tokenGasPriceFactor parameter to receive a greater amount than the user expected to pay.

Attack scenario

The user sends a transaction to the relayer. By the formula, payment value should be equal gasUsed/tokenGasPriceFactor. The user expects that the tokenGasPriceFactor will be equal to 100 for the specified token, but the relayer put tokenGasPriceFactor to 1. Relayer receives 100 times greater payout.

Impact

Obtaining a greater relayer benefit than the user expects, theft of user funds.

Recommended Mitigation Steps

Add the refundInfo.tokenGasPriceFactor to the preimage of the tx hash which should be signed by the owner.

Lines of code

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

Vulnerability details

Description

The execTransaction function includes an input parameter called batchId that is used to determine the nonce which is included in the data signed by the owner. However, batchId is not part of the signed data. This allows any third party to replay a signed transaction using a different batchId value, as the nonce values will be the same.

Impact

Possibility of replay of any transaction for which there exists a batchId with corresponding nonce value.

Recommended Mitigation Steps

Add batchId into Transaction struct and include its value into the tx hash preimage in encodeTransactionData function.

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L68 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/libs/MultiSend.sol#L26

Vulnerability details

Description

The handleOps function executes an array of UserOperation. If at least one user operation fails the whole transaction will revert. That means the error on one user ops will fully reverts the other executed ops.

The multiSend function reverts if at least one of the transactions fails, so it is also vulnerable to such type of attacks.

Attack scenario

Relayer offchain verify the batch of UserOperations, convinced that they will receive fees, then send the handleOps transaction to the mempool. An attacker front-run the relayers transaction with another handleOps transaction that executes only one UserOperation, the last user operation from the relayers handleOps operations. An attacker will receive the funds for one UserOperation. Original relayers transaction will consume gas for the execution of all except one, user ops, but reverts at the end.

Impact

Griefing attacks on the gas used for handleOps and multiSend function calls.

Please note, that while an attacker have no direct incentive to make such an attacks, they could short the token before the attack.

Recommended Mitigation Steps

Remove redundant require-like checks from internal functions called from the handleOps function and add the non-atomic execution logic to the multiSend function.

Judge has assessed an item in Issue #504 as 2 risk. The relevant finding follows:

Incorrect signature check in the validatePaymasterUserOp function

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L176 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L265 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L319 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L363 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L455 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L458

Vulnerability details

Description

According to the EIP-150 call can consume as most 63/64 of parent calls' gas. That means that it is possible to manipulate the gas amount to be passed into calls mentioned in the "Links to affected code" section. Specifically, if the amount of gas that is left in the current execution flow is smaller than requiredGasForTheCall*64/63 (where requiredGasForTheCall means the amount of gas that should be passed to the following call) then the wrong amount of gas will be passed, but the parent execution can be finished using the rest of the gas (which can be as big as requiredGasForTheCall/64), so the refund for the transaction execution will be provided to the relayer.

Attack scenario

Let's see the _executeUserOp logic:

try this.innerHandleOp(userOp.callData, opInfo, context) returns (uint256 _actualGasCost) {
    collected = _actualGasCost;
} catch {
    uint256 actualGas = preGas - gasleft() + opInfo.preOpGas;
    collected = _handlePostOp(opIndex, IPaymaster.PostOpMode.postOpReverted, opInfo, context, actualGas);
}

According to the 63/64 rule and code above, the innerHandleOp could be reverted due to gas limit issue on the child call and the all remaining gas will be spent on the _handlePostOp, that save the relayer refund. Thus, the relayer can fail execute transactions but invalidate the nonce and take funds for relay.

For a rough estimate of the cost of user operations that are subject to this type of attack, consider operation with 2_000_000 gas limit. Then, the 2_000_000 / 64 == 31_250, that should be enough to execute _handlePostOp without paymaster and rewrite one storage slot.

Impact

Any sufficiently expensive transaction can be forcfully failed by the relayer, but the relayer will receive refund. The problem may be occurs on the malicious relayer or even honest if it will send relay transaction with certain gas.

Recommended Mitigation Steps

Include logic related to the EIP-150 specification to the gas management. You can use the following:

• Check that the amount of gas left is great enough:

require(gasleft()*63/64 >= requiredGasForTheCall);

• Call with the required gas parameter:

contractAddress.call{gas: requiredGasForTheCall}(dataForTheCall);

Lines of code

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

Vulnerability details

Description

The SmartAccount contract inherits many contracts, some of which have their own storage management logic. In case of an upgrade, adding new storage variables to the inherited contracts will colapse the storage layout. This will create an inconvenience for updates with possible vulnerabilities on storage overlaps.

Impact

Deterioration of code architecture in case of an upgrade that creates storage overlaps.

Recommended Mitigation Steps

Use a base contract for all storage management logic related to the SmartAccount. Another way to solve this is to add a storage gap to all contracts that SmartAccount inherits.