Biconomy - Smart Contract Wallet contest - ast3ros'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: 52/105

Findings: 3

Award: $85.48

QA:

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: adriro

Also found by: 0x1f8b, 0x73696d616f, 0xdeadbeef0x, BClabs, HE1M, Haipls, Jayus, Kalzak, Lirios, Qeew, V_B, adriro, ast3ros, aviggiano, betweenETHlines, bin2chen, chaduke, dragotanqueray, ey88, giovannidisiena, hihen, horsefacts, ladboy233, wait, zaskoh

Awards

26.2582 USDC - $26.26

Labels

bug

3 (High Risk)

satisfactory

sponsor confirmed

duplicate-460

External Links

Link to GitHub issue

Lines of code

Vulnerability details

Impact

An attacker could front-runt the owner to call the deployCounterFactualWallet in SmartAccountFactory and change the address of the _handler to a malicious contract.

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

He can do it because the Smart Account wallet address is determined by the owner address and the index, therefore the attacker could call the deployCounterFactualWallet function and create the smart account wallet for the owner. The Smart Account contract only check if the handler address is not address(0)

With the malicious fallback handler function, the attacker can write a fallback function to transfer all the fund from Smart Account wallet to attacker. It leads to user fund being lost.

Proof of Concept

Alice deploys a counter factual wallet with deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index).
Bob can watch the above tx in the mempool and frontrun it changing the values of _handler to malicious contract. (Bob cannot change _owner or _index because it will create a different wallet address, he cannot change _entryPoint because the wrong entryPoint address will be immediately identified when entryPoint contract calls the wallet. Changing only the fallbackHandler address will be difficult to identified).
Alice's deployCounterFactualWallet reverts.
The event SmartAccountCreated(proxy,_defaultImpl,_owner, VERSION, _index) will be emit as normal, and in front-end user will be notified that the account is created successfully.
Bob waits for balance of Smart Account increased then call the fallback function to get all the Alice fund.

Tools Used

Manual

Recommended Mitigation Steps

There are many ways to mitigate this risk. One way can be:
- The Smart Account could use a constructor to assign owner = _owner. After that the init function to setup _entryPoint, _handler could only be called by onwer.
- Event SmartAccountCreated also include _handler address for off-chain monitoring for strange address.

#0 - c4-judge
2023-01-17T07:47:49Z

gzeon-c4 marked the issue as duplicate of #460

#1 - c4-sponsor
2023-01-26T06:29:02Z

livingrockrises marked the issue as sponsor confirmed

#2 - livingrockrises
2023-01-26T06:29:58Z

"The Smart Account could use a constructor to assign owner = _owner. After that the init function to setup _entryPoint, _handler could only be called by onwer." this is not valid

#3 - c4-judge
2023-02-10T12:25:30Z

gzeon-c4 marked the issue as satisfactory

Findings Information

🌟 Selected for report: 0xdeadbeef0x

Also found by: Atarpara, BClabs, Fon, Haipls, Koolex, Lirios, Manboy, StErMi, Tricko, V_B, adriro, ast3ros, ayeslick, bin2chen, csanuragjain, gogo, hihen, immeas, kankodu, ladboy233, ro, romand, static, wait, wallstreetvilkas

Awards

22.7235 USDC - $22.72

Labels

bug

3 (High Risk)

satisfactory

sponsor confirmed

duplicate-175

External Links

Link to GitHub issue

Lines of code

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

Vulnerability details

Impact

An Attack contract can call execTransaction function and transfer funds to any addresses. He can do it because in case v is 0 (a contract signature), when the signature is checked in the checkSignatures function, it only verifies whether the contract signs the transaction without verifying that the contract is the owner of the Smart Account.

Therefore, the external contract does not have to be the owner of the contract to execute a transaction from the Smart Account. It leads to user fund being lost.

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

Proof of Concept

The Attacker writes a Attack smart contract following EIP-1271 with isValidSignature signature function. The function always returns MAGICVALUE.
The Attacker writes a transaction to transfer all the funds from targeted SmartAccount.
The Attacker call execTransaction.
The execTransaction function is executed, the checkSignatures calls ISignatureValidator(_signer).isValidSignature in the Attack contract and get passed.
The Attack smart contract will execute malicious transaction to draw all the fund from Smart Account.

Tools Used

Manual

Recommended Mitigation Steps

Implement checking if the contract that call execTransaction is the owner of SmartAccount. require(_signer == owner, "INVALID_SIGNATURE");

Example could be seen in https://github.com/safe-global/safe-contracts/blob/821d5fbdc2a4e7776d66c9f232b000b81e60bffc/contracts/GnosisSafe.sol#L301

#0 - c4-judge
2023-01-17T07:17:52Z

gzeon-c4 marked the issue as duplicate of #175

#1 - c4-sponsor
2023-01-26T00:19:43Z

livingrockrises marked the issue as sponsor confirmed

#2 - c4-judge
2023-02-10T12:28:27Z

gzeon-c4 marked the issue as satisfactory

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

Q-28

External Links

Link to GitHub issue

[L-1] Unspecific compiler version pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an different/outdated compiler version that might introduce bugs that affect the contract system negatively.

Instance:

Recommended Mitigation Steps:

Use fixed solidity version

[L-2] Input Address is not checked

The address of the receiver/withdrawer token need to be checked with address(0) to avoid locking fund.

Instance

[L-3] Missing Events on State Changing Functions and critical functions:

Events are used by off-chain participants to track on-chain state changes. There are several functions that don't emit events:

Instance

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L53 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L525 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L166 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L449 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol#L106 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L65 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L119

No events on execution success or failure https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L192

Recommended Mitigation Steps

Add events to all critical functions and important state change to help monitor off chain and alert.

