Platform: Code4rena
Start Date: 11/11/2022
Pot Size: $90,500 USDC
Total HM: 52
Participants: 92
Period: 7 days
Judge: LSDan
Total Solo HM: 20
Id: 182
League: ETH
Rank: 25/92
Findings: 3
Award: $546.83
🌟 Selected for report: 0
🚀 Solo Findings: 0
3.1274 USDC - $3.13
Judge has assessed an item in Issue #38 as M risk. The relevant finding follows:
LiquidStakingManager.sol#L280
#0 - c4-judge
2022-11-29T15:51:15Z
dmvt marked the issue as duplicate of #67
#1 - c4-judge
2022-11-30T11:40:17Z
dmvt marked the issue as satisfactory
#2 - c4-judge
2022-11-30T11:40:22Z
dmvt marked the issue as partial-50
#3 - C4-Staff
2022-12-21T00:11:18Z
JeeberC4 marked the issue as duplicate of #378
🌟 Selected for report: 0xSmartContract
Also found by: 0x4non, 0xNazgul, 0xRoxas, 0xdeadbeef0x, 0xmuxyz, 9svR6w, Awesome, Aymen0909, B2, Bnke0x0, CloudX, Deivitto, Diana, Franfran, IllIllI, Josiah, RaymondFam, ReyAdmirado, Rolezn, Sathish9098, Secureverse, SmartSek, Trust, Udsen, a12jmx, aphak5010, brgltd, bulej93, c3phas, ch0bu, chaduke, chrisdior4, clems4ever, cryptostellar5, datapunk, delfin454000, fs0c, gogo, gz627, hl_, immeas, joestakey, lukris02, martin, nogo, oyc_109, pashov, pavankv, peanuts, pedr02b2, rbserver, rotcivegaf, sahar, sakman, shark, tnevler, trustindistrust, zaskoh, zgo
475.5642 USDC - $475.56
Instane --> Instance
depoistor --> depositor
trigerringAddress --> triggeringAddress
admiting --> admitting
Unrecognised --> Unrecognized
validtor --> validator
initals --> initials
overriden --> overridden
customise --> customize
Inconsisent --> Inconsistent
determins --> determines
publice --> public
collatearlized --> collateralized
amongs --> amongst
Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
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.
Even assembly can benefit from using readable constants instead of hex/numeric literals
the require check will always be false so it the function will not do what it was designed to do and it will always revert Unnecessary update to same status
MODULO
Usually lines in source code are limited to 80 characters. Its advised to keep lines lower than 120 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
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
Using very old versions of Solidity prevents benefits of bug fixes and newer security checks. Using the latest versions might make contracts susceptible to undiscovered compiler bugs
Whenever a function is not being called internally in the code, it can be easily declared as external. While the entire code base have explicit visibilities for every function, some of them can be changed to be external.
totalRewardsReceived
isKnotDeregistered
deployNewLiquidStakingDerivativeNetwork
withdrawETHForStaking
withdrawETH
Earlier versions of solidity can use uint<n>(-1) instead. Expressions not including the - 1 can often be re-written to accommodate the change (e.g. by using a > rather than a >=, which will also save some gas)
If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth))). Having no access control on the function means that someone may send Ether to the contract, and have no way to get anything back out, which is a loss of funds
There are multiple occasions where certain numbers have been hardcoded, either in variables or in the code itself. Large numbers can become hard to read.
starting from right put underscores every 3 numbers
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract’s value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don’t get out of sync. https://medium.com/coinmonks/gas-cost-of-solidity-library-functions-dbe0cedd4678
many require()s are being used twice but some are used over 3 times:
require(smartWallet != address(0), "No smart wallet"); // used 4 times
Use abi.encode instead
#0 - c4-judge
2022-11-29T15:50:28Z
dmvt marked the issue as grade-a
🌟 Selected for report: IllIllI
Also found by: 0xSmartContract, Awesome, Aymen0909, CloudX, Deivitto, ReyAdmirado, Saintcode_, bharg4v, brgltd, btk, c3phas, chrisdior4, ignacio, imare, lukris02, skyle, tnevler
68.1383 USDC - $68.14
Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmaccess (100 gas) with a PUSH32 (3 gas).
If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables are also cheaper.
put it near one of the state vars that have less than 32 bytes (like address ones)
Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.
consider putting them with other mappings that with them would be under 32byte to save 20000 gas each
here
more possibilities here if checked carefully
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
because it if statement will be true cache transferHookProcessor first
cache before if statement lpTokenETH
cache enableWhitelisting before emit
cache smartWalletRepresentative[smartWallet]before if statement, highly possible gas save
gatekeeper
stakehouse
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if statementrequire(a <= b); x = b - a => require(a <= b); unchecked { x = b - a } if(a <= b); x = b - a => if(a <= b); unchecked { x = b - a } this will stop the check for overflow and underflow so it will save gas
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves gas
also making 2 stack variables without being used
++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-loopsIn 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.
Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas
&& saves gasthis will have a large deployment gas cost but with enough runtime calls the split require version will be 3 gas cheaper
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution.
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
_onWithdraw(its inherited but still used once in the whole project)
_assertUserHasEnoughGiantLPToClaimVaultLP
_onDepositETH(its inherited but still used once in the whole project)
_isNodeRunnerValid
_stake
_joinLSDNStakehouse
_createLSDNStakehouse
_initSavETHVault
_initStakingFundsVault
_calculateCommission
_assertEtherIsReadyForValidatorStaking
_beforeTokenTransfer
_afterTokenTransfer
If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is cheaper in solidity.
Contracts are allowed to override their parents’ functions and change the visibility from external to public and can save gas by doing so.
totalRewardsReceived
isKnotDeregistered
deployNewLiquidStakingDerivativeNetwork
withdrawETHForStaking
withdrawETH
This will save near 97 gas
if (<x> == true) => if (<x>)
if (<x> == false) => if (!<x>)
require(<x> == true) => require(<x>)
require(<x> == false) => require(!<x>)
#0 - c4-judge
2022-11-29T15:47:05Z
dmvt marked the issue as grade-b