Stader Labs - etherhood's results

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L270 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L285

Vulnerability details

Impact

Corruption of oracle data

Proof of Concept

Block for lastReportedSDPriceData = 7200 Let current block = 21601 Now StaderOracle will have data for 14400 and 21600 both blocks being pushed by nodes and in prices array it will be all mixed up. Also, As soon as 14400 block is finalised data for block 21600 is all lost as well.

Tools Used

Recommended Mitigation Steps

add if (_sdPriceData.reportingBlockNumber == getSDPriceReportableBlock()) to ensure it is always latest reportable block data add mapping(uint256 => uint256[]) blockPrices to store prices array separately for each block being reported to avoid mixing and corruption of data or have uint256 currentEpochBlock so that when new block data is pushed, previous data is deleting before pushing new data

if(_sdPriceData.reportingBlockNumber!=currentEpochBlock){
   delete prices;
}




## Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L637

Vulnerability details

Impact

Stale data of exchange rate data being used protocol, resulting in loss in terms of ETHX being minted at stale exchange rate (which won't have accounted for new reward)

Recommended Mitigation Steps

Check lastUpdatedAt timestamp and that it is within acceptable range of data update frequency when PORFeed is being used

Assessed type

Other

• In code at https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L124 staderConfig.getStakePoolManager() is being called again after calling same function and checking if vault is healthy, this line can be bumped before that check, poolManager being used in check statement directly will save gas cost for calling same function again ~ 1300 gas
• At https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L145, totalEthRequestedForWithdraw can be calculated and summed in loop and then subtracted at end of loop, rather than writing to storage during each iteration, it would (n-1)*100 gas
• https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L241 can be moved out of loop, and if any deposit is made can be updated, will save (poolCount - 1)*100 gas
• In function https://github.com/code-423n4/2023-06-stader/blob/main/contracts/SocializingPool.sol#L61, totalOperatorETHRewardsRemaining,totalOperatorSDRewardsRemaining is being read 3 times from storage and then incremented, which can instead be cached and then post update stored in storage variables, would save 400 gas.
• Similarly staderConfig is being read from storage for more than 2 times at a lot functions across different files, where it can be cached in memory and used instead of reading repeatedly from storage, would save lot of gas. Eg. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/SocializingPool.sol#L61, https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolSelector.sol#L76, etc
• Cache INodeRegistry((staderConfig).getPermissionlessNodeRegistry()).POOL_ID() in memory or storage during initialization instead of calling each time in loop in https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionlessPool.sol#L96
• In https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionlessNodeRegistry.sol#L228 getPreDepositSize should be called and cached and passed in handleInvalidSignature function, where instead it is being called for each key in loop costing 3k gas in each call