Biconomy - Smart Contract Wallet contest - btk's results

Total L issues

Total NC issues

[L-1] Low level calls with solidity version 0.8.14 and lower can result in optimiser bug

The protocol is using low level calls with solidity version 0.8.12 which can result in optimizer bug.

Ref: https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d

Recommended Mitigation Steps

Consider upgrading to solidity 0.8.17

[L-2] Critical changes should use-two step procedure

The following contract have a function that allows them an owner to change it to a different address. If the owner accidentally uses an invalid address for which they do not have the private key, then the system will gets locked.

Lines of code

• SmartAccount.sol:109

Recommended Mitigation Steps

Consider adding two step procedure on the critical functions where the first is announcing a pending new owner and the new address should then claim its ownership.

A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

[L-3] initialize() function can be called by anyone

initialize() function can be called anyone when the contract is not initialized

Lines of code

• SmartAccount.sol:166
• SimpleAccount.sol:80

Recommended Mitigation Steps

Add a deployer address and require that only him can call the initialize() function.

[L-4] Avoid using tx.origin

tx.origin is a global variable in Solidity that returns the address of the account that sent the transaction.

Using the variable could make a contract vulnerable if an authorized account calls a malicious contract. You can impersonate a user using a third party contract.

Lines of code

• SmartAccount.sol:257
• SmartAccount.sol:281
• SmartAccount.sol:511
• SimpleAccount.sol:113

[NC-1] 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.

• EntryPoint.sol:255

[NC-2] Include return parameters in NatSpec comments

If Return parameters are declared, you must prefix them with /// @return. Some code analysis programs do analysis by reading NatSpec details, if they can't see the @return tag, they do incomplete analysis.

[NC-3] Non-usage of specific imports

The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly.

• https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files

Recommended Mitigation Steps

Use specific imports syntax per solidity docs recommendation.

[NC-4] Lines are too long

Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length.

Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length

Lines of code

• SmartAccount.sol:46
• SmartAccount.sol:186
• SmartAccount.sol:239
• SmartAccount.sol:346
• SmartAccount.sol:356
• SmartAccount.sol:357
• SmartAccount.sol:489
• EntryPoint.sol:168
• EntryPoint.sol:289
• EntryPoint.sol:319
• /EntryPoint.sol:349
• EntryPoint.sol:363
• EntryPoint.sol:401
• EntryPoint.sol:409
• EntryPoint.sol:440

[NC-5] Use bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() vs abi.encodePacked(<bytes>,<bytes>)

Lines of code

• SmartAccount.sol:294
• SmartAccount.sol:347
• SmartAccount.sol:374
• SmartAccount.sol:445
• SmartAccountFactory.sol:34
• SmartAccountFactory.sol:35
• SmartAccountFactory.sol:54
• SmartAccountFactory.sol:69
• SmartAccountFactory.sol:70
• SmartAccountFactory.sol:71

Recommended Mitigation Steps

Use bytes.concat()

[NC-6] Use require instead of assert

Assert should not be used except for tests, require should be used

Prior to Solidity 0.8.0, pressing a confirm consumes the remainder of the process's available gas instead of returning it, as request()/revert() did.

The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.

Meanwhile, a require() statement when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.

Assertion should be avoided even after solidity version 0.8.0, because its documentation states "The Assert function generates an error of type Panic(uint256). Code that works properly should never Panic, even on invalid external input. If this happens, you need to fix it in your contract. there's a mistake".

Lines of code

• Singleton.sol:13
• Proxy.sol:16

Recommended Mitigation Steps

Use require() instead of assert()

[NC-7] Typos

• SmartAccount.sol:74

Fix:

    /**
     * @notice Throws if the sender is not the owner.
     */

[NC-8] Lack of event emit

The below methods do not emit an event when the state changes, something that it's very important for dApps and users.

• BasePaymaster.sol:24
• VerifyingSingletonPaymaster.sol:65

[NC-9] Require messages are too short and unclear

The correct and clear error description explains to the user why the function reverts, but the error descriptions below in the project are not self-explanatory. These error descriptions are very important in the debug features of DApps like Tenderly. Error definitions should be added to the require block, not exceeding 32 bytes.

• SmartAccount.sol:371
• SmartAccount.sol:528
• BasePaymaster.sol:105
• SimpleAccount.sol:139
• Math.sol:78

[NC-10] Unused receive() Function Will Lock Ether In Contract

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert.

Lines of code

• SmartAccount.sol:550
• SimpleAccount.sol:41

Recommended Mitigation Steps

The function should call another function, otherwise it should revert.