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

One-Stop solution to enable an effortless experience in your dApp to onboard new users and abstract away transaction complexities.

General Information

Platform: Code4rena

Start Date: 04/01/2023

Pot Size: $60,500 USDC

Total HM: 15

Participants: 105

Period: 5 days

Judge: gzeon

Total Solo HM: 1

Id: 200

League: ETH

Biconomy

Findings Distribution

Researcher Performance

Rank: 63/105

Findings: 2

Award: $75.26

QA:

grade-b

Gas:

grade-b

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Biconomy

Findings Information

🌟 Selected for report: 0xSmartContract

Also found by: 0x1f8b, 0xAgro, 0xdeadbeef0x, 0xhacksmithh, 2997ms, Atarpara, Bnke0x0, Diana, HE1M, IllIllI, Josiah, Kalzak, Lirios, MalfurionWhitehat, MyFDsYours, Raiders, RaymondFam, Rolezn, SaharDevep, Sathish9098, Udsen, Viktor_Cortess, adriro, ast3ros, betweenETHlines, btk, chaduke, chrisdior4, cryptostellar5, csanuragjain, giovannidisiena, gz627, hl_, horsefacts, joestakey, juancito, ladboy233, lukris02, nadin, oyc_109, pauliax, peanuts, prady, sorrynotsorry, zaskoh

Awards

36.5015 USDC - $36.50

Labels

bug

grade-b

QA (Quality Assurance)

sponsor confirmed

edited-by-warden

Q-54

External Links

Link to GitHub issue

[Low-01] Old Solidity

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

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.

[Low-02] Multiple Solidity version used

Below contracts using Solidity 0.8.12

File: BaseSmartAccount.sol
File: Proxy.sol
File: SmartAccount.sol
File: SmartAccountFactory.sol
File: interfaces/ISignatureValidator.sol
File: interfaces/ERC1155TokenReceiver.sol
File: interfaces/ERC721TokenReceiver.sol
File: interfaces/ERC777TokensRecipient.sol
File: interfaces/IERC1271Wallet.sol
File: common/Singleton.sol
File: common/SignatureDecoder.sol
File: base/Executor.sol
File: common/SecuredTokenTransfer.sol
File: base/ModuleManager.sol
File: base/FallbackManager.sol
File: interfaces/IERC165.sol
File: libs/Math.sol
File: libs/LibAddress.sol
File: paymasters/PaymasterHelpers.sol
File: paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol
File: common/Enum.sol
File: libs/MultiSend.sol
File: MultiSendCallOnly.sol

Below contracts using Solidity ^0.8.12

File: aa-4337/interfaces/IAccount.sol
File: aa-4337/core/EntryPoint.sol
File: aa-4337/core/SenderCreator.sol
File: aa-4337/core/StakeManager.sol
File: aa-4337/utils/Exec.sol
File: aa-4337/interfaces/IAggregator.sol
File: aa-4337/interfaces/UserOperation.sol
File: aa-4337/interfaces/IStakeManager.sol
File: aa-4337/interfaces/IEntryPoint.sol
File: aa-4337/interfaces/IAggregatedAccount.sol
File: aa-4337/interfaces/IPaymaster.sol
File: paymasters/BasePaymaster.sol

[Low-03] Contracts has payable function to receive ETH, but there is no rescue function to send out ETH from those contract

Instances(4)

File: BaseSmartAccount.sol

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

File: Proxy.sol

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

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

File: lib/MultiSendCallOnly.sol
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/libs/MultiSendCallOnly.sol#L21-L59

[Low-4] Changing Owner or setting any most important addresses should be a 2 Step-process

Instances(1)

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

[Low-5] Could some lines of code(checks) remove from function `init()`