[L-4] Critical address chang should use two-step procedure

Instance

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

Recommended Mitigation Steps

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

[L-5] Owner can be set by the contract itself

Owner should not be set by anyone but the old owner.

Instance

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

Recommended Mitigation Steps

Change the modifier to onlyOwner

[L-6] Change names and implement struct in EntryPoint contract to follow EIP4337

Naming and struct should follow EIP4337 interfaces.

Instance

Change from SimulationResult to ValidationResult https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/interfaces/IEntryPoint.sol#L123

Change from SimulationResult to ValidationResultWithAggregation https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/interfaces/IEntryPoint.sol#L138

ReturnInfo Struct is missing

Recommended Mitigation Steps

Interface in EIP4337 could be seen here: https://github.com/ethereum/EIPs/blob/8061f8e2243eaae829d1fa91f7a763c889aca371/EIPS/eip-4337.md?plain=1#L87 https://github.com/ethereum/EIPs/blob/8061f8e2243eaae829d1fa91f7a763c889aca371/EIPS/eip-4337.md?plain=1#L90 https://github.com/ethereum/EIPs/blob/8061f8e2243eaae829d1fa91f7a763c889aca371/EIPS/eip-4337.md?plain=1#L94-L101

[NC-1] Remove deadcode and dead comments

Dead code:

Dead comments:

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L242-L243 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L267-L268 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L292-L293 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L68-L69 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L44 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L10

[NC-2] Event is missing indexed fields

Instance:

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/interfaces/IEntryPoint.sol#L51

[NC-3] Wrong error message

The message should be Invalid FallbackHandler instead of Invalid Entrypoint

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

[NC-4] Code duplication

Create an internal function to avoid code duplication and keep code clean.

[NC-5] Missing natspecs in many functions, or natspecs without parameter explaination

[NC-6] require() / revert() statements should have descriptive reason strings

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

[NC-7] Add function simulateHandleOp to simulate full execution of a UserOperation (including both validation and target execution)

See implementation: https://github.com/eth-infinitism/account-abstraction/blob/6c6c5dc76526046474cc1bad5467e756ee0bb82f/contracts/core/EntryPoint.sol#L167-L177

[NC-8] Function order does not follow Solidity style guides

In the style guides, the order is: constructor -> receive function (if exists) -> fallback function (if exists) -> external -> public -> internal -> private. There are many instances that order of functions is mixed and it leads to the readability of contracts.

Receive function move below init function https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L550

Private function move down below external and public functions https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L35 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L48

[NC-9] Functions not used internally could be marked external

Instance

[NC-10] Threshold be declared as constant or varialbe instead of magic number

Number 1 should be change to threshold constant

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

[NC-11] Natspec is not updated to latest information

The natspec still haves Safe keyword from Gnosis code.

Instance

#0 - c4-judge
2023-01-22T15:35:42Z

gzeon-c4 marked the issue as grade-b

#1 - c4-sponsor
2023-02-09T12:33:08Z

livingrockrises marked the issue as sponsor confirmed

Biconomy - Smart Contract Wallet contest - ast3ros'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

[H-03] [H-02] An attacker could front-run the counter factual wallet and register malicious handler contract - $26.26

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - c4-judge2023-01-17T07:47:49Z

#1 - c4-sponsor2023-01-26T06:29:02Z

#2 - livingrockrises2023-01-26T06:29:58Z

#3 - c4-judge2023-02-10T12:25:30Z

[H-04] [H-01] An attacker contract can send transactions to the Smart Account and withdraw all the funds of user in the account - $22.72

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - c4-judge2023-01-17T07:17:52Z

#1 - c4-sponsor2023-01-26T00:19:43Z

#2 - c4-judge2023-02-10T12:28:27Z

[Q-28] QA Report - $36.50

Findings Information

Awards

Labels

External Links

[L-1] Unspecific compiler version pragma

Instance:

Recommended Mitigation Steps:

[L-2] Input Address is not checked

Instance

[L-3] Missing Events on State Changing Functions and critical functions:

Instance

Recommended Mitigation Steps

[L-4] Critical address chang should use two-step procedure

Instance

Recommended Mitigation Steps

[L-5] Owner can be set by the contract itself

Instance

Recommended Mitigation Steps

[L-6] Change names and implement struct in EntryPoint contract to follow EIP4337

Instance

Recommended Mitigation Steps

[NC-1] Remove deadcode and dead comments

Dead code:

Dead comments:

[NC-2] Event is missing indexed fields

[NC-3] Wrong error message

[NC-4] Code duplication

[NC-5] Missing natspecs in many functions, or natspecs without parameter explaination

[NC-6] require() / revert() statements should have descriptive reason strings

[NC-7] Add function simulateHandleOp to simulate full execution of a UserOperation (including both validation and target execution)

[NC-8] Function order does not follow Solidity style guides

[NC-9] Functions not used internally could be marked external

Instance

[NC-10] Threshold be declared as constant or varialbe instead of magic number

[NC-11] Natspec is not updated to latest information

Instance

#0 - c4-judge2023-01-22T15:35:42Z

#1 - c4-sponsor2023-02-09T12:33:08Z

#0 - c4-judge
2023-01-17T07:47:49Z

#1 - c4-sponsor
2023-01-26T06:29:02Z

#2 - livingrockrises
2023-01-26T06:29:58Z

#3 - c4-judge
2023-02-10T12:25:30Z

#0 - c4-judge
2023-01-17T07:17:52Z

#1 - c4-sponsor
2023-01-26T00:19:43Z

#2 - c4-judge
2023-02-10T12:28:27Z

#0 - c4-judge
2023-01-22T15:35:42Z

#1 - c4-sponsor
2023-02-09T12:33:08Z