Platform: Code4rena
Start Date: 02/06/2023
Pot Size: $100,000 USDC
Total HM: 15
Participants: 75
Period: 7 days
Judge: Picodes
Total Solo HM: 5
Id: 249
League: ETH
Rank: 25/75
Findings: 2
Award: $272.43
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: RaymondFam
Also found by: 0xWaitress, 0xhacksmithh, ChrisTina, DadeKuma, LaScaloneta, Rolezn, SAAJ, Sathish9098, T1MOH, bin2chen, btk, catellatech, ernestognw, fatherOfBlocks, hals, hunter_w3b, jaraxxus, matrix_0wl, mgf15, naman1778, niser93, solsaver, turvy_fuzz
18.5651 USDC - $18.57
payable.call() is a low-level method that can be used to send ether to a contract, but it has some limitations and risks as you've pointed out. One of the primary risks of using payable.call() is that it doesn't guarantee that the contract's payable function will be called successfully. This can lead to funds being lost or stuck in the contract
The contract does not have a payable callback The contract’s payable callback spends more than 2300 gas (which is only enough to emit something) The contract is called through a proxy which itself uses up the 2300 gas Use OpenZeppelin’s Address.sendValue() instead
FILE: 2023-06-stader/contracts/Auction.sol 131: (bool success, ) = payable(msg.sender).call{value: withdrawalAmount}('');
FILE: 2023-06-stader/contracts/SocializingPool.sol 91: (bool success, ) = payable(staderConfig.getStaderTreasury()).call{value: _rewardsData.protocolETHRewards} (''); 121: (success, ) = payable(operatorRewardsAddr).call{value: totalAmountETH}('');
FILE: 2023-06-stader/contracts/StaderStakePoolsManager.sol 102: (bool success, ) = payable(staderConfig.getUserWithdrawManager()).call{value: _amount}('');
Use OpenZeppelin’s Address.sendValue()
.transfer will relay 2300 gas and .call will relay all the gas. If the receive/fallback function from the recipient proxy contract has complex logic, using .transfer will fail, causing integration issues
FILE: 2023-06-stader/contracts/Auction.sol 114: if (!IERC20(staderConfig.getStaderToken()).transfer(staderConfig.getStaderTreasury(), _sdAmount)) { ``` https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/Auction.sol#LL114C8-L114C108 ```solidity FILE: 2023-06-stader/contracts/SDCollateral.sol 68: if (!IERC20(staderConfig.getStaderToken()).transfer(payable(operator), _requestedSD)) { ``` https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/SDCollateral.sol#LL68C8-L68C96 ### Recommended Mitigation Replace .transfer with .call. Note that the result of .call need to be checked. ## [L-3] Unbounded loop ### This instance not found by bot race. This is most important Unbounded loop logic ``nextOperatorId`` the value is only incremented . PermissionedNodeRegistry.allocateValidatorsAndUpdateOperatorId() will iterate all the OperatorId. Currently, ``nextOperatorId`` can grow indefinitely. E.g. there’s no maximum limit and there’s no functionality to remove OperatorId If the value grows too large, calling PermissionedNodeRegistry.allocateValidatorsAndUpdateOperatorId() might run out of gas and revert. ```solidity FILE: Breadcrumbs2023-06-stader/contracts/PermissionedNodeRegistry.sol 206: for (uint256 i = 1; i < nextOperatorId; ) { 488: for (uint256 i = 1; i < nextOperatorId; ) { ``` https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L488 ## ## [L-4] Inconsistency between the comment and the actual value assigned It appears that there is an inconsistency between the comment and the actual value assigned to the constant variable. The comment states that the value represents a 24-hour duration, but the assigned value of 7200 actually corresponds to a duration of 2 hours ```diff FILE: Breadcrumbs2023-06-stader/contracts/Auction.sol - 22: uint256 public constant MIN_AUCTION_DURATION = 7200; // 24 hours + 22: uint256 public constant MIN_AUCTION_DURATION = 7200; // 2 hours ``` https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/Auction.sol#LL22C5-L22C69 ## ## [L-5] Resolve the warning to avoid unexpected behavior ``` Warning (3628): Warning: This contract has a payable fallback function, but no receive ether function. Consider adding a receive ether function. --> contracts/VaultProxy.sol:8:1: | 8 | contract VaultProxy is IVaultProxy { | ^ (Relevant source part starts here and spans across multiple lines). Note: The payable fallback function is defined here. --> contracts/VaultProxy.sol:41:5: | 41 | fallback(bytes calldata _input) external payable returns (bytes memory) { | ^ (Relevant source part starts here and spans across multiple lines). ``` ## ## [L-6] Consider using upgradable ERC20 contracts Upgradable ERC20 contracts provide flexibility and allow for future upgrades and improvements to the contract's functionality without disrupting the existing ecosystem. https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/Auction.sol#L9 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/SDCollateral.sol#L14 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/SocializingPool.sol#L15 ## ## [L-7] nextValidatorId inrement should be placed on top of the for loop When increase bottom of the for loop nextValidatorId is increased even next iteration condition check is false. ```solidity FILE: 2023-06-stader/contracts/PermissionedNodeRegistry.sol 173: validatorIdByPubkey[_pubkey[i]] = nextValidatorId; 174: validatorIdsByOperatorId[operatorId].push(nextValidatorId); 175: emit AddedValidatorKey(msg.sender, _pubkey[i], nextValidatorId); 176: nextValidatorId++; 177: unchecked { 178: ++i; 179: } ``` https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L173-L179 Recommended Mitigation: nextValidatorId value should be increased when enter to new iterations ## ## [L-8] Missing Event for initialize Description: Events help non-contract tools to track changes, and events prevent users from being surprised by changes Issuing event-emit during initialization is a detail that many projects skip ```solidity FILE: 2023-06-stader/contracts/PoolSelector.sol function initialize(address _admin, address _staderConfig) external initializer { UtilLib.checkNonZeroAddress(_admin); UtilLib.checkNonZeroAddress(_staderConfig); __AccessControl_init_unchained(); poolAllocationMaxSize = 50; staderConfig = IStaderConfig(_staderConfig); _grantRole(DEFAULT_ADMIN_ROLE, _admin); } ``` https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolSelector.sol#L36-L43 ```solidity FILE: Breadcrumbs2023-06-stader/contracts/VaultProxy.sol function initialise( bool _isValidatorWithdrawalVault, uint8 _poolId, uint256 _id, address _staderConfig ) external { if (isInitialized) { revert AlreadyInitialized(); } UtilLib.checkNonZeroAddress(_staderConfig); isValidatorWithdrawalVault = _isValidatorWithdrawalVault; isInitialized = true; poolId = _poolId; id = _id; staderConfig = IStaderConfig(_staderConfig); owner = staderConfig.getAdmin(); } ``` https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/VaultProxy.sol#LL20C5-L36C6 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/factory/VaultFactory.sol#L23-L32 Recommendation: Add Event-Emit ## # NON CRITICAL ## [NC-1] Tokens accidentally sent to the contract cannot be recovered ### Context contracts/staking/NeoTokyoStaker.sol: It can’t be recovered if the tokens accidentally arrive at the contract address, which has happened to many popular projects, so I recommend adding a recovery code to your critical contracts. ### Recommended Mitigation Steps Add this code: /** * @notice Sends ERC20 tokens trapped in contract to external address * @dev Onlyowner is allowed to make this function call * @param account is the receiving address * @param externalToken is the token being sent * @param amount is the quantity being sent * @return boolean value indicating whether the operation succeeded. * */ function rescueERC20(address account, address externalToken, uint256 amount) public onlyOwner returns (bool) { IERC20(externalToken).transfer(account, amount); return true; } } ## [NC-2] Use SMTChecker The highest tier of smart contract behavior assurance is formal mathematical verification. All assertions that are made are guaranteed to be true across all inputs → The quality of your asserts is the quality of your verification. https://twitter.com/0xOwenThurm/status/1614359896350425088?t=dbG9gHFigBX85Rv29lOjIQ&s=19 ## [NC-3] According to the syntax rules, use => mapping ( instead of => mapping( using spaces as keyword https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolSelector.sol#L23 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderOracle.sol#L44-L47 ## ## [NC-4] Assembly Codes Specific – Should Have Comments Since this is a low level language that is more difficult to parse by readers, include extensive documentation, comments on the rationale behind its use, clearly explaining what each assembly instruction does. This will make it easier for users to trust the code, for reviewers to validate the code, and for developers to build on or update the code. Note that using Assembly removes several important security features of Solidity, which can make the code more insecure and more error-prone. https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L511-L513 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L581-L583 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L605-L607 ## ## [NC-5] NOT USING THE NAMED RETURN VARIABLES ANYWHERE IN THE FUNCTION IS CONFUSING Consider changing the variable to be an unnamed one https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolSelector.sol#L76-L79 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolSelector.sol#L50-L54 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L720 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L513 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L680 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L93 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L432 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L692 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L648 ## ## [NC-6] NATSPEC COMMENTS SHOULD BE INCREASED IN CONTRACTS It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability. (https://docs.soliditylang.org/en/v0.8.15/natspec-format.html) ### Recommendation NatSpec comments should be increased in Contracts ## ## [NC-7] Use a more recent version of solidity Use at least solidity version 0.8.17 INSTANCE ALL CONTRACTS ## ## [NC-8] For functions, follow Solidity standard naming conventions (internal function style rule) Description The bellow codes don’t follow Solidity’s standard naming convention, internal and private functions and variables : the mixedCase format starting with an underscore (_mixedCase starting with an underscore) https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L680 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L687-L692 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L720 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L732 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L738 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L747 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L752 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L758 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedPool.sol#L279 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedPool.sol#L283 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedPool.sol#L288-L293 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L598-L602 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L618 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L625 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L633 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessNodeRegistry.sol#L643-L648 ## ## [NC-9] Critical changes should use two-step procedure Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two-step procedure on the critical functions. Consider adding a two-steps pattern on critical changes to avoid mistakenly transferring ownership of roles or critical functionalities to the wrong address https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedPool.sol#L236-L245 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessPool.sol#L66-L75 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolSelector.sol#L147-L151 ## ## [NC-10] add a timelock to critical functions https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedPool.sol#L236-L245 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionlessPool.sol#L66-L75 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolSelector.sol#L147-L151 ## ## [NC-11] No Same value input control https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolSelector.sol#L140-L144 ## ## [NC-12] Use Fuzzing Test for math code bases I recommend fuzzing testing in math code structures, Recommendation: Use should fuzzing test like Echidna. Fuzzing is not easy, the tools are rough, and the math is hard, but it is worth it. Fuzzing gives me a level of confidence in my smart contracts that I didn’t have before. Relying just on unit testing anymore and poking around in a testnet seems reckless now. https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05 ## ## [NC-13] Project Upgrade and Stop Scenario should be At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an " EMERGENCY STOP (CIRCUIT BREAKER) PATTERN ". https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol
#0 - Picodes
2023-06-14T07:06:53Z
Is it me or the 2 issues are use .call and do not use .call ?
#1 - c4-judge
2023-06-14T07:06:57Z
Picodes marked the issue as grade-c
#2 - sathishpic22
2023-07-04T12:56:24Z
@Picodes
I think the findings 1 is understood in wrong way.
My suggestion is payable(address).call should be avoided not normal .call
primary risks of using payable(address).call() is that it doesn't guarantee that the contract's payable function will be called successfully. This can lead to funds being lost or stuck in the contract.
This finding is accepted in many contests as LOW
#3 - c4-judge
2023-07-07T12:32:13Z
Picodes marked the issue as grade-b
#4 - Picodes
2023-07-07T12:33:05Z
Thanks for clarifying.
🌟 Selected for report: JCN
Also found by: 0x70C9, 0xSmartContract, 0xWaitress, 0xhacksmithh, DavidGiladi, K42, LaScaloneta, Rageur, Raihan, SAAJ, SAQ, SM3_SS, Sathish9098, Tomio, bigtone, c3phas, ernestognw, etherhood, koxuan, matrix_0wl, mgf15, naman1778, niser93, petrichor, piyushshukla, sebghatullah, shamsulhaq123
253.86 USDC - $253.86
All the gas optimizations are determined based opcodes and sample tests.
| Count | Issues | Instances | Gas Saved |
|---|---|---|---|
| [G-1] | Using bools for storage incurs overhead | 7 | 119700 |
| [G-2] | Using storage instead of memory for structs/arrays saves gas | 4 | 8400 |
| [G-3] | External function calls should be cached outside the loop | 3 | PerCall (2100 gas) |
| [G-4] | Avoid emitting storage variable when stack variable available | 4 | 400 |
| [G-5] | Refactor the for loop to avoid unwanted variable creation inside the loop | 1 | 13 |
| [G-6] | Cache the state variable out side the loop | 1 | PerCall (100 gas) |
| [G-7] | Caching global variables is more expensive than using the actual variable (use msg.sender instead of caching it) | 3 | 39 |
| [G-8] | Repack the state variable to avoid on extra slot | 1 | 2000 |
Results : INSTANCES 7, Saves 119700 GAS
// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
FILE: Breadcrumbs2023-06-stader/contracts/PermissionedNodeRegistry.sol 54: mapping(address => bool) public override permissionList;
FILE: 2023-06-stader/contracts/StaderOracle.sol 18: bool public override erInspectionMode; 19: bool public override isPORFeedBasedERData; 44: mapping(address => bool) public override isTrustedNode; 45: mapping(bytes32 => bool) private nodeSubmissionKeys;
FILE: 2023-06-stader/contracts/VaultProxy.sol 10: bool public override isValidatorWithdrawalVault; 11: bool public override isInitialized;
Results : INSTANCES 3, Saves 8400 GAS
When 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.
storage variables can potentially save gas by eliminating the cost of copying the struct from storage to memory4200 GASFILE: 2023-06-stader/contracts/PermissionedNodeRegistry.sol - 733: Validator memory validator = validatorRegistry[_validatorId]; + 733: Validator storage validator = validatorRegistry[_validatorId]; - 739: Validator memory validator = validatorRegistry[_validatorId]; + 739: Validator storage validator = validatorRegistry[_validatorId];
4200 GASFILE: 2023-06-stader/contracts/PermissionlessNodeRegistry.sol - 692: Validator memory validator = validatorRegistry[_validatorId]; + 692: Validator storage validator = validatorRegistry[_validatorId]; - 702: Validator memory validator = validatorRegistry[_validatorId]; + 702: Validator storage validator = validatorRegistry[_validatorId];
Results : INSTANCES 3, Saves 2100 GAS per call
Results : Saves Approximately 2100 gas for single call
When caching external call values, it's generally more efficient to perform the external call outside of any loops to avoid unnecessary redundant calls.
In terms of opcode savings, caching the external call result outside the loop can eliminate repeated opcodes such as CALL that would have been used for the external calls within the loop. This can reduce the overall gas consumption.
staderConfig.getStakedEthPerNode() external call should be cached outside the loop to avoid redundant calls saves at least 700 GAS for every iterations.FILE: 2023-06-stader/contracts/StaderStakePoolsManager.sol + uint256 stakedEthPerNode=staderConfig.getStakedEthPerNode(); 231: uint256 poolCount = poolIdArray.length; 232: for (uint256 i = 0; i < poolCount; i++) { 233: uint256 validatorToDeposit = selectedPoolCapacity[i]; 235: if (validatorToDeposit == 0) { 236: continue; 237: } 238: address poolAddress = IPoolUtils(poolUtils).poolAddressById(poolIdArray[i]); - 239: uint256 poolDepositSize = staderConfig.getStakedEthPerNode() - + 239: uint256 poolDepositSize = stakedEthPerNode - 240: IPoolUtils(poolUtils).getCollateralETH(poolIdArray[i]);
INodeRegistry((staderConfig).getPermissionlessNodeRegistry()).POOL_ID() external call should be cached outside the loop to avoid redundant calls saves 1400 GAS for every iterations.FILE: Breadcrumbs2023-06-stader/contracts/PermissionlessPool.sol + uint8 poolId= INodeRegistry((staderConfig).getPermissionlessNodeRegistry()).POOL_ID() ; 94: for (uint256 i; i < pubkeyCount; ) { 95: address withdrawVault = IVaultFactory(vaultFactory).computeWithdrawVaultAddress( - 96: INodeRegistry((staderConfig).getPermissionlessNodeRegistry()).POOL_ID(), + 96: poolId, 97: _operatorId, 98: _operatorTotalKeys + i 99: );
Results : INSTANCES 4, Saves 400 GAS
When accessing a storage variable, an opcode like SLOAD is used to read the value from storage into the stack. This incurs a gas cost based on the complexity of accessing storage and the amount of data being loaded
Stack variables, on the other hand, do not require the use of SLOAD or SSTORE opcodes. They are directly manipulated within the function's stack frame, which is a more efficient operation in terms of gas usage
_duration stack variable should be used instead of storage variable duration saves 1 SLOD (100 gas ) FILE: 2023-06-stader/contracts/Auction.sol 147: duration = _duration; - 148: emit AuctionDurationUpdated(duration); + 148: emit AuctionDurationUpdated(_duration);
_nextQueuedValidatorIndex stack variable should be used instead of storage variable nextQueuedValidatorIndex saves 1 SLOD (100 gas )FILE: Breadcrumbs2023-06-stader/contracts/PermissionlessNodeRegistry.sol 270: nextQueuedValidatorIndex = _nextQueuedValidatorIndex; - 271: emit UpdatedNextQueuedValidatorIndex(nextQueuedValidatorIndex); + 271: emit UpdatedNextQueuedValidatorIndex(_nextQueuedValidatorIndex);
_maxNonTerminalKeyPerOperator stack variable should be used instead of storage variable maxNonTerminalKeyPerOperator saves 1 SLOD (100 gas )FILE: Breadcrumbs2023-06-stader/contracts/PermissionlessNodeRegistry.sol 338: maxNonTerminalKeyPerOperator = _maxNonTerminalKeyPerOperator; - 339: emit UpdatedMaxNonTerminalKeyPerOperator(maxNonTerminalKeyPerOperator); + 339: emit UpdatedMaxNonTerminalKeyPerOperator(_maxNonTerminalKeyPerOperator);
_erChangeLimit stack variable should be used instead of storage variable erChangeLimit saves 1 SLOD (100 gas )FILE: Breadcrumbs2023-06-stader/contracts/StaderOracle.sol 535: erChangeLimit = _erChangeLimit; - 536: emit UpdatedERChangeLimit(erChangeLimit); + 536: emit UpdatedERChangeLimit(_erChangeLimit);
Results : INSTANCES 1, Saves 13 GAS per call
The variable pubkeyRoot is declared within each iteration of the for loop, but it is only used once. This results in unnecessary variable creation.This refactoring helps to optimize gas costs and code readability by reducing the number of variable declarations within the loop, especially for variables that are only used once.
UtilLib.getPubkeyRoot(_mapd.sortedPubkeys[i]) can apply value directly instead of cache with local variable. For every iteration can saves 15-20 GAS FILE: 2023-06-stader/contracts/StaderOracle.sol 485: for (uint256 i; i < keyCount; ) { - 486: bytes32 pubkeyRoot = UtilLib.getPubkeyRoot(_mapd.sortedPubkeys[i]); - 487: missedAttestationPenalty[pubkeyRoot]++; + 487: missedAttestationPenalty[UtilLib.getPubkeyRoot(_mapd.sortedPubkeys[i])]++;
Results : INSTANCES 1, Saves 100 GAS per call
1 SLOD 100 GASFILE: 2023-06-stader/contracts/Penalty.sol + IStaderConfig public _staderConfig = staderConfig ; 100: for (uint256 i; i < reportedValidatorCount; ) { 101: if (UtilLib.getValidatorSettleStatus(_pubkey[i], _staderConfig)) {
Results : INSTANCES 3, Saves 39 GAS
Caching global variable consumes extra 13 gas
msg.sender cache global variables is more expensive. This cost extra 13 GASFILE: Breadcrumbs2023-06-stader/contracts/SDCollateral.sol INSTANCE 1: - 59: address operator = msg.sender; - 60: uint256 opSDBalance = operatorSDBalance[operator]; + 60: uint256 opSDBalance = operatorSDBalance[msg.sender]; 61: - 62: if (opSDBalance < getOperatorWithdrawThreshold(operator) + _requestedSD) { + 62: if (opSDBalance < getOperatorWithdrawThreshold(msg.sender) + _requestedSD) { 63: revert InsufficientSDToWithdraw(opSDBalance); 64: } - 65: operatorSDBalance[operator] -= _requestedSD; + 65: operatorSDBalance[msg.sender] -= _requestedSD; 66: 67: // cannot use safeERC20 as this contract is an upgradeable contract - 68: if (!IERC20(staderConfig.getStaderToken()).transfer(payable(operator), _requestedSD)) { + 68: if (!IERC20(staderConfig.getStaderToken()).transfer(payable(msg.sender), _requestedSD)) { 69: revert SDTransferFailed(); 70: } 71: - 72: emit SDWithdrawn(operator, _requestedSD); + 72: emit SDWithdrawn(msg.sender, _requestedSD); 73: } INSTANCE: 2: - 44: address operator = msg.sender; - 45: operatorSDBalance[operator] += _sdAmount; + 45: operatorSDBalance[msg.sender] += _sdAmount; 46: - 47: if (!IERC20(staderConfig.getStaderToken()).transferFrom(operator, address(this), _sdAmount)) { + 47: if (!IERC20(staderConfig.getStaderToken()).transferFrom(msg.sender, address(this), _sdAmount)) { 48: revert SDTransferFailed(); 49: } 50: - 51: emit SDDeposited(operator, _sdAmount); + 51: emit SDDeposited(msg.sender, _sdAmount);
msg.sender cache global variables is more expensive. This cost extra 13 GASFILE: Breadcrumbs2023-06-stader/contracts/SocializingPool.sol - 113: address operator = msg.sender; - 114: (uint256 totalAmountSD, uint256 totalAmountETH) = _claim(_index, operator, _amountSD, _amountETH, _merkleProof); + 114: (uint256 totalAmountSD, uint256 totalAmountETH) = _claim(_index, msg.sender, _amountSD, _amountETH, _merkleProof); - 116: address operatorRewardsAddr = UtilLib.getOperatorRewardAddress(operator, staderConfig); + 116: address operatorRewardsAddr = UtilLib.getOperatorRewardAddress(msg.sender, staderConfig);
Results : INSTANCES 1, Saves 2000 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 can also be cheaper
_erChangeLimit > ER_CHANGE_MAX_BPS as per this condition check the erChangeLimit not exceeds 10000. So uint64 alone more than enough instead of uint256
FILE: 2023-06-stader/contracts/StaderOracle.sol 18: bool public override erInspectionMode; 19: bool public override isPORFeedBasedERData; 20: SDPriceData public lastReportedSDPriceData; + 28: uint64 public override erChangeLimit; 21: IStaderConfig public override staderConfig; 22: ExchangeRate public inspectionModeExchangeRate; 23: ExchangeRate public exchangeRate; 24: ValidatorStats public validatorStats; 25: 26: uint256 public constant MAX_ER_UPDATE_FREQUENCY = 7200 * 7; // 7 days 27: uint256 public constant ER_CHANGE_MAX_BPS = 10000; - 28: uint256 public override erChangeLimit;
#0 - c4-judge
2023-06-14T05:55:10Z
Picodes marked the issue as grade-b
#1 - sathishpic22
2023-07-04T12:51:04Z
@Picodes
I think my reports may save more than some of the grade a reports. Yes other wardens sent more findings than me still those mostly informational and less gas optimized findings. Even some wardens sent wrong instances still they got grade a.
https://github.com/code-423n4/2022-06-stader-findings/issues/413, https://github.com/code-423n4/2022-06-stader-findings/issues/382, https://github.com/code-423n4/2022-06-stader-findings/issues/378
Yes, My title suggests that some these findings are already found in bot race. But i have checked all my findings this instances are not found in bot race. In other wardens case the missing instances are accepted i don't what is wrong in my case. All other wardens also sent missing instances.
I have only focused more impact findings than less impact findings. The point is how much gas saved that particular reports instead of the findings count. You can check my instances.
[G-1] | Using bools for storage incurs overhead | 7 | 119700 [G-2] Using storage instead of memory for structs/arrays saves gas - 8400 gas [G-3] External function calls should be cached outside the loop - 2100 gas [G-4] Avoid emitting storage variable when stack variable available - 400 gas [G-6] Cache the state variable out side the loop - 100 gas [G-8] Repack the state variable to avoid on extra slot - 2000 gas
If ignore bool findings still my reports saves - 12000 gas which is higher than some of the grade a reports
#2 - sathishpic22
2023-07-04T14:45:52Z
Even JCN saved more gas via
Structs can be packed into fewer storage slots, State variables can be packed into fewer storage slots saved 38000 gas. The problem is he simply down casting from uint256 to some other types without sufficiently proving that down casting not affect the protocol. In that way anyone can simply downcast and save more SLOTs.
If we down casting any state variable warden must prove that down casting not affect protocols.
Also half of his findings are missing instances in bot races. This is my thoughts . If anything wrong or any comments about my findings I am ready to accept yours comments.
Thank you
#3 - sathishpic22
2023-07-07T09:15:53Z
@Picodes
#4 - Picodes
2023-07-07T09:33:35Z
@sathishpic22 don't forget to reference your comments during Q&As in the discussion as the judge can't properly keep track of what's happening on the 400+ issues otherwise. My apologies for missing your comments.
I still think your report is quite far from the best reports in terms of formatting and originality of the findings. However, you are right that there is no out of scope issues and it adds some value (aside from the bool thing which in my opinion is not very interesting)
#5 - c4-judge
2023-07-07T09:33:40Z
Picodes marked the issue as grade-a