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: 26/92
Findings: 2
Award: $543.70
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
During the audit, 2 low and 10 non-critical issues were found.
| â„– | Title | Risk Rating | Instance Count |
|---|---|---|---|
| L-1 | Missing check for zero address | Low | 2 |
| L-2 | Open TODO | Low | 1 |
| NC-1 | Order of Functions | Non-Critical | 18 |
| NC-2 | Order of Layout | Non-Critical | 13 |
| NC-3 | Public functions can be external | Non-Critical | 10 |
| NC-4 | Open question | Non-Critical | 1 |
| NC-5 | Unused event | Non-Critical | 1 |
| NC-6 | Typos | Non-Critical | 7 |
| NC-7 | Constants may be used | Non-Critical | 24 |
| NC-8 | Missing NatSpec | Non-Critical | 10 |
| NC-9 | No space between the control structures | Non-Critical | 13 |
| NC-10 | Maximum line length exceeded | Non-Critical | 63 |
If address(0x0) is set it may cause the contract to revert or work wrong.
Add checks.
Resolve issue.
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:
function receive() is not right after the constructor:
external functions after/between public:
public function after internal:
Reorder functions where possible.
According to Order of Layout, inside each contract, library or interface, use the following order:
events before state variables:
modifier should be before constructor:
Place events after state variables and before modifiers.
Place modifiers before constructor.
If functions are not called by the contract where they are defined, they can be declared external.
Make public functions external, where possible.
Remove event or use it.
require(numOfTokens == _amounts.length, "Inconsisent array length"); => Inconsistent/// @notice Utility function that determins whether an LP can be burned for dETH if the associated derivatives have been minted => determines// register validtor initals for each of the KNOTs => validator// register validtor initals for each of the KNOTs => initials/// @param _newLPToken Instane of the new LP token (to be minted) => Instance// mint LP tokens for the depoistor with 1:1 ratio of LP tokens and ETH supplied => depositor// mint LP tokens for the depoistor with 1:1 ratio of LP tokens and ETH supplied => depositorConstants may be used instead of literal values.
for 32:
for 24:
require(dETHBalance >= 24 ether, "Nothing to withdraw");redemptionValue = (dETHDetails.savETHBalance * _amount) / 24 ether;require(_amount >= 24 ether, "Amount cannot be less than 24 ether");maxStakingAmountPerValidator = 24 ether;savETHVault.withdrawETHForStaking(smartWallet, 24 ether);require(savETHVaultLP.totalSupply() == 24 ether, "KNOT must have 24 ETH in savETH vault");require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");for 12:
if (totalStaked == 12 ether) revert KnotIsFullyStakedWithFreeFloatingSlotTokens();if (_sETHAmount + totalStaked > 12 ether) revert InvalidStakeAmount();uint256 stakeAmount = 12 ether;for 4:
totalShares += 4 ether;require(_amount >= 4 ether, "Amount cannot be less than 4 ether");maxStakingAmountPerValidator = 4 ether;if (currentSlashedAmount < 4 ether) {balance * unprocessedForKnot / (4 ether - currentSlashedAmount);if (currentSlashedAmount < 4 ether) {return (_ethSinceLastUpdate * PRECISION) / (numberOfRegisteredKnots * 4 ether);require(associatedSmartWallet.balance >= 4 ether, "Insufficient balance");4 etherrequire(msg.value == len * 4 ether, "Insufficient ether provided");stakingFundsVault.withdrawETH(smartWallet, 4 ether);require(associatedSmartWallet.balance >= 4 ether, "Smart wallet balance must be at least 4 ether");require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether");Define constant variables, especially for repeated values.
NatSpec is missing for 10 functions in 4 contracts.
Add NatSpec for all functions.
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.
if(validatorStatus == IDataStructures.LifecycleStatus.TOKENS_MINTED) {if(!dETHDetails.withdrawn) {for(uint256 i; i < _blsPubKeys.length; ++i) {if(smartWallet == address(0)) {if(smartWalletRepresentative[smartWallet] != address(0)) {for(uint256 i; i < len; ++i) {if(representative != address(0)) {if(numberOfKnots == 0) {if(stakedKnotsOfSmartWallet[smartWallet] == 0) {if(enableWhitelisting) {if(!_isEnabled && smartWalletRepresentative[_smartWallet] != address(0)) {else if(_isEnabled && smartWalletRepresentative[_smartWallet] == address(0)) {if(address(lpToken) != address(0)) {Change:
if(...) { ... }
to:
if (...) { ... }
According to Style Guide, maximum suggested line length is 120 characters.
GiantLP.sol:
LPToken.sol:
SavETHVault.sol:
GiantMevAndFeesPool.sol:
StakingFundsVault.sol:
Syndicate.sol:
LiquidStakingManager.sol:
ETHPoolLPFactory.sol:
Make the lines shorter.
#0 - c4-judge
2022-12-02T22:10:37Z
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
During the audit, 5 gas issues were found.
Total savings are more than 2700.
| â„– | Title | Instance Count | Saved |
|---|---|---|---|
| G-1 | Use unchecked blocks for incrementing i | 38 | 1330 |
| G-2 | Use calldata instead of memory for read-only arguments | 15 | 900 |
| G-3 | Using storage pointer to bytes is cheaper than using memory | 13 | |
| G-4 | Cache state variables instead of reading them from storage multiple times | 3 | 300 |
| G-5 | Use local variable cache instead of accessing mapping or array multiple times | 4 | 160 |
In Solidity 0.8+, there’s a default overflow and underflow check on unsigned integers. In the loops, "i" 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*38 = 1330
calldata instead of memory for read-only argumentsSince Solidity v0.6.9, memory and calldata are allowed in all functions regardless of their visibility type (See "Calldata Variables" section here).
When function arguments should not be modified, it is cheaper to use calldata.
Consider using calldata where possible.
This saves at least 60 gas per iteration.
So, ~60*15 = 900
storage pointer to bytes is cheaper than using memoryChange:
bytes memory x = y;
to:
bytes storage x = y
This saves ~ X.
Memory read is much cheaper than storage read.
dao)This saves ~100.
So, ~100*3 = 300
It saves gas due to not having to perform the same key’s keccak256 hash and/or offset recalculation.
isNoLongerPartOfSyndicate[_blsPubKey])smartWalletRepresentative[smartWallet])smartWalletRepresentative[smartWallet])smartWalletRepresentative[smartWallet])This saves ~40.
So, ~40*4 = 160
#0 - c4-judge
2022-12-02T22:10:52Z
dmvt marked the issue as grade-b