Biconomy - Smart Contract Wallet contest - 0x1f8b's results

Lines of code

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

Vulnerability details

Impact

The salt used for create2 does not include information from the init method, so it is vulnerable to front-running.

Proof of Concept

it's impossible to override an existing contract in Ethereum. From EIP-684:

If a contract creation is attempted, due to either a creation transaction or the CREATE (or future CREATE2) opcode, and the destination address already has either nonzero nonce, or nonempty code, then the creation throws immediately, with exactly the same behavior as would arise if the first byte in the init code were an invalid opcode. This applies retroactively starting from genesis.

For that reason if the salt is repeated, the call fill fault.

bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));

If the user want to deploy a wallet using deployCounterFactualWallet with the _owner=A and _index=0, an attacker can listen the memory pool and deploy with an higer gas, a wallet with the same owner and index, but with different _entryPoint and _handler so the attacker can deny the original call, and make unwanted actions in the proxy.

BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);

Recommended Mitigation Steps

Include all received fields in the salt calculation.

• Low
  • 1. Use abstract for base contracts
  • 2. Mixing and Outdated compiler
  • 3. Wrong logic in paymasterContext
  • 4. Lack of checks
  • 5. Centralization risks
  • 6. Poxy isues
  • 7. Integer overflow
  • 8. Loss of tokens
• Non critical
  • 9. Open TODO
  • 10. Wrong comment
  • 11. Lack of ACK during owner change
  • 12. Unsafe low level call
  • 13. Wong naming
  • 14. Avoid hardcoded values

Low

1. Use abstract for base contracts

abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• Enum.sol:5
• MultiSend.sol:9
• MultiSendCallOnly.sol:8
• Executor.sol:7
• FallbackManager.sol:8
• Singleton.sol:6

The following ones should be libraries instead of contracts:

• SignatureDecoder.sol:5
• SecuredTokenTransfer.sol:5

2. Mixing and Outdated compiler

The pragma version used are:

pragma solidity 0.8.12;

Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

0.8.17

• Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.

Apart from these, there are several minor bug fixes and improvements.

3. Wrong logic in paymasterContext

The method comment states:

Encodes the paymaster context: sender, token, rate, and fee

However, only paymasterId is encoded.

    /**
     * @dev Encodes the paymaster context: sender, token, rate, and fee
     */
    function paymasterContext(
        UserOperation calldata op,
        PaymasterData memory data
    ) internal pure returns (bytes memory context) {
        return abi.encode(data.paymasterId);
    }

Affected source code:

• PaymasterHelpers.sol:21-29

4. Lack of checks

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

• BasePaymaster.sol:20
• BasePaymaster.sol:24

It doesn't check for range of _unstakeDelaySec and can leave locked forever with high time, it also doesn't check that msg.value is greater than 0, so a second call with 0, works and could not be expected.

• StakeManager.sol:59

5. Centralization risks

The owner can change the address of _entryPoint and affect user deposits when they call the deposit method (for example with front-running), thus requiring an unnecessary trust in the project to use.

Affected source code:

• BasePaymaster.sol:24

6. Poxy isues

There are a lack of checks during the constructor that allows an empty _implementation, fallback works with target = address(0) and the receive method does not call target, so tokens could be lost if it's called.

    fallback() external payable {
        address target;
        // solhint-disable-next-line no-inline-assembly
        assembly {
            target := sload(_IMPLEMENTATION_SLOT)
            calldatacopy(0, 0, calldatasize())
            let result := delegatecall(gas(), target, 0, calldatasize(), 0, 0)
            returndatacopy(0, 0, returndatasize())
            switch result
            case 0 {revert(0, returndatasize())}
            default {return (0, returndatasize())}
        }
    }

If the proxy is deployed with an empty address for implementation, the token will be lost.

Affected source code:

• Proxy.sol:15

7. Integer overflow

Because the code is inside an unchecked block, overflows can occur. If the call to innerHandleOp uses a high opInfo.preOpGas, actualGas might overflow the _handlePostOp method when it is in an unchecked region.

Affected source code:

• EntryPoint.sol:453
• EntryPoint.sol:469

8. Loss of tokens

If the user set the address of StakeManager as withdrawAddress.

        (bool success,) = withdrawAddress.call{value : withdrawAmount}("");

The sender will be StakeManager and the tokens are lost forever because it will be deposited as the contract himself.

    receive() external payable {
        depositTo(msg.sender);
    }

Affected source code:

