Biconomy - Smart Contract Wallet contest - RaymondFam's results

Modularity on import usages

For cleaner Solidity code in conjunction with the rule of modularity and modular programming, use named imports with curly braces instead of adopting the global import approach.

For instance, the import instances below could be refactored as follows:

File: ModuleManager.sol#L4-L6

- import "../common/Enum.sol";
- import "../common/SelfAuthorized.sol";
- import "./Executor.sol";
+ import {Enum} from "../common/Enum.sol";
+ import {SelfAuthorized} from "../common/SelfAuthorized.sol";
+ import {Executor} from "./Executor.sol";

Expression for Hashed Values

Long bytes of literal values assigned to constants may incur typo/human error. In fact, it was validated in the constructor of Proxy.sol and Singleton.sol using assert() to compare the hashed constant with its bytes32() result to avoid this error:

File: Singleton.sol#L13 File: Proxy.sol#L16

         assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("biconomy.scw.proxy.implementation")) - 1));

As such, consider at least assigning these constants with their corresponding bytes32() expression where possible. For instance, the instance below could have its code line refactored as follows:

File: SmartAccount.sol#L42-L48

-    bytes32 internal constant DOMAIN_SEPARATOR_TYPEHASH = 0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218;
+    bytes32 internal constant DOMAIN_SEPARATOR_TYPEHASH = keccak256("EIP712Domain(uint256 chainId,address verifyingContract)");

Or, better yet, use assert() to make it absolute error free in the constructor.

Missing tokenGasPriceFactor

In encodeTransactionData() of SmartAccount.sol, refundInfo.tokenGasPriceFactor is missing in keccak256(abi.encode()). Consider commenting the omission adopted for this particular variable where possible for more distinct code readability.

File: SmartAccount.sol#L430-L444

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

Open TODOs

Open TODOs can point to architecture or programming issues that still need to be resolved. Consider resolving them before deploying.

Here is the one instance entailed:

File: EntryPoint.sol#L255

        // TODO: copy logic of gasPrice?

Empty blocks

Function with empty block should have a comment explaining why it is empty, or an event emitted.

For instance, the fallback instance may be refactored as follows just as it has been done in Proxy.sol:

File: SmartAccount.sol#L550

+ event Received(uint indexed value, address indexed sender, bytes data);

-    receive() external payable {}
+    receive() external payable {
+        emit Received(msg.value, msg.sender, "");
+    }

Library not adequately utilized

Library functions in Math.sol should be imported and fully made used of in the protocol instead of being re-implemented in the contracts.

Here are the max() and min() instances entailed:

File: SmartAccount.sol

224:        require(gasleft() >= max((_tx.targetTxGas * 64) / 63,_tx.targetTxGas + 2500) + 500, "BSA010");

181:    function max(uint256 a, uint256 b) internal pure returns (uint256) {
182:        return a >= b ? a : b;
183:    }

File: UserOperation.sol

53:        return min(maxFeePerGas, maxPriorityFeePerGas + block.basefee);

77:    function min(uint256 a, uint256 b) internal pure returns (uint256) {
78:        return a < b ? a : b;
79:    }

Missing checks for contract existence

Performing a low-level calls without confirming contract’s existence (not yet deployed or have been destructed which could still be a non-zero address) could return success even though no function call was executed as documented in the link below:

https://docs.soliditylang.org/en/v0.8.7/control-structures.html#error-handling-assert-require-revert-and-exceptions

Consider check for target contract existence before call.

Here is an instance entailed that could be refactored by making use of LibAddress.sol that has already been imported to the contract:

File: SmartAccount.sol#L449-L453

    function transfer(address payable dest, uint amount) external nonReentrant onlyOwner {
        require(dest != address(0), "this action will burn your funds");
+        require(dest.isContract(), "INVALID_IMPLEMENTATION");
        (bool success,) = dest.call{value:amount}("");
        require(success,"transfer failed");
    }

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Here is a contract instance with missing NatSpec in its entirety:

File: ERC777TokensRecipient.sol

checkSignatures() missing @param data:

File: SmartAccount.sol#L297-L306

Commented codes

Throughout the code base, there are several commented code lines that could point to items that are not done or need redesigning, be a mistake, debugging or just be testing overhead. Consider removing them before deployment for readability and conciseness.

Here are some of the instances entailed:

File: SmartAccount.sol#L68-L69

    // nice to have
    // event SmartAccountInitialized(IEntryPoint indexed entryPoint, address indexed owner);

