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: 50/105
Findings: 3
Award: $101.52
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
26.2582 USDC - $26.26
_entryPoint and _handler are not included in the salt, so a front-running attack could be possible for the same _owner and _index (same counterfactual address) with malicious _entryPoint and _handler (DoS/freeze funds) only entryPoint can be updated, not handler. Add to salt and below for getAddressForCounterfactual.
#0 - c4-judge
2023-01-17T07:26:22Z
gzeon-c4 marked the issue as duplicate of #460
#1 - c4-sponsor
2023-01-26T02:55:04Z
livingrockrises marked the issue as sponsor confirmed
#2 - livingrockrises
2023-01-26T02:55:11Z
should be high risk as per other proofs
#3 - c4-judge
2023-02-10T12:24:50Z
gzeon-c4 marked the issue as satisfactory
#4 - c4-judge
2023-02-10T12:25:21Z
gzeon-c4 changed the severity to 3 (High Risk)
🌟 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
Functions across multiple files.
It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI) as stated in the official Solidity documentation.
Add complete NatSpec (@notice/@param/@return) to all public interfaces across the repo.
This contract acts as a proxy for the user's account but could benefit from additional clarity in its naming.
Rename Proxy to SmartAccountProxy and update the _IMPLEMENTATION_SLOT hash.
As is done in the updateEntryPoint function, it is recommended to follow the checks-effects-interaction pattern. This is usually to safeguard against unsafe external calls and re-entrancy but is still good practice as issues with events can cause problems for off-chain infrastructure such as indexers.
Emit events prior to making any state changes.
This function should revert if the _handler parameter is the zero address; however, there appears to be a simple copy-paste error.
Update the revert string to "Invalid Handler".
Duplicated check for _handler being equal to the zero address is not necessary.
Given the input has already been validated, remove the unnecessary if statement.
Multiple min and max functions are defined and inlined accross multiple contracts.
For readability, use libs/Math.sol library functions rather than inlining. Additionally, the version of max in SmartAccount unnecessarily uses >= which gives the same return values as > as in the library.
Math.sol VersionThis repo uses v4.7.0 of the library.
Ensure that copied library contracts are up-to-date or alternatively import as a dependency. This version is missing a revert string "Math: mulDiv overflow" on #L78 and should read "base 256" on #L336.
Precedence of arithmetic operators means that the final two sets of parentheses here are redundant.
payment = (gasUsed + baseGas) * gasPrice / tokenGasPriceFactor;
Correct formatting and folling of Solidity style guidelines helps with readability and auditing. There appears to be an additional indentation within part of the if statement.
Remove the extra indentation to aid in readability of if/else block.
Given some of the code has been forked from Gnosis Safe and Eth-Infinitism, there remain some relic references. For example, the VerifyingSingletonPaymaster should read 'the wallet signs to prove identity and account ownership'.
Remove references to 'safe' and 'wallet' and change to 'account'.
sigTimeRange as per EIP-4337 UpdatesThe return value is packed of sigFailure, validUntil and validAfter timestamps. sigFailure is 1 byte value of “1” the signature check failed (should not revert on signature failure, to support estimate). validUntil is 8-byte timestamp value, or zero for “infinite”. The UserOp is valid only up to this time. validAfter is 8-byte timestamp. The UserOp is valid only after this time.
// TODO
should return SIG_VALIDATION_FAILED as per EIP and all other errors should revert
// TODO
The SmartAccount singleton should not be able to receive ether.
Remove the receive function. Proxied SmartAccounts already have a receive function.
In other contracts such as SmartAccount and Singleton there are references to singleton rather than defaultImpl.
Rename _deafaultImpl to s_singleton to remain consistent with the naming in SmartAccount contract.
Many of the contracts import several other contracts so it can be difficult to track down from where specific items are being imported. For example, consider this use of UserOperationLib in BaseAccount implicitly imported from IAccount.
For the sake of readability, considering using explicit imports of the form import {X, Y, Z} from "A".
// TODO
Remove the Exec.sol, ECDSA.sol, Initializable.sol and Signatures.sol imports, in addition to the using for directive in PaymasterHelpers.sol.
The DepositInfo struct is commented to state that 112 bit allows for 2^15 eth; however, this calculation is not correct: (2^112 - 1)/1e18 = 5e15
Update the comment to 5e15
Code is duplicated when using EIP-1967 storage at the same slot across both Proxy and Singleton.
Make use of a storage library which can have an internal function to be called by SmartAccount on #L122.
Given the Singleton contract is utilising EIP-1967 storage, the comments regarding order of variable declaration affecting storage location are not accurate. The position of implementation address in storage does not follow Solidity's rules for storage layout and is instead fixed at the _IMPLEMENTATION_SLOT.
Remove these comments (as they pertain to the Gnosis Safe contracts).
_getImplementation With Proxy FallbackIt is possible to combine this function call with the Proxy fallback function, in addition to the recommendation above removing the need for Singleton altogether.
Integrate this call with the Proxy fallback at no additional cost, as is done by GnosisSafeProxy: https://github.com/safe-global/safe-contracts/blob/main/contracts/proxies/GnosisSafeProxy.sol#L31-L34. The difference will be in accessing _IMPLEMENTATION_SLOT directly.
As suggested by the comment on #L4, this interface is not used.
Remove the interface.
This workaround is included to silence unused variable warnings; however, requiredPrefund is in fact used on #L109.
Remove #L99.
Obsolete comments affect readability.
Remove comments which are repeated or no longer relevant.
The naming convention of storage variables even within a single contract is inconsistent and affects readability.
Consider marking all storage variables with s_prefix to be explicit when accessing storage.
The _signer validation is duplicated.
Validate only once after all code paths.
#0 - c4-judge
2023-01-22T15:55:38Z
gzeon-c4 marked the issue as grade-b
#1 - c4-sponsor
2023-02-08T07:29:34Z
livingrockrises marked the issue as sponsor confirmed
#2 - livingrockrises
2023-02-08T07:29:48Z
good report
🌟 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
5 gas can be saved by assigning owner within the event. This also follows checks-effects-interactions due to right-to-left evaluation.
There is 1 instance of this issue:
File: contracts/smart-contract-wallet/SmartAccount.sol 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); }
10 gas can be saved by using the built-in <address>.code.length > 0.
There are 2 instances of this issue:
File: contracts/smart-contract-wallet/libs/LibAddress.sol /** * @notice Will return true if provided address is a contract * @param account Address to verify if contract or not * @dev This contract will return false if called within the constructor of * a contract's deployment, as the code is not yet stored on-chain. */ function isContract(address account) internal view returns (bool) { uint256 csize; // solhint-disable-next-line no-inline-assembly assembly { csize := extcodesize(account) } return csize != 0; }
File: contracts/smart-contract-wallet/paymasters/verifying/VerifyingSingletonPaymaster.sol import "@openzeppelin/contracts/utils/Address.sol"; ... require(!Address.isContract(paymasterId), "Paymaster Id can not be smart contract address");
13 gas can be saved by using the built-in block.chainid in place of this function. This also reduces the bytecode size as this function can be removed completely, given it is only used once.
There is 1 instance of this issue:
File: contracts/smart-contract-wallet/SmartAccount.sol /// @dev Returns the chain id used by this contract. function getChainId() public view returns (uint256) { uint256 id; // solhint-disable-next-line no-inline-assembly assembly { id := chainid() } return id; }
115 can be saved by removing multiplication by 1 in signature checking arithmetic given the account is a 1/1 multisig and so the number of required signatures will only ever be 1. This will be true unless the code changes to support multiple signers per account, in which case it is recommended to document this saving and code requirement for the case of multiple signers. It is also recommended to remove the unnecessary stack variable i on #L302 and pass 0 to signatureSplit directly.
There is 1 instance of this issue:
File: contracts/smart-contract-wallet/SmartAccount.sol /** * @dev Checks whether the signature provided is valid for the provided data, hash. Will revert otherwise. * @param dataHash Hash of the data (could be either a message hash or transaction hash) * @param signatures Signature data that should be verified. Can be ECDSA signature, contract signature (EIP-1271) or approved hash. */ function checkSignatures( bytes32 dataHash, bytes memory data, bytes memory signatures ) public view virtual { uint8 v; bytes32 r; bytes32 s; uint256 i = 0; address _signer; (v, r, s) = signatureSplit(signatures, i); //review if(v == 0) { // If v is 0 then it is a contract signature // When handling contract signatures the address of the contract is encoded into r _signer = address(uint160(uint256(r))); // Check that signature data pointer (s) is not pointing inside the static part of the signatures bytes // This check is not completely accurate, since it is possible that more signatures than the threshold are send. // Here we only check that the pointer is not pointing inside the part that is being processed require(uint256(s) >= uint256(1) * 65, "BSA021"); ...
Do not create local variables to cache memory reads when they are referenced only once and can be used directly.
There are 2 instances of this issue:
File: contracts/smart-contract-wallet/paymasters/verifying/VerifyingSingletonPaymaster.sol function withdrawTo(address payable withdrawAddress, uint256 amount) public override { uint256 currentBalance = paymasterIdBalances[msg.sender]; require(amount <= currentBalance, "Insufficient amount to withdraw"); paymasterIdBalances[msg.sender] -= amount; entryPoint.withdrawTo(withdrawAddress, amount); }
File: contracts/smart-contract-wallet/SmartAccount.sol /** * @dev Checks whether the signature provided is valid for the provided data, hash. Will revert otherwise. * @param dataHash Hash of the data (could be either a message hash or transaction hash) * @param signatures Signature data that should be verified. Can be ECDSA signature, contract signature (EIP-1271) or approved hash. */ function checkSignatures( bytes32 dataHash, bytes memory data, bytes memory signatures ) public view virtual { uint8 v; bytes32 r; bytes32 s; uint256 i = 0; address _signer; (v, r, s) = signatureSplit(signatures, i); ...
From a previous Gnosis Safe audit:
- Currently, the “transaction” parameter is a byte string that contains Call or DelegateCall operations packed according to ABI. This leaves some gaps filled with zeros. If these gaps were removed, the gas savings per operation could be around 400 gas. I understand that this encoding is used because of client side simplicity (there are ready available functions to perform ABI-style encoding of parameters).
There is 1 instance of this issue:
#0 - c4-judge
2023-01-22T16:24:20Z
gzeon-c4 marked the issue as grade-b
#1 - c4-sponsor
2023-02-08T07:36:59Z
livingrockrises marked the issue as sponsor confirmed