• StakeManager.sol:120

---

Non critical

9. Open TODO

The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.

Affected source code:

TODO: copy logic of gasPrice?

• EntryPoint.sol:255

10. Wrong comment

The contract Math has the following comment:

Return the log in base 10, following the selected rounding direction, of a positive value.

But this is not true, because is base 256 as was fixed in https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3916.

Affected source code:

• Math.sol:336

11. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because of human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• BasePaymaster.sol:16
• SmartAccount.sol:109

12. Unsafe low level call

The following low level calls are insecure since they do not verify that the calling address is a contract and not an empty address or an EOA. For that reason, these methods are prone to phantom methods attacks.

Proof of Concept:

If you try to call the following methods with the following contract 0x000000000000000000000000000000000000000000000 (or any EOA) you can see that the transaction is successful, you have to check that the address is a valid contract and maybe check that the call returns a certain data. Otherwise it is possible to call it and lose tokens or produce undesired errors.

This remind me to this attack https://certik.medium.com/qubit-bridge-collapse-exploited-to-the-tune-of-80-million-a7ab9068e1a0 where we can see:

Affected source code:

• Exec.sol:57

13. Wong naming

Authoritative internal methods must use '_' before the name. We have an example in Singleton.sol#L12 where the methods that require authorization but are internal, a different naming is established.

Affected source code:

• Executor.sol:13

14. Avoid hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these values from the source code.

Affected source code:

Use the selector instead of the raw value:

• DefaultCallbackHandler.sol:22
• DefaultCallbackHandler.sol:32
• DefaultCallbackHandler.sol:41
• SecuredTokenTransfer.sol:17
• Proxy.sol:11

• Gas
  • 1. Remove unnecessary variables
  • 2. Optimize internalIncrementDeposit
  • 3. Optimize unlockStake
  • 4. Reorder structure layout
  • 5. Use the unchecked keyword

Gas

1. Remove unnecessary variables

The following state variables can be removed without affecting the logic of the contract since they are not used and/or are redundant because they could be used inline.

Affected source code:

• In the following case is used, but also is added: VerifyingSingletonPaymaster.sol:99
• BasePaymaster.sol:50
• VerifyingSingletonPaymaster.sol:124

2. Optimize internalIncrementDeposit

It's possible to optimize the internalIncrementDeposit method like follows:

-   function internalIncrementDeposit(address account, uint256 amount) internal {
+   function internalIncrementDeposit(address account, uint256 amount) internal returns (uint112 ret) {
        DepositInfo storage info = deposits[account];
        uint256 newAmount = info.deposit + amount;
        require(newAmount <= type(uint112).max, "deposit overflow");
+       ret = uint112(newAmount);
+       info.deposit = ret;
-       info.deposit = uint112(newAmount);
    }
    function depositTo(address account) public payable {
-       internalIncrementDeposit(account, msg.value);
+       emit Deposited(account, internalIncrementDeposit(account, msg.value));
-       DepositInfo storage info = deposits[account];
-       emit Deposited(account, info.deposit);
    }

Gas diff:

In red the old version, in green with the applied changes:

- rcpt.gasUsed= 155168 0x31494a79c7dcc9163be9fc3c9f79c43547e4b39050c8b14920f02a5bcb97588d
+ rcpt.gasUsed= 154996 0x40cc316df0a2fb9d6261915aea960f905dd63e9da797f3d5e24b0fe68149c1c3
-   == actual gasUsed (from tx receipt)= 155168
+	== actual gasUsed (from tx receipt)= 154996
...
-   == actual gasUsed (from tx receipt)= 383762
+   == actual gasUsed (from tx receipt)= 383578
-   == calculated gasUsed (paid to beneficiary)= BigNumber { value: "334787" }
+	== calculated gasUsed (paid to beneficiary)= BigNumber { value: "334599" }

Affected source code:

• StakeManager.sol:38-43

3. Optimize unlockStake

You can use withdrawTime != 0 as a staked flag, so you could remove that field from the DepositInfo structure, saving space and gas.

Affected source code:

• StakeManager.sol:86

4. Reorder structure layout

The following structures could be optimized moving the position of certain values in order to save some storage slots:

struct Transaction {
        address to;
+       Enum.Operation operation;
        uint256 value;
        bytes data;
-       Enum.Operation operation;
        uint256 targetTxGas;
    }

Reference:

• https://ethdebug.github.io/solidity-data-representation

Enums are represented by integers; the possibility listed first by 0, the next by 1, and so forth. An enum type just acts like uintN, where N is the smallest legal value large enough to accomodate all the possibilities.

• https://docs.soliditylang.org/en/v0.8.17/internals/layout_in_storage.html#storage-inplace-encoding

Gas diff:

In red the old version, in green with the applied changes:

mint tokens to owner address..
{
  to: '0x0ed64d01D0B4B655E410EF1441dD677B695639E7',
  value: 0,
  data: '0xa9059cbb0000000000000000000000003c44cdddb6a900fa2b585dd299e03d12fa4293bc0000000000000000000000000000000000000000000000008ac7230489e80000',
  operation: 0,
  targetTxGas: 0,
  baseGas: 0,
  gasPrice: 0,
  tokenGasPriceFactor: 1,
  gasToken: '0x0000000000000000000000000000000000000000',
  refundReceiver: '0x0000000000000000000000000000000000000000',
  nonce: BigNumber { value: "0" }
}
- estimated gas to be used  102719
+ estimated gas to be used  102710
- Real txn gas used:  98875
+ Real txn gas used:  98866

targetTxGas estimation part 1:  36544
- estimated gas to be used  135491
+ estimated gas to be used  135482
{
-  baseGas: 103947,
+  baseGas: 103938,

Affected source code:

• BaseSmartAccount.sol:16

5. Use the unchecked keyword

When an underflow or overflow cannot occur, one might conserve gas by using the unchecked keyword to prevent unnecessary arithmetic underflow/overflow tests.

    function simulateValidation(UserOperation calldata userOp) external {
        uint256 preGas = gasleft();

        UserOpInfo memory outOpInfo;

        (address aggregator, uint256 deadline) = _validatePrepayment(0, userOp, outOpInfo, SIMULATE_FIND_AGGREGATOR);
        uint256 prefund = outOpInfo.prefund;
-       uint256 preOpGas = preGas - gasleft() + userOp.preVerificationGas;
+       uint256 preOpGas;
+       unchecked {
+           preOpGas = preGas - gasleft() + userOp.preVerificationGas;
+       }
        StakeInfo memory paymasterInfo = getStakeInfo(outOpInfo.mUserOp.paymaster);
        StakeInfo memory senderInfo = getStakeInfo(outOpInfo.mUserOp.sender);
        bytes calldata initCode = userOp.initCode;
        address factory = initCode.length >= 20 ? address(bytes20(initCode[0 : 20])) : address(0);
        StakeInfo memory factoryInfo = getStakeInfo(factory);

        if (aggregator != address(0)) {
            AggregatorStakeInfo memory aggregatorInfo = AggregatorStakeInfo(aggregator, getStakeInfo(aggregator));
            revert SimulationResultWithAggregation(preOpGas, prefund, deadline, senderInfo, factoryInfo, paymasterInfo, aggregatorInfo);
        }
        revert SimulationResult(preOpGas, prefund, deadline, senderInfo, factoryInfo, paymasterInfo);

    }

Gas diff:

In red the old version, in green with the applied changes:

        √ legacy mode (maxPriorityFee==maxFeePerGas) should not use "basefee" opcode
-    == est gas= 114321
+    == est gas= 114297
-   rcpt.gasUsed= 111704 0x914c39060be62a5980f0ffa05b56214fc3301dbc44b7ba872afd0753058b95ca
+   rcpt.gasUsed= 111680 0xa023ccb107b9dc80adcb3d031d48664146215e76e030839f0d8033f100d44faf
-   	== actual gasUsed (from tx receipt)= 111704
+       == actual gasUsed (from tx receipt)= 111680

      create account
        √ should reject if account not funded
-   == create gasUsed= 381974
+   == create gasUsed= 381962
-   == actual gasUsed (from tx receipt)= 381974
+	== actual gasUsed (from tx receipt)= 381962

- |  EntryPoint               ·  handleAggregatedOps         ·     163341  ·     345337  ·     245824  ·            6  ·          -  │
+ |  EntryPoint               ·  handleAggregatedOps         ·     163341  ·     345337  ·     245822  ·            6  ·          -  │
- |  EntryPoint               ·  handleOps                   ·     101633  ·     507641  ·     256369  ·           28  ·          -  │
+ |  EntryPoint               ·  handleOps                   ·     101597  ·     507653  ·     256364  ·           28  ·          -  │

Affected source code:

• EntryPoint.sol:233
• SmartAccount.sol:291
• SmartAccount.sol:372