Biconomy - Smart Contract Wallet contest - betweenETHlines's results

Lines of code

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

Vulnerability details

Impact

Every userOperation with valid initCode (wallet deployment) can be frontran which results in DoS

Proof of Concept

1. Alice provides a userOperation with the intention to deploy a proxy wallet for herself
2. Malicious attacker spots the transaction in the mempool and fetches the initCode
3. Malicious attacker either calls SenderCreator.createSender with the initcode, or directly SmartAccountFactory.deployCounterFactualWallet
4. This will result in a deployment of the desired proxy wallet
5. Alice's transaction gets executed but reverts during the desired wallet deployment:

require(address(proxy) != address(0), "Create2 call failed"); *The create2 call will return address(0) if the desired contract is already deployed.

6. This way, any userOperation with an initCode can be DoS'd

Tools Used

VSCode

Recommended Mitigation Steps

Currently, there is no easy way to minimize this issue, because the create2 call doesnt return any difference if the transaction actually failed because something went wrong, or if it just failed because the desired contract is already deployed.

One could consider removing the require(address(proxy) != address(0), "Create2 call failed"); requirement, however, this most likely will lead to other issues in the transaction flow one needs to think careful about.

Lines of code

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

Vulnerability details

Impact

execute and executeBatch are never callable by entryPoint

Proof of Concept

Currently, the execute and executeBatch functions are most probably meant to be called by entryPoint or owner:

_requireFromEntryPointOrOwner();

However, the onlyOwner modifier effectively prevents these functions from ever being called by entryPoint, limiting the contracts functionalities.

Tools Used

VSCode

Recommended Mitigation Steps

Consider removing the onlyOwner modifier from execute and executeBatch

Lines of code

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

Vulnerability details

Impact

The SmartAccount contract is used as a proxy implementation, however, it inherits non-upgradeable dependencies which could break upgradeability.

We are aware of the fact that the proxy -> implementation solution is mostly used for gas-savings, however, this fact is undeniable present and might occur in production.

Proof of Concept

1. Proxy and SmartAccount are deployed
2. Admin deploys a new SmartAccount with the same dependencies, but a one or a few dependencies have additional state variables
3. Admin upgrades the implementation, resulting in storage collision due to the additional state variables

Tools Used

VSCode

Recommended Mitigation Steps

Consider adding a uint256[49] private __gap; variable at the end of the state variable declarations to free up space for future variables.

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

2. Duplicate initialization check: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L168

The owner == address(0) check is sufficient.

3. name mismatch for pullTokens

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

The function should be named pushTokens because it doesn't execute a transferFrom but a (safe)transfer

4. SafeERC20 can be used as using SafeERC20 for IERC20`

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

Consider implementing the using syntax.

5. Enum already imported

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

It is not necessary to import Enum, since its already imported within Executor.

6. Missing comment for getModulesPaginated

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

The getModulesPaginated function works in descending order, however, this is not well documented. Users would expect this function to be working in ascending order.

Consider adding a comment to this function.

7. setFallbackHandler lacks a non-zero validation

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

Consider adding a non-zero validation for the function input.

8. _defaultImpl can never be changed

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

Consider removing the immutable keyword and add a setter function for the aforementioned variable for the case that the implementation needs to be changed.

9. Missing event for deployWallet

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

Consider adding an event for the aforementioned function.

10. Events for the Executor contract should have an indexed to parameter

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

Consider adding the indexed keyword to the to parameter.

11. Unused import: import "../utils/Exec.sol";

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L17

Consider removing the unused import.

12. General structure: `EntryPoint``

The EntryPoint contract is written in the bad structure e.g. internal before public functions, variable declarations in the middle of the contract etc.

This reduces readability for third-parties.

Consider refactoring the structure for the aforementioned contracts.

13. handleOps might run out of gas

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

The handleOps function loops twice over the UserOperations array, if this array ever becomes too large, the function will run out of gas.

Consider limiting the array to a reasonable length.

*The same applies for handleAggregatedOps

14. Missing non-zero validation for setEntryPoint

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

Consider adding a non-zero validation for this function.

15. Best practice: using Address for address

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L49

The contract should follow the best-practices and implement using Address for address to make the code cleaner and more readable.

16. [GLOBAL] Missing _disableInitializer

Currently, all implementation contracts lack the aforementioned function. It might make sense to call this function within the constructor to avoid an initialization of the implementation contract.