| Number | Issues Details | Context |
|---|---|---|
| [L-1] | Low level calls with solidity version 0.8.14 and lower can result in optimiser bug | |
| [L-2] | Critical changes should use-two step procedure | 1 |
| [L-3] | initialize() function can be called by anyone |
2 |
| [L-4] | Avoid using tx.origin |
4 |
| Number | Issues Details | Context |
|---|---|---|
| [NC-1] | Open TODO | 1 |
| [NC-2] | Include return parameters in NatSpec comments | |
| [NC-3] | Non-usage of specific imports | |
| [NC-4] | Lines are too long | 14 |
| [NC-5] | Use bytes.concat() |
10 |
| [NC-6] | Use require instead of assert | 2 |
| [NC-7] | Typos | 1 |
| [NC-8] | Lack of event emit | 2 |
| [NC-9] | Require messages are too short and unclear | 5 |
| [NC-10] | Unused receive() Function Will Lock Ether In Contract |
2 |
The protocol is using low level calls with solidity version 0.8.12 which can result in optimizer bug.
Ref: https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d
Consider upgrading to solidity 0.8.17
The following contract have a function that allows them an owner to change it to a different address. If the owner accidentally uses an invalid address for which they do not have the private key, then the system will gets locked.
Consider adding two step procedure on the critical functions where the first is announcing a pending new owner and the new address should then claim its ownership.
A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105
initialize() function can be called anyone when the contract is not initialized
Add a deployer address and require that only him can call the initialize() function.
tx.origin is a global variable in Solidity that returns the address of the account that sent the transaction.
Using the variable could make a contract vulnerable if an authorized account calls a malicious contract. You can impersonate a user using a third party contract.
The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.
If Return parameters are declared, you must prefix them with /// @return.
Some code analysis programs do analysis by reading NatSpec details, if they can't see the @return tag, they do incomplete analysis.
The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly.
Use specific imports syntax per solidity docs recommendation.
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length.
Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
- SmartAccount.sol:46
- SmartAccount.sol:186
- SmartAccount.sol:239
- SmartAccount.sol:346
- SmartAccount.sol:356
- SmartAccount.sol:357
- SmartAccount.sol:489
- EntryPoint.sol:168
- EntryPoint.sol:289
- EntryPoint.sol:319
- /EntryPoint.sol:349
- EntryPoint.sol:363
- EntryPoint.sol:401
- EntryPoint.sol:409
- EntryPoint.sol:440
Solidity version 0.8.4 introduces bytes.concat() vs abi.encodePacked(<bytes>,<bytes>)
- SmartAccount.sol:294
- SmartAccount.sol:347
- SmartAccount.sol:374
- SmartAccount.sol:445
- SmartAccountFactory.sol:34
- SmartAccountFactory.sol:35
- SmartAccountFactory.sol:54
- SmartAccountFactory.sol:69
- SmartAccountFactory.sol:70
- SmartAccountFactory.sol:71
Use bytes.concat()
Assert should not be used except for tests, require should be used
Prior to Solidity 0.8.0, pressing a confirm consumes the remainder of the process's available gas instead of returning it, as request()/revert() did.
The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.
Meanwhile, a require() statement when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.
Assertion should be avoided even after solidity version 0.8.0, because its documentation states "The Assert function generates an error of type Panic(uint256). Code that works properly should never Panic, even on invalid external input. If this happens, you need to fix it in your contract. there's a mistake".
Use require() instead of assert()
/**
* @notice Throws if the sender is not the owner.
*/The below methods do not emit an event when the state changes, something that it's very important for dApps and users.
The correct and clear error description explains to the user why the function reverts, but the error descriptions below in the project are not self-explanatory. These error descriptions are very important in the debug features of DApps like Tenderly. Error definitions should be added to the require block, not exceeding 32 bytes.
If the intention is for the Ether to be used, the function should call another function, otherwise it should revert.
The function should call another function, otherwise it should revert.