File: SmartAccount.sol#L201

        //console.log("init %s", 21000 + msg.data.length * 8);

File: SmartAccountFactory.sol#L22

    // event SmartAccountCreated(address indexed _proxy, address indexed _implementation, address indexed _owner);

Use delete to clear variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address or a boolean to false. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic.

For instance, the a[x] instance below may be refactored as follows:

File: ModuleManager.sol#L52

-        modules[module] = address(0);
+        delete modules[module];

uint256 over uint

Across the code base, there are numerous instances of uint, as opposed to uint256. In favor of explicitness, consider replacing all instances of uint with uint256.

Here are some of the instances entailed:

File: SmartAccountFactory.sol

35:        bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));

54:        bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));

69:       bytes memory code = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));

Inconsistency in interface naming

Some interfaces in the code bases are named without the prefix I that could cause confusion to developers and readers referencing or interacting with the protocol. Consider conforming to Solidity's naming conventions by having the instances below refactored as follow:

File: ERC1155TokenReceiver.sol#L7

- interface ERC1155TokenReceiver {
+ interface IERC1155TokenReceiver {

File: ERC721TokenReceiver.sol#L5

- interface ERC721TokenReceiver {
+ interface IERC721TokenReceiver {

File: ERC777TokensRecipient.sol#L4

- interface ERC777TokensRecipient {
+ interface IERC777TokensRecipient {

Non-compliant contract layout with Solidity's Style Guide

According to Solidity's Style Guide below:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:

constructor, receive function (if exists), fallback function (if exists), external, public, internal, private

And, within a grouping, place the view and pure functions last.

Additionally, inside each contract, library or interface, use the following order:

type declarations, state variables, events, modifiers, functions

Consider adhering to the above guidelines for all contract instances entailed.

Contracts should be imported

In ISignatureValidator.sol, the contract ISignatureValidatorConstants is showing up in its entirety at the top of the abstract contract ISignatureValidator which facilitates ease of references on the same file page.

Additionally, both the contract and the abstract contract have been named with the prefix I that is generally (if not exclusively) reserved for interfaces.

Consider:

1. having their names renamed as follows:

File: ISignatureValidator.sol

// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity 0.8.12;

- contract ISignatureValidatorConstants {
+ contract SignatureValidatorConstants {
    // bytes4(keccak256("isValidSignature(bytes,bytes)")
    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;
}

- abstract contract ISignatureValidator is ISignatureValidatorConstants {
+ abstract contract SignatureValidator is ISignatureValidatorConstants {
    /**
     * @dev Should return whether the signature provided is valid for the provided data
     * @param _data Arbitrary length data signed on the behalf of address(this)
     * @param _signature Signature byte array associated with _data
     *
     * MUST return the bytes4 magic value 0x20c13b0b when function passes.
     * MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5)
     * MUST allow external calls
     */
    function isValidSignature(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4);
}

2. saving the contract entailed into SignatureValidatorConstants.sol, and have it imported like it has been done in all other contracts or abstract contracts.

Typo mistakes

File: ISignatureValidator.sol#L12

-     * @param _data Arbitrary length data signed on the behalf of address(this)
+     * @param _data Arbitrary length data signed on behalf of address(this)

File: SmartAccount.sol#L44

-    // review? if rename wallet to account is must
+    // review? if renaming wallet to account is a must

File: SmartAccount.sol#L74

-     * @notice Throws if the sender is not an the owner.
+     * @notice Throws if the sender is not the owner.

File: SmartAccount.sol#L153

-     * @return nonce : the number of transaction made within said batch
+     * @return nonce : the number of transactions made within said batch

File: SignatureDecoder.sol#L30-L31

-            // 'byte' is not working due to the Solidity parser, so lets
+            // 'byte' is not working due to the Solidity parser, so let's
            // use the second best option, 'and'

File: SmartAccount.sol#L320

-                // This check is not completely accurate, since it is possible that more signatures than the threshold are send.
+                // This check is not completely accurate, since it is possible that more signatures than the threshold are sent.

File: SmartAccount.sol#L382

-    /// @param targetTxGas Fas that should be used for the safe transaction.
+    /// @param targetTxGas Gas that should be used for the safe transaction.

File: SignatureDecoder.sol#L4

- /// @title SignatureDecoder - Decodes signatures that a encoded as bytes
+ /// @title SignatureDecoder - Decodes signatures that are encoded as bytes

File: IERC1271Wallet.sol#L12

-   * @param _data       Arbitrary length data signed on the behalf of address(this)
+   * @param _data       Arbitrary length data signed on behalf of address(this)

address(this) over this

As denoted in Solidity Documentation:

"Prior to version 0.5.0, Solidity allowed address members to be accessed by a contract instance, for example this.balance. This is now forbidden and an explicit conversion to address must be done: address(this).balance."

Here is an instance entailed:

File: SmartAccount.sol#L136

-        return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, getChainId(), this));
+        return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, getChainId(), address(this)));

Lack of events for critical operations

Critical operations not triggering events will make it difficult to review the correct behavior of the deployed contracts. Users and blockchain monitoring systems will not be able to detect suspicious behaviors at ease without events. Consider adding events where appropriate for all critical operations for better support of off-chain logging API.

Here is an instance entailed:

File: VerifyingSingletonPaymaster.sol#L65-L68

    function setSigner( address _newVerifyingSigner) external onlyOwner{
        require(_newVerifyingSigner != address(0), "VerifyingPaymaster: new signer can not be zero address");
        verifyingSigner = _newVerifyingSigner;
    }

Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier instance below may be refactored as follows just as it has been implemented in SelfAuthorized.sol:

File: SmartAccount.sol#L82-L85

+    function _mixedAuth() private view {
+        require(msg.sender == owner || msg.sender == address(this),"Only owner or self");
+    }

   modifier mixedAuth {
-        require(msg.sender == owner || msg.sender == address(this),"Only owner or self");
+        _mixedAuth();
       _;
   }

Unneeded uint256() cast

Casting an unsigned integer to uint256() is unnecessary.

For instance, the code line below may be refactored to save gas both on contract deployment and function call as follows:

File: SmartAccount.sol#L322

-              require(uint256(s) >= uint256(1) * 65, "BSA021");
+              require(uint256(s) >= 65, "BSA021");

Note: Since 1 * 65 == 65, replacing uint256(1) * 65 with 65 instead of 1 * 65 saves even more gas.

Use of named returns for local variables saves gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: SmartAccountNoAuth.sol#L155-L159

    function getNonce(uint256 batchId)
    public view
-    returns (uint256) {
+    returns (uint256 _2dNonce) {
-        return nonces[batchId];
+        _2dNonce = nonces[batchId];
    }

Check logic that could lead to waste of gas

In ModuleManager.sol, getModulesPaginated() has currentModule != address(0x0) included in the while statement. If start was inputted as a non-existent key, (currentModule == address(0x0). This would skip the while loop and move on to executing the rest of the code lines while irrelevantly and meaninglessly returning the output.

Consider separating the zero address check by having the function refactored as follows so that a revert could stem unneeded code executions coming after it:

File: ModuleManager.sol#L114-L132

    function getModulesPaginated(address start, uint256 pageSize) external view returns (address[] memory array, address next) {
        // Init array with max page size
        array = new address[](pageSize);

        // Populate return array
        uint256 moduleCount = 0;
        address currentModule = modules[start];
+        require (currentModule != address(0x0), "BSA105"); // change to a different revert message where deemed fit
-        while (currentModule != address(0x0) && currentModule != SENTINEL_MODULES && moduleCount < pageSize) {
+        while (currentModule != SENTINEL_MODULES && moduleCount < pageSize) {
            array[moduleCount] = currentModule;
            currentModule = modules[currentModule];
            moduleCount++;
        }
        next = currentModule;
        // Set correct size of returned array
        // solhint-disable-next-line no-inline-assembly
        assembly {
            mstore(array, moduleCount)
        }
    }

Payable access control functions costs less gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

For instance, the code line below may be refactored as follows:

File: SmartAccount.sol#L449

-    function transfer(address payable dest, uint amount) external nonReentrant onlyOwner {
+    function transfer(address payable dest, uint amount) external payable nonReentrant onlyOwner {

Redundant checks

init() in SmartAccount.sol has _handler != address(0) unnecessarily included twice in its function logic. Consider removing the second identical check to save gas both on contract deployment and function calls:

File: SmartAccount.sol#L166-L176

    function init(address _owner, address _entryPointAddress, address _handler) public override initializer { 
        require(owner == address(0), "Already initialized");
        require(address(_entryPoint) == address(0), "Already initialized");
        require(_owner != address(0),"Invalid owner");
        require(_entryPointAddress != address(0), "Invalid Entrypoint");
        require(_handler != address(0), "Invalid Entrypoint");
        owner = _owner;
        _entryPoint =  IEntryPoint(payable(_entryPointAddress));
-        if (_handler != address(0)) internalSetFallbackHandler(_handler);
+        internalSetFallbackHandler(_handler);
        setupModules(address(0), bytes(""));
    }

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, consider replacing >= with the strict counterpart > in the following inequality instance:

File: SmartAccount.sol#L322

-              require(uint256(s) >= uint256(1) * 65, "BSA021");
+              require(uint256(s) > uint256(1) * 65 - 1, "BSA021");

Early checks

In BaseSmartAccount.sol, (missingAccountFunds != 0) should be prepended before _payPrefund() is invoked in validateUserOp() instead of being included in _payPrefund(). This will make the external function call revert earlier to save some gas if need be.

File: BaseSmartAccount.sol#L60-L68

    function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, address aggregator, uint256 missingAccountFunds)
    external override virtual returns (uint256 deadline) {
        _requireFromEntryPoint();
        deadline = _validateSignature(userOp, userOpHash, aggregator);
        if (userOp.initCode.length == 0) {
            _validateAndUpdateNonce(userOp);
        }
+        if (missingAccountFunds != 0) {
            _payPrefund(missingAccountFunds);
+        }
    }

File: BaseSmartAccount.sol#L106-L112

    function _payPrefund(uint256 missingAccountFunds) internal virtual {
-        if (missingAccountFunds != 0) {
            (bool success,) = payable(msg.sender).call{value : missingAccountFunds, gas : type(uint256).max}("");
-            (success);
            //ignore failure (its EntryPoint's job to verify, not account.)
-        }
    }

Since success isn't going to be checked, it may also be removed from the code block to save gas both on contract deployment and function call.

Unchecked SafeMath saves gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ... } to use the previous wrapping behavior further saves gas just as in the instance below as an example:

File: SmartAccount.sol#L216

+    unchecked {
            nonces[batchId]++;
+    }

Inline over internal

Internal function entailing only one line of code may be embedded in the calling function(s) to save gas.

Here are some of the instances entailed:

File: SmartAccount.sol

461:        _requireFromEntryPointOrOwner();

466:        _requireFromEntryPointOrOwner();

494:    function _requireFromEntryPointOrOwner() internal view {
495:        require(msg.sender == address(entryPoint()) || msg.sender == owner, "account: not Owner or EntryPoint");
496:    }

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.12",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Split require statements using &&

Instead of using the && operator in a single require statement to check multiple conditions, using multiple require statements with 1 condition per require statement will save 3 GAS per &&.

Here are some of the instances entailed:

File: ModuleManager.sol

34:        require(module != address(0) && module != SENTINEL_MODULES, "BSA101");

49:        require(module != address(0) && module != SENTINEL_MODULES, "BSA101");

68:        require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "BSA104");

Unneeded zero address check

Consider removing module != address(0) since modules[prevModule] == module is going to revert if module == address(0) to save gas both on contract deployment and function calls:

File: ModuleManager.sol#L47-L54

    function disableModule(address prevModule, address module) public authorized {
        // Validate module address and check that it corresponds to module index.
-        require(module != address(0) && module != SENTINEL_MODULES, "BSA101");
+        require(module != SENTINEL_MODULES, "BSA101");
        require(modules[prevModule] == module, "BSA103");
        modules[prevModule] = modules[module];
        modules[module] = address(0);
        emit DisabledModule(module);
    }

Use storage instead of memory for structs/arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

Here are some of the instances entailed:

File: EntryPoint.sol

171:        MemoryUserOp memory mUserOp = opInfo.mUserOp;

229:        UserOpInfo memory outOpInfo;

234:        StakeInfo memory paymasterInfo = getStakeInfo(outOpInfo.mUserOp.paymaster);
235:        StakeInfo memory senderInfo = getStakeInfo(outOpInfo.mUserOp.sender);

238:        StakeInfo memory factoryInfo = getStakeInfo(factory);

241:            AggregatorStakeInfo memory aggregatorInfo = AggregatorStakeInfo(aggregator, getStakeInfo(aggregator));

293:        MemoryUserOp memory mUserOp = opInfo.mUserOp;

351:        MemoryUserOp memory mUserOp = opInfo.mUserOp;

359:        MemoryUserOp memory mUserOp = outOpInfo.mUserOp;

444:        MemoryUserOp memory mUserOp = opInfo.mUserOp;

+= and -= cost more gas

+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.

For instance, the += instance below may be refactored as follows:

File: EntryPoint.sol#L81

-            collected += _executeUserOp(i, ops[i], opInfos[i]);
+            collected = collected + _executeUserOp(i, ops[i], opInfos[i]);