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: 29/105
Findings: 2
Award: $399.77
🌟 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
361.0144 USDC - $361.01
During the audit, 4 low and 14 non-critical issues were found.
| № | Title | Risk Rating | Instance Count |
|---|---|---|---|
| L-1 | Review is needed | Low | 10 |
| L-2 | The require for not being a smart contract can be passed | Low | 1 |
| L-3 | Use the two-step-transfer of ownership | Low | 1 |
| L-4 | Misleading comment | Low | 1 |
| NC-1 | Order of Functions | Non-Critical | 5 |
| NC-2 | Order of Layout | Non-Critical | 6 |
| NC-3 | Open TODOs | Non-Critical | 1 |
| NC-4 | Typos | Non-Critical | 6 |
| NC-5 | Unused named return variables | Non-Critical | 4 |
| NC-6 | Commented code | Non-Critical | 12 |
| NC-7 | Empty function body | Non-Critical | 1 |
| NC-8 | Duplicate comment | Non-Critical | 1 |
| NC-9 | Variables with similar names | Non-Critical | 4 |
| NC-10 | No space between the control structures | Non-Critical | 2 |
| NC-11 | "nice to have" | Non-Critical | 1 |
| NC-12 | Missing NatSpec | Non-Critical | 6 |
| NC-13 | Missing leading underscores | Non-Critical | 18 |
| NC-14 | Maximum line length exceeded | Non-Critical | 65 |
There are many places in the code that have questions and require further review.
// review virtual// Review if we need to make view function// review? if rename wallet to account is must// review IEntryPoint private _entryPoint;//@review getNonce specific to EntryPoint requirements//review if(v == 0) {//@review //Method is updated to instruct delegate call and emit regular events// Review for sig collision and HAL-04 report i// review need and impact of this update wallet -> accountaddress paymasterId; //@reviewResolve issues and/or delete irrelevant comments.
It is unsafe to assume that an address for which isContract function returns false is an externally-owned account (EOA) and not a contract. Among others, isContract will return false for the following types of addresses:
If the owner accidentally transfers ownership to an incorrect address, protected functions may become permanently inaccessible.
Link:
function setOwner(address _newOwner) external mixedAuth { require(_newOwner != address(0), "Smart Account:: new Signatory address cannot be zero"); address oldOwner = owner; owner = _newOwner; emit EOAChanged(address(this), oldOwner, _newOwner); }
Consider using a two-step-transfer of ownership: the current owner would nominate a new owner, and to become the new owner, the nominated account would have to approve the change, so that the address is proven to be valid.
Only mode variable is unused.
Change to:
// (mode; // unused param
According to Style Guide, ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier.
Functions should be grouped according to their visibility and ordered:
External functions should be placed before public:
receive() function should be placed before fallback():
Reorder functions where possible.
According to Order of Layout, inside each contract, library or interface, use the following order:
State variables should be placed before events:
Event should be placed before constructor:
enum should be before function:
struct should be placed before events:
Resolve issues.
// Domain Seperators => Separators// We only substract 2500 (compared to the 3000 before) to ensure that the amount passed is still higher than targetTxGas => subtract/// @param targetTxGas Fas that should be used for the safe transaction. => Gas// 0xa9059cbb - keccack("transfer(address,uint256)") => keccak* - the validateUserOp MUST valiate the aggregator parameter, and MAY ignore the userOp.signature field. => validate// get returned data from last call or calldelegate => delegatecallBoth named return variable(s) and return statement are used.
To improve clarity use only named return variables.
For example, change:
function functionName() returns (uint id) { return x;
to
function functionName() returns (uint id) { id = x;
Delete commented code.
The same comment is at lines №8 and №12 in Executor.sol
Delete one of the comments.
Variables with similar names in the same part of the code make it difficult to understand the function and test it, and also it increases the likelihood of an error due to a misprint.
This part of the code has variables opasLen and opslen, opa and ops.
Consider renaming variables to more different.
According to Style Guide, there should be a single space between the control structures if, while, and for and the parenthetic block representing the conditional.
Change:
if(...) { ... }
to:
if (...) { ... }
Probably, this event was supposed to be implemented.
Link:
// nice to have // event SmartAccountInitialized(IEntryPoint indexed entryPoint, address indexed owner);
6 contracts have fucntions withh missing or incomplete NatSpec.
Add NatSpec for all functions.
Internal and private constants, immutables and functions should have a leading underscore:
Public immutables - should not:
Add and remove leading underscores where needed.
According to Style Guide, maximum suggested line length is 120 characters.
Longer lines make the code harder to read.
Make the lines shorter.
#0 - c4-judge
2023-01-22T15:54:34Z
gzeon-c4 marked the issue as grade-a
#1 - c4-sponsor
2023-02-09T10:55:43Z
livingrockrises marked the issue as sponsor confirmed
#2 - livingrockrises
2023-02-09T10:55:49Z
like report quality!
🌟 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
38.7634 USDC - $38.76
During the audit, 2 gas issues were found.
Total savings ~455.
| № | Title | Instance Count | Saved |
|---|---|---|---|
| G-1 | Use unchecked blocks for incrementing | 10 | 350 |
| G-2 | Use unchecked blocks for subtractions where underflow is impossible | 3 | 105 |
In Solidity 0.8+, there’s a default overflow and underflow check on unsigned integers. In the loops, "i" (or other variable) will not overflow because the loop will run out of gas before that.
Change:
for (uint256 i; i < n; ++i) { // ... }
to:
for (uint256 i; i < n;) { // ... unchecked { ++i; } }
This saves ~30-40 gas per iteration.
So, ~35*10 = 350
In Solidity 0.8+, there’s a default overflow and underflow check on unsigned integers. When an overflow or underflow isn’t possible (after require or if-statement), some gas can be saved by using unchecked blocks.
This saves ~35.
So, ~35*3 = 105
#0 - c4-judge
2023-01-22T16:20:45Z
gzeon-c4 marked the issue as grade-b
#1 - c4-sponsor
2023-02-09T12:26:09Z
livingrockrises marked the issue as sponsor confirmed