As here is a initializer modifier that will help to initialize state variable only once through init()`` and ownerand_entryPoint``` by default holds default value.

I mean this function only callable a single time, so no need to checks for owner == address(0) and address(_entryPoint) == address(0) so these 2 line could removed.

    function init(address _owner, address _entryPointAddress, address _handler) public override initializer { 
        require(owner == address(0), "Already initialized");  // @audit no need to check
        require(address(_entryPoint) == address(0), "Already initialized");  // @audit no need to check
        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);
        setupModules(address(0), bytes(""));
    }

Instances(2)

File: SmartAccount.sol

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

[Low-6] Absence of signature length check that `signature` that pass as a function parameter before `v,r,s` derived from it

Instances(1)

File: common/SignatureDecoder.sol

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/common/SignatureDecoder.sol#L10-L34

[Low-07] While setting fallbackHandler via `setFallbackHandler()` missing of zero-address check for function input

Instances(1)

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

[Info-1] Events missing `indexed` key word that helpful in on-chain tracking

Instances(3)

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

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

[Info-2] Unused lines of comment, some comments that are perticularly for development not removed

Instances(4)

File: SmartAccount.sol

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

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

[Info-3] Absence of error msg from `require()` statement

Instances(1)

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

#0 - livingrockrises
2023-01-19T19:44:03Z

good quality report

#1 - livingrockrises
2023-01-19T19:53:57Z

[Low-03] Contracts has payable function to receive ETH, but there is no rescue function to send out ETH from those contract

there is a rescue function in the smart account. it's called transfer. execTransaction() a value can be passed in _tx

#2 - c4-sponsor
2023-01-19T19:59:20Z

livingrockrises marked the issue as sponsor confirmed

#3 - c4-judge
2023-01-22T15:12:23Z

gzeon-c4 marked the issue as grade-b

Findings Information

🌟 Selected for report: 0xSmartContract

Also found by: 0x1f8b, 0xhacksmithh, Aymen0909, Bnke0x0, IllIllI, Rageur, RaymondFam, Rickard, Rolezn, Secureverse, arialblack14, chaduke, chrisdior4, cthulhu_cult, giovannidisiena, gz627, lukris02, oyc_109, pavankv, privateconstant, shark

Awards

38.7634 USDC - $38.76

Labels

bug

G (Gas Optimization)

grade-b

sponsor confirmed

edited-by-warden

G-22

External Links

Link to GitHub issue

[Gas-01] Should Use `require()` Instead of `assert()`

when assert() executed, it revert all state variable change and consume all remaining gas when require() executed, it reverts all state variable change and return all remaining gas to caller

From gas saving perspective its recommended to use require() instead of assert()

Instances(2)

File: Proxy.sol

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

File: common/Singleton.sol
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/common/Singleton.sol#L13

[Gas-02] `onlyOwner` functions could make as `payable`

Instances(10)

File: SmartAccount.sol

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L449-L453
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L455-L458
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L460-L463
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L465-L473
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L536-L538

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

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

[Gas-03] Instead of using && Operator in `require()` by splitting them can save gas

Instances(3)

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

[Gas-04] Arithmatic Operation could be `uncheck` which will never goes `Underflow`/`Overflow`

Instances(1)

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

[Gas-05] Instead `Divided by 2`, `Bit Shift` could more gas efficient

Instances(1)

File: libs/Math.sol
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/libs/Math.sol#L36

[Gas-06] Instead `<x>+=<y>`, `<x>=<x>+<y>` could more gas efficient

Instances(13)

File: libs/Math.sol
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/libs/Math.sol#L148

File: aa-4337/core/EntryPoint.sol
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L81
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L101
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L107
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L112
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L114
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L128
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L134
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L135
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L136
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L468

File: paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol

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

[Gas-07] `abi.encodePacked()` more efficient than `abi.encode()`

return abi.encode(data.paymasterId);

Should replace with

return abi.encodePacked(data.paymasterId);

Instances(1)

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

#0 - livingrockrises
2023-01-19T19:01:22Z

[Gas-06] Instead <x>+=<y>, <x>=<x>+<y> could more gas efficient

not sure if this true.

for gas optimisation, it would be great if wardens can also give gas savings estimates

#1 - livingrockrises
2023-01-19T19:09:28Z

[Gas-02] onlyOwner functions could make as payable

not sure about the purpose of this

#2 - livingrockrises
2023-01-19T19:16:16Z

https://ethereum.stackexchange.com/questions/74539/difference-between-a-addb-and-a-b-as-well-as-a-ab

#3 - c4-sponsor
2023-01-19T19:39:12Z

livingrockrises marked the issue as sponsor confirmed

#4 - c4-judge
2023-01-22T16:25:57Z

gzeon-c4 marked the issue as grade-b

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

One-Stop solution to enable an effortless experience in your dApp to onboard new users and abstract away transaction complexities.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-54] QA Report - $36.50

Findings Information

Awards

Labels

External Links

[Low-01] Old Solidity

[Low-02] Multiple Solidity version used

[Low-03] Contracts has payable function to receive ETH, but there is no rescue function to send out ETH from those contract

[Low-4] Changing Owner or setting any most important addresses should be a 2 Step-process

[Low-5] Could some lines of code(checks) remove from function init()

[Low-6] Absence of signature length check that signature that pass as a function parameter before v,r,s derived from it

[Low-07] While setting fallbackHandler via setFallbackHandler() missing of zero-address check for function input

[Info-1] Events missing indexed key word that helpful in on-chain tracking

[Info-2] Unused lines of comment, some comments that are perticularly for development not removed

[Info-3] Absence of error msg from require() statement

#0 - livingrockrises2023-01-19T19:44:03Z

#1 - livingrockrises2023-01-19T19:53:57Z

#2 - c4-sponsor2023-01-19T19:59:20Z

#3 - c4-judge2023-01-22T15:12:23Z

[G-22] Gas Report - $38.76

Findings Information

Awards

Labels

External Links

[Gas-01] Should Use require() Instead of assert()

[Gas-02] onlyOwner functions could make as payable

[Gas-03] Instead of using && Operator in require() by splitting them can save gas

[Gas-04] Arithmatic Operation could be uncheck which will never goes Underflow/Overflow

[Gas-05] Instead Divided by 2, Bit Shift could more gas efficient

[Gas-06] Instead <x>+=<y>, <x>=<x>+<y> could more gas efficient

[Gas-07] abi.encodePacked() more efficient than abi.encode()

#0 - livingrockrises2023-01-19T19:01:22Z

#1 - livingrockrises2023-01-19T19:09:28Z

#2 - livingrockrises2023-01-19T19:16:16Z

#3 - c4-sponsor2023-01-19T19:39:12Z

#4 - c4-judge2023-01-22T16:25:57Z

[Low-5] Could some lines of code(checks) remove from function `init()`

[Low-6] Absence of signature length check that `signature` that pass as a function parameter before `v,r,s` derived from it

[Low-07] While setting fallbackHandler via `setFallbackHandler()` missing of zero-address check for function input

[Info-1] Events missing `indexed` key word that helpful in on-chain tracking

[Info-3] Absence of error msg from `require()` statement

#0 - livingrockrises
2023-01-19T19:44:03Z

#1 - livingrockrises
2023-01-19T19:53:57Z

#2 - c4-sponsor
2023-01-19T19:59:20Z

#3 - c4-judge
2023-01-22T15:12:23Z

[Gas-01] Should Use `require()` Instead of `assert()`

[Gas-02] `onlyOwner` functions could make as `payable`

[Gas-03] Instead of using && Operator in `require()` by splitting them can save gas

[Gas-04] Arithmatic Operation could be `uncheck` which will never goes `Underflow`/`Overflow`

[Gas-05] Instead `Divided by 2`, `Bit Shift` could more gas efficient

[Gas-06] Instead `<x>+=<y>`, `<x>=<x>+<y>` could more gas efficient

[Gas-07] `abi.encodePacked()` more efficient than `abi.encode()`

#0 - livingrockrises
2023-01-19T19:01:22Z

#1 - livingrockrises
2023-01-19T19:09:28Z

#2 - livingrockrises
2023-01-19T19:16:16Z

#3 - c4-sponsor
2023-01-19T19:39:12Z

#4 - c4-judge
2023-01-22T16:25:57Z