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: 9/105
Findings: 3
Award: $940.23
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: 0x52
Also found by: 0xSmartContract, Deivitto, Diana, IllIllI, Koolex, Rolezn, SleepingBugs, V_B, adriro, betweenETHlines, cryptostellar5, oyc_109, peanuts
44.8261 USDC - $44.83
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L18 https://github.com/code-423n4/2023-01-biconomy/tree/main/scw-contracts/contracts\smart-contract-wallet\base\FallbackManager.sol#L8 https://github.com/code-423n4/2023-01-biconomy/tree/main/scw-contracts/contracts\smart-contract-wallet\base\ModuleManager.sol#L9 https://github.com/code-423n4/2023-01-biconomy/tree/main/scw-contracts/contracts\smart-contract-wallet\handler\DefaultCallbackHandler.sol#L11 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L22
For upgradeable contracts, there must be storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Otherwise it may be very difficult to write new implementation code. Without storage gap, the variable in child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences to the child contracts.
Refer to the bottom part of this article: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable
However, the contract doesn't contain a storage gap. The storage gap is essential for upgradeable contract because "It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments". See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
Recommend adding appropriate storage gap at the end of upgradeable contracts such as the below. Please reference OpenZeppelin upgradeable contract templates.
uint256[50] private __gap;
#0 - c4-judge
2023-01-17T15:53:31Z
gzeon-c4 marked the issue as duplicate of #352
#1 - livingrockrises
2023-01-19T17:42:41Z
gnosis safe is upgradeable contract and smart account uses the same proxy - implementation pattern.
https://github.com/safe-global/safe-contracts/blob/main/contracts/Safe.sol#L21
here the first inherited contract (with state) is Singleton.sol
where,
/// @title Singleton - Base for singleton contracts (should always be first super contract) /// This contract is tightly coupled to our proxy contract (see `proxies/GnosisSafeProxy.sol`) /// @author Richard Meissner - <richard@gnosis.io> contract Singleton { // singleton always needs to be first declared variable, to ensure that it is at the same location as in the Proxy contract. // It should also always be ensured that the address is stored alone (uses a full word) address private singleton; }``` now if the inherited contracts do not have the storage gaps, is this a problem?
#2 - c4-sponsor
2023-01-19T17:42:52Z
livingrockrises requested judge review
#3 - c4-sponsor
2023-01-19T17:43:00Z
livingrockrises marked the issue as sponsor disputed
#4 - c4-judge
2023-02-10T12:36:42Z
gzeon-c4 marked the issue as satisfactory
🌟 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
| Issue | Contexts | |
|---|---|---|
| LOW‑1 | Avoid using tx.origin | 3 |
| LOW‑2 | Use of ecrecover is susceptible to signature malleability | 2 |
| LOW‑3 | Init functions are susceptible to front-running | 1 |
| LOW‑4 | Low Level Calls With Solidity Version 0.8.14 Can Result In Optimiser Bug | 6 |
| LOW‑5 | Missing parameter validation in constructor | 1 |
| LOW‑6 | Missing Contract-existence Checks Before Low-level Calls | 10 |
| LOW‑7 | Contracts are not using their OZ Upgradeable counterparts | 9 |
| LOW‑8 | Remove unused code | 5 |
| LOW‑9 | require() should be used instead of assert() | 1 |
| LOW‑10 | Unused receive() Function Will Lock Ether In Contract | 1 |
| LOW‑11 | Missing Non Reentrancy Modifiers | 6 |
Total: 45 contexts over 11 issues
| Issue | Contexts | |
|---|---|---|
| NC‑1 | Critical Changes Should Use Two-step Procedure | 6 |
| NC‑2 | Function creates dirty bits | 7 |
| NC‑3 | Function writing that does not comply with the Solidity Style Guide | 37 |
| NC‑4 | Use delete to Clear Variables | 4 |
| NC‑5 | No need to initialize uints to zero | 11 |
| NC‑6 | Lines are too long | 3 |
| NC‑7 | Missing event for critical parameter change | 4 |
| NC‑8 | Implementation contract may not be initialized | 6 |
| NC‑9 | Non-usage of specific imports | 43 |
| NC‑10 | Use a more recent version of Solidity | 37 |
| NC‑11 | Open TODOs | 1 |
| NC‑12 | Use bytes.concat() | 16 |
| NC‑13 | Use of Block.Timestamp | 5 |
| NC‑14 | Commented Code | All in-scope contracts |
| NC‑15 | Missing/In-complete NATSPEC | All in-scope contracts |
Total: 252 contexts over 15 issues
tx.origintx.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.
This can make it easier to create a vault on behalf of another user with an external administrator (by receiving it as an argument).
257: address payable receiver = refundReceiver == address(0) ? payable(tx.origin) : refundReceiver;
281: address payable receiver = refundReceiver == address(0) ? payable(tx.origin) : refundReceiver;
511: require(owner == hash.recover(userOp.signature) || tx.origin == address(0), "account: wrong signature");
ecrecover is susceptible to signature malleabilityThe built-in EVM precompile ecrecover is susceptible to signature malleability, which could lead to replay attacks.
References: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121, and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57.
While this is not immediately exploitable, this may become a vulnerability if used elsewhere.
347: 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); if(v == 0) { _signer = address(uint160(uint256(r))); require(uint256(s) >= uint256(1) * 65, "BSA021"); require(uint256(s) + 32 <= signatures.length, "BSA022"); uint256 contractSignatureLen; assembly { contractSignatureLen := mload(add(add(signatures, s), 0x20)) } require(uint256(s) + 32 + contractSignatureLen <= signatures.length, "BSA023"); bytes memory contractSignature; assembly { contractSignature := add(add(signatures, s), 0x20) } require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024"); } else if(v > 30) { _signer = ecrecover(keccak256(abi.encodePacked("/x19Ethereum Signed Message:/n32", dataHash)), v - 4, r, s); require(_signer == owner, "INVALID_SIGNATURE"); } else { _signer = ecrecover(dataHash, v, r, s); require(_signer == owner, "INVALID_SIGNATURE"); } }
350: 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); if(v == 0) { _signer = address(uint160(uint256(r))); require(uint256(s) >= uint256(1) * 65, "BSA021"); require(uint256(s) + 32 <= signatures.length, "BSA022"); uint256 contractSignatureLen; assembly { contractSignatureLen := mload(add(add(signatures, s), 0x20)) } require(uint256(s) + 32 + contractSignatureLen <= signatures.length, "BSA023"); bytes memory contractSignature; assembly { contractSignature := add(add(signatures, s), 0x20) } require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024"); } else if(v > 30) { _signer = ecrecover(keccak256(abi.encodePacked("/x19Ethereum Signed Message:/n32", dataHash)), v - 4, r, s); require(_signer == owner, "INVALID_SIGNATURE"); } else { _signer = ecrecover(dataHash, v, r, s); require(_signer == owner, "INVALID_SIGNATURE"); } }
Consider using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L138-L149
Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.
Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: code-423n4/2021-09-sushimiso-findings#64)
166: function init(address _owner, address _entryPointAddress, address _handler) public override initializer {
Ensure atomic calls to init functions along with deployment via robust deployment scripts or factory contracts. Emit explicit events for initializations of contracts. Enforce prevention of re-initializations via explicit setting/checking of boolean initialized variables in the main init function instead of downstream function checks.
The project contracts in scope are using low level calls with solidity version before 0.8.14 which can result in optimizer bug. https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d
Simliar findings in Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#5-low-level-calls-with-solidity-version-0814-can-result-in-optimiser-bug
POC can be found in the above medium reference url.
Functions that execute low level calls in contracts with solidity version under 0.8.14
512: assembly {mstore(0, number())}
63: assembly { let ofs := userOp let len := sub(sub(sig.offset, ofs), 32) ret := mload(0x40) mstore(0x40, add(ret, add(len, 32))) mstore(ret, len) calldatacopy(add(ret, 32), ofs, len) }
41: assembly { let ptr := mload(0x40) mstore(0x40, add(ptr, add(returndatasize(), 0x20))) mstore(ptr, returndatasize()) returndatacopy(add(ptr, 0x20), 0, returndatasize()) returnData := ptr }
35: assembly { let handler := sload(slot) if iszero(handler) { return(0, 0) } calldatacopy(0, 0, calldatasize()) // The msg.sender address is shifted to the left by 12 bytes to remove the padding // Then the address without padding is stored right after the calldata mstore(calldatasize(), shl(96, caller())) // Add 20 bytes for the address appended add the end let success := call(gas(), handler, 0, 0, add(calldatasize(), 20), 0, 0) returndatacopy(0, 0, returndatasize()) if iszero(success) { revert(0, returndatasize()) } return(0, returndatasize()) }
88: assembly { // Load free memory location let ptr := mload(0x40) // We allocate memory for the return data by setting the free memory location to // current free memory location + data size + 32 bytes for data size value mstore(0x40, add(ptr, add(returndatasize(), 0x20))) // Store the size mstore(ptr, returndatasize()) // Store the data returndatacopy(add(ptr, 0x20), 0, returndatasize()) // Point the return data to the correct memory location returnData := ptr }
129: assembly { mstore(array, moduleCount) }
Consider upgrading to at least solidity v0.8.15.
constructorSome parameters of constructors are not checked for invalid values.
15: address _implementation
Validate the parameters.
Low-level calls return success if there is no code present at the specified address.
108: (bool success,) = payable(msg.sender).call{value : missingAccountFunds, gas : type(uint256).max}("");
261: (bool success,) = receiver.call{value: payment}("");
285: (bool success,) = receiver.call{value: payment}("");
451: (bool success,) = dest.call{value:amount}("");
478: (bool success, bytes memory result) = target.call{value : value}(data);
527: (bool req,) = address(entryPoint()).call{value : msg.value}("");
37: (bool success,) = beneficiary.call{value : amount}("");
176: (bool success,bytes memory result) = address(mUserOp.sender).call{gas : mUserOp.callGasLimit}(callData);
106: (bool success,) = withdrawAddress.call{value : stake}("");
120: (bool success,) = withdrawAddress.call{value : withdrawAmount}("");
In addition to the zero-address checks, add a check to verify that <address>.code.length > 0
The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.
5: import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 6: import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; 16: import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
7: import "@openzeppelin/contracts/access/Ownable.sol";
7: import "@openzeppelin/contracts/access/Ownable.sol";
4: import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
6: import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; 7: import "@openzeppelin/contracts/proxy/utils/Initializable.sol"; 8: import "@openzeppelin/contracts/utils/Address.sol";
Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.
This code is not used in the main project contract files, remove it or add event-emit. Code that is not in use, suggests that they should not be present and could potentially contain insecure functionalities.
function delegateCall
function callAndRevert
function _getImplementation
function ceilDiv
require() Should Be Used Instead Of assert()Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its <a href="https://docs.soliditylang.org/en/v0.8.14/control-structures.html#panic-via-assert-and-error-via-require">documentation</a> states that "The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix".
13: assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("biconomy.scw.proxy.implementation")) - 1));
receive() Function Will Lock Ether In ContractIf the intention is for the Ether to be used, the function should call another function, otherwise it should revert
550: receive() external payable {}
The function should call another function, otherwise it should revert
The following functions are missing reentrancy modifier although some other public/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well.
function execTransaction( Transaction memory _tx, uint256 batchId, FeeRefund memory refundInfo, bytes memory signatures ) public payable virtual override returns (bool success) {
function handlePaymentRevert( uint256 gasUsed, uint256 baseGas, uint256 gasPrice, uint256 tokenGasPriceFactor, address gasToken, address payable refundReceiver ) external returns (uint256 payment) {
function requiredTxGas( address to, uint256 value, bytes calldata data, Enum.Operation operation ) external returns (uint256) {
function addDeposit() public payable {
function withdrawDepositTo(address payable withdrawAddress, uint256 amount) public onlyOwner {
Add reentrancy modifiers
The critical procedures should be two step process.
See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure
109: function setOwner(address _newOwner) external mixedAuth {
24: function setEntryPoint(IEntryPoint _entryPoint) public onlyOwner {
26: function setFallbackHandler(address handler) public authorized {
20: function setupModules(address to, bytes memory data) internal {
24: function setEntryPoint(IEntryPoint _entryPoint) public onlyOwner {
65: function setSigner( address _newVerifyingSigner) external onlyOwner{
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
This explanation should be added in the NatSpec comments of this function that sends ether with call;
Note that this code probably isn’t secure or a good use case for assembly because a lot of memory management and security checks are bypassed. Use with caution! Some functions in this contract knowingly create dirty bits at the destination of the free memory pointer.
21: function createSender(bytes calldata initCode) external returns (address sender) { address initAddress = address(bytes20(initCode[0 : 20])); bytes memory initCallData = initCode[20 :]; bool success; assembly { success := call(gas(), initAddress, 0, add(initCallData, 0x20), mload(initCallData), 0, 32) sender := mload(0) } if (!success) { sender = address(0); } } }
15: function call( address to, uint256 value, bytes memory data, uint256 txGas ) internal returns (bool success) { assembly { success := call(txGas, to, value, add(data, 0x20), mload(data), 0, 0) } }
28: function execute( address to, uint256 value, bytes memory data, Enum.Operation operation, uint256 txGas ) internal returns (bool success) { if (operation == Enum.Operation.DelegateCall) { assembly { success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0) } } else { assembly { success := call(txGas, to, value, add(data, 0x20), mload(data), 0, 0) } } if (success) emit ExecutionSuccess(to, value, data, operation, txGas); else emit ExecutionFailure(to, value, data, operation, txGas); } }
45: function setFallbackHandler(address handler) public authorized { internalSetFallbackHandler(handler); emit ChangedFallbackHandler(handler); } fallback() external { bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT; assembly { let handler := sload(slot) if iszero(handler) { return(0, 0) } calldatacopy(0, 0, calldatasize()) mstore(calldatasize(), shl(96, caller())) let success := call(gas(), handler, 0, 0, add(calldatasize(), 20), 0, 0) returndatacopy(0, 0, returndatasize()) if iszero(success) { revert(0, returndatasize()) } return(0, returndatasize()) } } }
22: function transferToken( address token, address receiver, uint256 amount ) internal returns (bool transferred) { bytes memory data = abi.encodeWithSelector(0xa9059cbb, receiver, amount); assembly { let success := call(sub(gas(), 10000), token, 0, add(data, 0x20), mload(data), 0, 0x20) switch returndatasize() case 0 { transferred := success } case 0x20 { transferred := iszero(or(iszero(success), iszero(mload(0)))) } default { transferred := 0 } } } }
53: function multiSend(bytes memory transactions) public payable { require(address(this) != multisendSingleton, "MultiSend should only be called via delegatecall"); assembly { let length := mload(transactions) let i := 0x20 for { } lt(i, length) { } { let operation := shr(0xf8, mload(add(transactions, i))) let to := shr(0x60, mload(add(transactions, add(i, 0x01)))) let value := mload(add(transactions, add(i, 0x15))) let dataLength := mload(add(transactions, add(i, 0x35))) let data := add(transactions, add(i, 0x55)) let success := 0 switch operation case 0 { success := call(gas(), to, value, data, dataLength, 0, 0) } case 1 { success := delegatecall(gas(), to, data, dataLength, 0, 0) } if eq(success, 0) { revert(0, 0) } i := add(i, add(0x55, dataLength)) } } } }
47: function multiSend(bytes memory transactions) public payable { assembly { let length := mload(transactions) let i := 0x20 for { } lt(i, length) { } { let operation := shr(0xf8, mload(add(transactions, i))) let to := shr(0x60, mload(add(transactions, add(i, 0x01)))) let value := mload(add(transactions, add(i, 0x15))) let dataLength := mload(add(transactions, add(i, 0x35))) let data := add(transactions, add(i, 0x55)) let success := 0 switch operation case 0 { success := call(gas(), to, value, data, dataLength, 0, 0) } case 1 { revert(0, 0) } if eq(success, 0) { revert(0, 0) } i := add(i, add(0x55, dataLength)) } } } }
Add this comment to the function:
/// @dev Use with caution! Some functions in this contract knowingly create dirty bits at the destination of the free memory pointer. Note that this code probably isn’t secure or a good use case for assembly because a lot of memory management and security checks are bypassed.
Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.
https://docs.soliditylang.org/en/v0.8.17/style-guide.html
Functions should be grouped according to their visibility and ordered:
All in-scope contracts
delete to Clear Variablesdelete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.
The delete key better conveys the intention and is also more idiomatic. Consider replacing assignments of zero with delete statements.
106: opIndex = 0;
102: info.unstakeDelaySec = 0; 103: info.withdrawTime = 0; 104: info.stake = 0;
There is no need to initialize uint variables to zero as their default value is 0
234: uint256 payment = 0;
310: uint256 i = 0;
78: uint256 collected = 0;
99: uint256 totalOps = 0; 106: uint256 opIndex = 0; 126: uint256 collected = 0;
313: uint256 missingAccountFunds = 0;
119: uint256 moduleCount = 0;
259: uint256 result = 0;
310: uint256 result = 0;
310: uint256 result = 0;
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
46: // "AccountTx(address to,uint256 value,bytes data,uint8 operation,uint256 targetTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,uint256 nonce)"
186: /// @dev Allows to execute a Safe transaction confirmed by required number of owners and then pays the account that submitted the transaction.
357: /// Since the `estimateGas` function includes refunds, call this method to get an estimated of the costs that are deducted from the safe with `execTransaction`
When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.
24: function setEntryPoint(IEntryPoint _entryPoint) public onlyOwner {
20: function setupModules(address to, bytes memory data) internal {
24: function setEntryPoint(IEntryPoint _entryPoint) public onlyOwner {
65: function setSigner( address _newVerifyingSigner) external onlyOwner{
OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5
15: constructor(address _implementation)
17: constructor(address _baseImpl)
20: constructor(IEntryPoint _entryPoint)
12: constructor()
20: constructor(IEntryPoint _entryPoint)
35: constructor(IEntryPoint _entryPoint, address _verifyingSigner) BasePaymaster(_entryPoint)
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. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files
A good example:
import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol"; import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; import {SafeCastLib} from "solmate/utils/SafeCastLib.sol"; import {ERC20} from "solmate/tokens/ERC20.sol"; import {IProducer} from "src/interfaces/IProducer.sol"; import {GlobalState, UserState} from "src/Common.sol";
10: import "./common/Enum.sol";
4: import "./libs/LibAddress.sol"; 8: import "./BaseSmartAccount.sol"; 9: import "./common/Singleton.sol"; 10: import "./base/ModuleManager.sol"; 11: import "./base/FallbackManager.sol"; 12: import "./common/SignatureDecoder.sol"; 13: import "./common/SecuredTokenTransfer.sol"; 14: import "./interfaces/ISignatureValidator.sol"; 15: import "./interfaces/IERC165.sol";
4: import "./Proxy.sol"; 5: import "./BaseSmartAccount.sol";
8: import "../interfaces/IPaymaster.sol"; 9: import "../interfaces/IEntryPoint.sol";
12: import "../interfaces/IAccount.sol"; 13: import "../interfaces/IPaymaster.sol"; 15: import "../interfaces/IAggregatedAccount.sol"; 16: import "../interfaces/IEntryPoint.sol"; 17: import "../utils/Exec.sol"; 18: import "./StakeManager.sol"; 19: import "./SenderCreator.sol";
4: import "../interfaces/IStakeManager.sol";
4: import "./UserOperation.sol";
4: import "./UserOperation.sol"; 5: import "./IAccount.sol"; 6: import "./IAggregator.sol";
4: import "./UserOperation.sol";
12: import "./UserOperation.sol"; 13: import "./IStakeManager.sol"; 14: import "./IAggregator.sol";
4: import "./UserOperation.sol";
4: import "../common/Enum.sol";
4: import "../common/SelfAuthorized.sol";
4: import "../common/Enum.sol"; 5: import "../common/SelfAuthorized.sol"; 6: import "./Executor.sol";
4: import "../interfaces/ERC1155TokenReceiver.sol"; 5: import "../interfaces/ERC721TokenReceiver.sol"; 6: import "../interfaces/ERC777TokensRecipient.sol"; 7: import "../interfaces/IERC165.sol";
5: import "../../BasePaymaster.sol"; 9: import "../../PaymasterHelpers.sol"; 10: import "../samples/Signatures.sol";
Use specific imports syntax per solidity docs recommendation.
<a href="https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/">0.8.13</a>: Ability to use using for with a list of free functions
<a href="https://blog.soliditylang.org/2022/05/18/solidity-0.8.14-release-announcement/">0.8.14</a>:
ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases. Override Checker: Allow changing data location for parameters only when overriding external functions.
<a href="https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/">0.8.15</a>:
Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays. Yul Optimizer: Keep all memory side-effects of inline assembly blocks.
<a href="https://blog.soliditylang.org/2022/08/08/solidity-0.8.16-release-announcement/">0.8.16</a>:
Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.
<a href="https://blog.soliditylang.org/2022/09/08/solidity-0.8.17-release-announcement/">0.8.17</a>:
Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.
All in-scope contracts
Consider updating to a more recent solidity version.
An open TODO is present. It is recommended to avoid open TODOs as they may indicate programming errors that still need to be fixed.
255: // TODO: copy logic of gasPrice?
bytes.concat()Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))
294: revert(string(abi.encodePacked(requiredGas) 374: revert(string(abi.encodePacked(requiredGas) 347: _signer = ecrecover(keccak256(abi.encodePacked("/x19Ethereum Signed Message:/n32", dataHash) 294: revert(string(abi.encodePacked(requiredGas) 374: revert(string(abi.encodePacked(requiredGas) 445: return abi.encodePacked(bytes1(0x19)
34: bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index) 70: bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index) 35: bytes memory deploymentData = abi.encodePacked(type(Proxy) 54: bytes memory deploymentData = abi.encodePacked(type(Proxy) 35: bytes memory deploymentData = abi.encodePacked(type(Proxy) 54: bytes memory deploymentData = abi.encodePacked(type(Proxy) 69: bytes memory code = abi.encodePacked(type(Proxy) 34: bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index) 70: bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index) 71: bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff)
Use bytes.concat() and upgrade to at least Solidity version 0.8.4 if required.
Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts. References: SWC ID: 116
321: if (_deadline != 0 && _deadline < block.timestamp) {
365: if (_deadline != 0 && _deadline < block.timestamp) {
101: require(info.withdrawTime <= block.timestamp, "Stake withdrawal is not due");
Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.
Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.
Commented code is exposed throughout all in-scope contracts which suggest unfinished code. It is recommended to remove commented code for finalized contracts.
All in-scope contracts.
i.e. Such as SmartAccount.sol
192: function execTransaction( Transaction memory _tx, uint256 batchId, FeeRefund memory refundInfo, bytes memory signatures ) public payable virtual override returns (bool success) { // initial gas = 21k + non_zero_bytes * 16 + zero_bytes * 4 // ~= 21k + calldata.length * [1/3 * 16 + 2/3 * 4] uint256 startGas = gasleft() + 21000 + msg.data.length * 8; //console.log("init %s", 21000 + msg.data.length * 8); bytes32 txHash; // Use scope here to limit variable lifetime and prevent `stack too deep` errors { bytes memory txHashData = encodeTransactionData( // Transaction info _tx, // Payment info refundInfo, // Signature info nonces[batchId] ); // Increase nonce and execute transaction. // Default space aka batchId is 0 nonces[batchId]++; txHash = keccak256(txHashData); checkSignatures(txHash, txHashData, signatures); } // We require some gas to emit the events (at least 2500) after the execution and some to perform code until the execution (500) // We also include the 1/64 in the check that is not send along with a call to counteract potential shortings because of EIP-150 require(gasleft() >= max((_tx.targetTxGas * 64) / 63,_tx.targetTxGas + 2500) + 500, "BSA010"); // Use scope here to limit variable lifetime and prevent `stack too deep` errors { // If the gasPrice is 0 we assume that nearly all available gas can be used (it is always more than targetTxGas) // We only substract 2500 (compared to the 3000 before) to ensure that the amount passed is still higher than targetTxGas success = execute(_tx.to, _tx.value, _tx.data, _tx.operation, refundInfo.gasPrice == 0 ? (gasleft() - 2500) : _tx.targetTxGas); // If no targetTxGas and no gasPrice was set (e.g. both are 0), then the internal tx is required to be successful // This makes it possible to use `estimateGas` without issues, as it searches for the minimum gas where the tx doesn't revert require(success || _tx.targetTxGas != 0 || refundInfo.gasPrice != 0, "BSA013"); // We transfer the calculated tx costs to the tx.origin to avoid sending it to intermediate contracts that have made calls uint256 payment = 0; // uint256 extraGas; if (refundInfo.gasPrice > 0) { //console.log("sent %s", startGas - gasleft()); // extraGas = gasleft(); payment = handlePayment(startGas - gasleft(), refundInfo.baseGas, refundInfo.gasPrice, refundInfo.tokenGasPriceFactor, refundInfo.gasToken, refundInfo.refundReceiver); emit WalletHandlePayment(txHash, payment); } // extraGas = extraGas - gasleft(); //console.log("extra gas %s ", extraGas); } }
Missing NATSPEC documentation through many of the in-scope contracts
All in-scope contracts.
#0 - c4-judge
2023-01-22T15:37:42Z
gzeon-c4 marked the issue as grade-a
#1 - c4-sponsor
2023-02-09T12:36:15Z
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
| Issue | Contexts | Estimated Gas Saved | |
|---|---|---|---|
| GAS‑1 | abi.encode() is less efficient than abi.encodepacked() | 5 | 500 |
| GAS‑2 | State variables only set in the constructor should be declared immutable | 1 | - |
| GAS‑3 | Setting the constructor to payable | 6 | 78 |
| GAS‑4 | Duplicated require()/revert() Checks Should Be Refactored To A Modifier Or Function | 8 | 532 |
| GAS‑5 | Empty Blocks Should Be Removed Or Emit Something | 3 | - |
| GAS‑6 | Using fixed bytes is cheaper than using string | 4 | - |
| GAS‑7 | Functions guaranteed to revert when called by normal users can be marked payable | 18 | 378 |
| GAS‑8 | internal functions only called once can be inlined to save gas | 44 | - |
| GAS‑9 | Multiplication/division By Two Should Use Bit Shifting | 3 | - |
| GAS‑10 | Optimize names to save gas | 20 | 440 |
| GAS‑11 | <x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables | 6 | - |
| GAS‑12 | Public Functions To External | 45 | - |
| GAS‑13 | require() Should Be Used Instead Of assert() | 1 | - |
| GAS‑14 | require()/revert() Strings Longer Than 32 Bytes Cost Extra Gas | 19 | - |
| GAS‑15 | Splitting require() Statements That Use && Saves Gas | 3 | 27 |
| GAS‑16 | Help The Optimizer By Saving A Storage Variable’s Reference Instead Of Repeatedly Fetching It | 2 | - |
| GAS‑17 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 11 | - |
| GAS‑18 | ++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- And While-loops | 5 | 175 |
| GAS‑19 | Using unchecked blocks to save gas | 18 | 7344 |
| GAS‑20 | Use solidity version 0.8.17 to gain some gas boost | 37 | 3256 |
| GAS‑21 | Using storage instead of memory saves gas | 1 | 350 |
| GAS‑22 | Struct packing | 3 | - |
Total: 263 contexts over 22 issues
abi.encode() is less efficient than abi.encodepacked()See for more information: https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison
136: return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, getChainId(), this));
431: abi.encode(
197: return keccak256(abi.encode(userOp.hash(), address(this), block.chainid));
28: return abi.encode(data.paymasterId);
80: return keccak256(abi.encode(
constructor should be declared immutableAvoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).
19: _defaultImpl = _baseImpl
constructor to payableSaves ~13 gas per instance
15: constructor(address _implementation)
17: constructor(address _baseImpl)
20: constructor(IEntryPoint _entryPoint)
12: constructor()
20: constructor(IEntryPoint _entryPoint)
35: constructor(IEntryPoint _entryPoint, address _verifyingSigner) BasePaymaster(_entryPoint)
require()/revert() Checks Should Be Refactored To A Modifier Or FunctionSaves deployment costs
262: require(success, "BSA011"); 286: require(success, "BSA011"); 265: require(transferToken(gasToken, receiver, payment), "BSA012"); 289: require(transferToken(gasToken, receiver, payment), "BSA012"); 348: require(_signer == owner, "INVALID_SIGNATURE"); 351: require(_signer == owner, "INVALID_SIGNATURE");
34: require(module != address(0) && module != SENTINEL_MODULES, "BSA101"); 49: require(module != address(0) && module != SENTINEL_MODULES, "BSA101");
The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})
550: receive() external payable {}
119: try aggregator.validateSignatures(ops, opa.signature) {}
458: try IPaymaster(paymaster).postOp{gas : mUserOp.verificationGasLimit}(mode, context, actualGasCost) {}
stringAs a rule of thumb, use bytes for arbitrary-length raw byte data and string for arbitrary-length string (UTF-8) data. If you can limit the length to a certain number of bytes, always use one of bytes1 to bytes32 because they are much cheaper.
36: string public constant VERSION = "1.0.2"; // using AA 0.3.0
11: string public constant VERSION = "1.0.2";
12: string public constant NAME = "Default Callback Handler";
13: string public constant VERSION = "1.0.0";
payableIf a function modifier or require such as onlyOwner/onlyX is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.
449: function transfer(address payable dest, uint amount) external nonReentrant onlyOwner {
455: function pullTokens(address token, address dest, uint256 amount) external onlyOwner {
460: function execute(address dest, uint value, bytes calldata func) external onlyOwner{ 465: function execute(address dest, uint value, bytes calldata func) external onlyOwner{
465: function executeBatch(address[] calldata dest, bytes[] calldata func) external onlyOwner{
489: function execFromEntryPoint(address dest, uint value, bytes calldata func, Enum.Operation operation, uint256 gasLimit) external onlyEntryPoint returns (bool success) {
536: function withdrawDepositTo(address payable withdrawAddress, uint256 amount) public onlyOwner {
24: function setEntryPoint(IEntryPoint _entryPoint) public onlyOwner {
67: function withdrawTo(address payable withdrawAddress, uint256 amount) public onlyOwner {
75: function addStake(uint32 unstakeDelaySec) external payable onlyOwner {
90: function unlockStake() external onlyOwner {
99: function withdrawStake(address payable withdrawAddress) external onlyOwner {
24: function setEntryPoint(IEntryPoint _entryPoint) public onlyOwner {
67: function withdrawTo(address payable withdrawAddress, uint256 amount) public virtual onlyOwner {
75: function addStake(uint32 unstakeDelaySec) external payable onlyOwner {
90: function unlockStake() external onlyOwner {
99: function withdrawStake(address payable withdrawAddress) external onlyOwner {
65: function setSigner( address _newVerifyingSigner) external onlyOwner{
Functions guaranteed to revert when called by normal users can be marked payable.
internal functions only called once can be inlined to save gas73: function _requireFromEntryPoint
87: function _validateSignature
181: function max
501: function _validateAndUpdateNonce
506: function _validateSignature
48: function _postOp
104: function _requireFromEntryPoint
203: function _copyUserOpToMemory
248: function _getRequiredPrefund
261: function _createSenderIfNeeded
289: function _validateAccountPrepayment
349: function _validatePaymasterPrepayment
496: function min
500: function getOffsetOfMemoryBytes
504: function getMemoryBytesFromOffset
23: function getStakeInfo
38: function internalIncrementDeposit
36: function getSender
45: function gasPrice
57: function pack
73: function hash
77: function min
19: function staticcall
29: function delegateCall
40: function getReturnData
51: function revertWithData
57: function callAndRevert
13: function execute
14: function internalSetFallbackHandler
20: function setupModules
10: function transferToken
12: function _setImplementation
20: function _getImplementation
11: function isContract
19: function max
26: function min
34: function average
45: function ceilDiv
48: function _postOp
104: function _requireFromEntryPoint
24: function paymasterContext
34: function decodePaymasterData
43: function decodePaymasterContext
119: function _postOp
<x> * 2 is equivalent to <x> << 1 and <x> / 2 is the same as <x> >> 1. The MUL and DIV opcodes cost 5 gas, whereas SHL and SHR only cost 3 gas
36: return (a & b) + (a ^ b) / 2;
281: if (value >= 10**2) {
282: value /= 10**2;
Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.
See more <a href="https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92">here</a>
All in-scope contracts
Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the Gauge.sol contract will be the most used; A lower method ID may be given.
<x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables81: collected += _executeUserOp(i, ops[i], opInfos[i]);
101: totalOps += opsPerAggregator[i].userOps.length; 135: collected += _executeUserOp(opIndex, ops[i], opInfos[opIndex]);
468: actualGas += preGas - gasleft();
148: result += 1;
51: paymasterIdBalances[paymasterId] += msg.value;
58: paymasterIdBalances[msg.sender] -= amount;
128: paymasterIdBalances[extractedPaymasterId] -= actualGasCost;
The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.
function nonce() public view virtual returns (uint256);
function nonce() public view virtual override returns (uint256) {
function nonce(uint256 _batchId) public view virtual override returns (uint256) {
function entryPoint() public view virtual override returns (IEntryPoint) {
function domainSeparator() public view returns (bytes32) {
function getChainId() public view returns (uint256) {
function getNonce(uint256 batchId) public view returns (uint256) {
function init(address _owner, address _entryPointAddress, address _handler) public override initializer {
function execTransaction( Transaction memory _tx, uint256 batchId, FeeRefund memory refundInfo, bytes memory signatures ) public payable virtual override returns (bool success) {
function checkSignatures( bytes32 dataHash, bytes memory data, bytes memory signatures ) public view virtual {
function getTransactionHash( address to, uint256 value, bytes calldata data, Enum.Operation operation, uint256 targetTxGas, uint256 baseGas, uint256 gasPrice, uint256 tokenGasPriceFactor, address gasToken, address payable refundReceiver, uint256 _nonce ) public view returns (bytes32) {
function encodeTransactionData( Transaction memory _tx, FeeRefund memory refundInfo, uint256 _nonce ) public view returns (bytes memory) {
function getDeposit() public view returns (uint256) {
function addDeposit() public payable {
function withdrawDepositTo(address payable withdrawAddress, uint256 amount) public onlyOwner {
function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
function deployWallet(address _owner, address _entryPoint, address _handler) public returns(address proxy){
function setEntryPoint(IEntryPoint _entryPoint) public onlyOwner {
function deposit() public payable {
function withdrawTo(address payable withdrawAddress, uint256 amount) public onlyOwner {
function getDeposit() public view returns (uint256) {
function handleOps(UserOperation[] calldata ops, address payable beneficiary) public {
function handleAggregatedOps( UserOpsPerAggregator[] calldata opsPerAggregator, address payable beneficiary ) public {
function getUserOpHash(UserOperation calldata userOp) public view returns (bytes32) {
function getSenderAddress(bytes calldata initCode) public {
function getDepositInfo(address account) public view returns (DepositInfo memory info) {
function balanceOf(address account) public view returns (uint256) {
function depositTo(address account) public payable {
function addStake(uint32 _unstakeDelaySec) public payable {
function setFallbackHandler(address handler) public authorized {
function enableModule(address module) public authorized {
function disableModule(address prevModule, address module) public authorized {
function execTransactionFromModule( address to, uint256 value, bytes memory data, Enum.Operation operation ) public virtual returns (bool success) {
function execTransactionFromModuleReturnData( address to, uint256 value, bytes memory data, Enum.Operation operation ) public returns (bool success, bytes memory returnData) {
function isModuleEnabled(address module) public view returns (bool) {
function multiSend(bytes memory transactions) public payable {
function multiSend(bytes memory transactions) public payable {
function setEntryPoint(IEntryPoint _entryPoint) public onlyOwner {
function deposit() public virtual payable {
function withdrawTo(address payable withdrawAddress, uint256 amount) public virtual onlyOwner {
function getDeposit() public view returns (uint256) {
function deposit() public virtual override payable {
function depositFor(address paymasterId) public payable {
function withdrawTo(address payable withdrawAddress, uint256 amount) public override {
function getHash(UserOperation calldata userOp) public pure returns (bytes32) {
require() Should Be Used Instead Of assert()13: assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("biconomy.scw.proxy.implementation")) - 1));
require()/revert() Strings Longer Than 32 Bytes Cost Extra Gas74: require(msg.sender == address(entryPoint()), "account: not from EntryPoint");
110: require(_newOwner != address(0), "Smart Account:: new Signatory address cannot be zero");
128: require(_newEntryPoint != address(0), "Smart Account:: new entry point address cannot be zero");
342: require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024");
450: require(dest != address(0), "this action will burn your funds");
495: require(msg.sender == address(entryPoint()) || msg.sender == owner, "account: not Owner or EntryPoint");
38: require(success, "AA91 failed send to beneficiary");
213: require(paymasterAndData.length >= 20, "AA93 invalid paymasterAndData");
353: require(verificationGasLimit > gasUsedByValidateAccountPrepayment, "AA41 too little verificationGas");
62: require(_unstakeDelaySec >= info.unstakeDelaySec, "cannot decrease unstake time");
100: require(info.withdrawTime > 0, "must call unlockStake() first");
27: require(address(this) != multisendSingleton, "MultiSend should only be called via delegatecall");
49: require(!Address.isContract(paymasterId), "Paymaster Id can not be smart contract address");
50: require(paymasterId != address(0), "Paymaster Id can not be zero address");
57: require(amount <= currentBalance, "Insufficient amount to withdraw");
66: require(_newVerifyingSigner != address(0), "VerifyingPaymaster: new signer can not be zero address");
107: require(sigLength == 64 || sigLength == 65, "VerifyingPaymaster: invalid signature length in paymasterAndData");
108: require(verifyingSigner == hash.toEthSignedMessageHash().recover(paymasterData.signature), "VerifyingPaymaster: wrong signature");
109: require(requiredPreFund <= paymasterIdBalances[paymasterData.paymasterId], "Insufficient balance for paymaster id");
require() statements that use && saves gasInstead of using operator && on a single require. Using a two require can save more gas.
i.e.
for require(version == 1 && _bytecodeHash[1] == bytes1(0), "zf"); use:
require(version == 1); require(_bytecodeHash[1] == bytes1(0));
34: require(module != address(0) && module != SENTINEL_MODULES, "BSA101");
49: require(module != address(0) && module != SENTINEL_MODULES, "BSA101");
68: require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "BSA104");
To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array. The effect can be quite significant. As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.
113: _validatePrepayment(opIndex, ops[i], opInfos[opIndex], address(aggregator));
135: collected += _executeUserOp(opIndex, ops[i], opInfos[opIndex]);
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed
88: internal virtual returns (uint256 deadline);
307: uint8 v;
474: internalIncrementDeposit(refundAddress, refund);
49: internalIncrementDeposit(account, msg.value);
84: uint64 withdrawTime = uint64(block.timestamp) + info.unstakeDelaySec;
54: uint112 deposit;
56: uint112 stake;
57: uint32 unstakeDelaySec;
58: uint64 withdrawTime;
27: internalSetFallbackHandler(handler);
59: interfaceId == type(IERC165).interfaceId;
++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- And While-loopsThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP
100: for (uint256 i = 0; i < opasLen; i++) {
107: for (uint256 a = 0; a < opasLen; a++) {
112: for (uint256 i = 0; i < opslen; i++) {
unchecked blocks to save gasSolidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block
239: payment = handlePayment(startGas - gasleft(), refundInfo.baseGas, refundInfo.gasPrice, refundInfo.tokenGasPriceFactor, refundInfo.gasToken, refundInfo.refundReceiver);
267: uint256 requiredGas = startGas - gasleft(); 291: uint256 requiredGas = startGas - gasleft(); 372: uint256 requiredGas = startGas - gasleft();
347: _signer = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
267: uint256 requiredGas = startGas - gasleft(); 291: uint256 requiredGas = startGas - gasleft(); 372: uint256 requiredGas = startGas - gasleft();
55: uint256 actualGas = preGas - gasleft() + opInfo.preOpGas; 186: uint256 actualGas = preGas - gasleft() + opInfo.preOpGas;
317: missingAccountFunds = bal > requiredPrefund ? 0 : requiredPrefund - bal; 336: senderInfo.deposit = uint112(deposit - requiredPrefund); 338: gasUsedByValidateAccountPrepayment = preGas - gasleft();
354: uint256 gas = verificationGasLimit - gasUsedByValidateAccountPrepayment; 362: paymasterInfo.deposit = uint112(deposit - requiredPreFund);
418: uint256 gasUsed = preGas - gasleft(); 425: outOpInfo.preOpGas = preGas - gasleft() + userOp.preVerificationGas;
118: info.deposit = uint112(info.deposit - withdrawAmount);
Upgrade to the latest solidity version 0.8.17 to get additional gas savings.
All in-scope contracts
storage instead of memory saves gasWhen 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
17: bytes memory initCallData = initCode[20 :];
Entrypoint.sol
145: struct MemoryUserOp { 146: address sender; 147: uint256 nonce; 148: uint256 callGasLimit; 149: uint256 verificationGasLimit; 150: uint256 preVerificationGas; 151: address paymaster; 152: uint256 maxFeePerGas; 153: uint256 maxPriorityFeePerGas; 154: }
Nonce unlikely to reach max uint256. Can set to uint96 and thus saving 1 storage slot.
145: struct MemoryUserOp { 146: address sender; 147: uint96 nonce; 148: uint256 callGasLimit; 149: uint256 verificationGasLimit; 150: uint256 preVerificationGas; 151: address paymaster; 152: uint256 maxFeePerGas; 153: uint256 maxPriorityFeePerGas; 154: }
IStakeManager.sol
62: struct StakeInfo { 63: uint256 stake; 64: uint256 unstakeDelaySec; 65: }
stake is seen to be used in struct DepositInfo with uint112 and unstakeDelaySec with uint32.
Hence, we can save 1 storage slot by changing to:
62: struct StakeInfo { 63: uint112 stake; 64: uint32 unstakeDelaySec; 65: }
UserOperation.sol
19: struct UserOperation { 20: 21: address sender; 22: uint256 nonce; 23: bytes initCode; 24: bytes callData; 25: uint256 callGasLimit; 26: uint256 verificationGasLimit; 27: uint256 preVerificationGas; 28: uint256 maxFeePerGas; 29: uint256 maxPriorityFeePerGas; 30: bytes paymasterAndData; 31: bytes signature; 32: }
Nonce unlikely to reach max uint256. Can set to uint96 and thus saving 1 storage slot.
19: struct UserOperation { 20: 21: address sender; 22: uint96 nonce; 23: bytes initCode; 24: bytes callData; 25: uint256 callGasLimit; 26: uint256 verificationGasLimit; 27: uint256 preVerificationGas; 28: uint256 maxFeePerGas; 29: uint256 maxPriorityFeePerGas; 30: bytes paymasterAndData; 31: bytes signature; 32: }
#0 - c4-judge
2023-01-22T16:08:12Z
gzeon-c4 marked the issue as grade-a
#1 - c4-sponsor
2023-02-09T12:35:53Z
livingrockrises marked the issue as sponsor confirmed