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
Rank: 23/105
Findings: 2
Award: $570.89
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
36.5015 USDC - $36.50
| L-R | Issue | Instances |
|---|---|---|
| [L‑01] | Use the latest version of OpenZeppelin | 1 |
| [L‑02] | Unused/empty receive()/fallback() function | 1 |
| [L‑03] | Avoid the use of tx.origin | 1 |
Total: 3 instances over 3 issues
| N-C | Issue | Instances |
|---|---|---|
| [N‑01] | Event is missing indexed fields | 2 |
| [N‑02] | Commented out code | 2 |
| [N‑03] | Use the latest Solidity version | 1 |
| [N‑04] | Constants should be defined rather than using magic numbers | 2 |
Total: 7 instances over 4 issues
To prevent any issues in the future (e.g. using solely hardhat to compile and deploy the contracts), upgrade the used OZ packages within the package.json to the latest versions.
"@openzeppelin/contracts": "^4.7.3",
Consider using the latest OZ packages (v4.8.0) within package.json.
Sometimes Ether might be mistakenly sent to the contract. It's better to prevent this from happening. smartAccount.sol If the intention is for the Ether to be used, the function should call another function, otherwise it should revert If a method does not have an external call then it is impossible to reenter, so you can skip this modifier in such methods
File: SmartAccount.sol
receive() external payable {}
Lines of code:
The tx.origin environment variable has been found to influence a control flow decision. Note that using tx.origin as a security control might cause a situation where a user inadvertently authorizes a smart contract to perform an action on their behalf. It is recommended to use msg.sender instead.
File: SmartAccount.sol
address payable receiver = refundReceiver == address(0) ? payable(tx.origin) : refundReceiver;
Lines of code:
Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
File: SmartAccount.sol
event ImplementationUpdated(address _scw, string version, address newImplementation); event EntryPointChanged(address oldEntryPoint, address newEntryPoint);
Lines of code:
I suggest removing the commented-out code or adding an explanation.
File: SmartAccount.sol
// uint256 extraGas;
// extraGas = extraGas - gasleft();
Lines of code:
The contract is using 0.8.12, upgrade it to 0.8.17.
File: SmartAccount.sol
uint256 startGas = gasleft() + 21000 + msg.data.length * 8;
require(gasleft() >= max((_tx.targetTxGas * 64) / 63,_tx.targetTxGas + 2500) + 500, "BSA010");
Lines of code:
#0 - c4-judge
2023-01-22T15:25:48Z
gzeon-c4 marked the issue as grade-b
#1 - c4-sponsor
2023-02-09T12:40:05Z
livingrockrises marked the issue as sponsor confirmed
🌟 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
534.3927 USDC - $534.39
| G-O | Issue |
|---|---|
| [G‑01] | Use x != 0 instead of x > 0 for uint types |
| [G‑02] | Use custom errors instead of revert strings |
| [G‑03] | x = x - y costs less gas than x -= y, same for addition |
| [G‑04] | Remove public visibility from constant variables |
| [G‑05] | ++I/I++ SHOULD BE UNCHECKED{++I}/UNCHECKED{I++} WHEN IT IS NOT POSSIBLE FOR THEM TO OVERFLOW, AS IS THE CASE WHEN USED IN FOR-LOOP AND WHILE-LOOPS |
| [G‑06] | BYTES CONSTANTS ARE MORE EFFICIENT THAN STRING CONSTANTS |
| [G‑07] | Using Solidity version 0.8.17 will provide an overall gas optimization |
| [G‑08] | THERE’S NO NEED TO SET DEFAULT VALUES FOR VARIABLES |
| [G‑09] | USING STORAGE INSTEAD OF MEMORY FOR STRUCTS/ARRAYS SAVES GAS -3 |
| [G‑10] | ++I COSTS LESS GAS THAN I++, ESPECIALLY WHEN IT’S USED IN FOR-LOOPS (--I/I-- TOO) |
Total: 10 issues
Solidity 0.8.4 added the custom errors functionality, which can be use instead of revert strings, resulting in big gas savings on errors. Replace all revert statements with custom error ones
Example from EntryPoint.sol:
require(beneficiary != address(0), "AA90 invalid beneficiary");
Line of code:
The != operator costs less gas than > and for uint types you can use it to check for non-zero values to save gas
File: SmartAccount.sol
if (refundInfo.gasPrice > 0) {
Lines of code:
You can replace all -= and += occurrences to save gas
File: EntryPoint.sol
collected += _executeUserOp(i, ops[i], opInfos[i]);
totalOps += opsPerAggregator[i].userOps.length;
collected += _executeUserOp(opIndex, ops[i], opInfos[opIndex]);
actualGas += preGas - gasleft();
Lines of code:
Constant variables are custom to the contract and won't need to be read on-chain - anyone can just see their values from the source code and, if needed, hardcode them into other contracts. Removing the public visibility will optimise deployment cost since no automatically generated getters will exist in the bytecode of the contract.
File: SmartAccountFactory.sol
string public constant VERSION = "1.0.2";
Lines of code:
File: DefaultCallbackHandler.sol
string public constant NAME = "Default Callback Handler"; string public constant VERSION = "1.0.0";
Lines of code:
In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.
File: EntryPoint.sol
The for loops in function handleAggregatedOps can be unchecked.
If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is cheaper in solidity.
File: SmartAccountFactory.sol
string public constant VERSION = "1.0.2";
Lines of code:
File: DefaultCallbackHandler.sol
string public constant NAME = "Default Callback Handler"; string public constant VERSION = "1.0.0";
Lines of code:
Using at least 0.8.10 will save gas due to skipped extcodesize check if there is a return value. Currently the contracts are compiled using version 0.8.7 (Foundry default). It is easily changeable to 0.8.17 using the command sed -i 's/0.8.7/^0.8.0/' test/*.sol && sed -i '4isolc = "0.8.17"' foundry.toml. This will have the following total savings obtained by forge snapshot --diff | tail -1:
If a variable is not set/initialized, the default value is assumed (0, false, 0x0 … depending on the data type). You are simply wasting gas if you directly initialize it with its default value.
File: EntryPoint.sol
uint256 missingAccountFunds = 0;
Lines of code:
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct.
File: EntryPoint.sol
MemoryUserOp memory mUserOp = opInfo.mUserOp;
Lines of code:
Saves 5 gas per loop
All for loops in EntryPoint.sol.
Example:
for (uint256 i = 0; i < opslen; i++) {
#0 - c4-judge
2023-01-22T16:06:05Z
gzeon-c4 marked the issue as grade-a
#1 - c4-sponsor
2023-02-09T12:38:03Z
livingrockrises marked the issue as sponsor confirmed