Stader Labs - turvy_fuzz's results

Lines of code

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

Vulnerability details

Impact

If a negative value is returned (< 0) from chainlink oracle and the value is cast to type uint256, the resulting value will be the unsigned representation of that value which will be an inaccurate price. Also, cases where sdprice can't be less than 0 will revert (i.e division by 0 - https://github.com/code-423n4/2023-06-stader/blob/main/contracts/SDCollateral.sol#L212)

Proof of Concept

missing check: `(, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy()) .latestRoundData(); (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy()) .latestRoundData(); // @audit // if ( // totalETHBalanceInInt <= 0 || totalETHBalanceInInt <= 0 // ) revert StaderOracle_BadFeed();

return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number);`

Tools Used

Recommended Mitigation Steps

Always validate price feed answers before using and also it's important to note that casting a negative value of signed integer to an unsigned integer type may result in unexpected behavior, add the missing check above: if ( // totalETHBalanceInInt <= 0 || totalETHBalanceInInt <= 0 // ) revert StaderOracle_BadFeed(); and Always exercise caution when performing such type conversions to ensure that the resulting value is appropriate

Assessed type

Oracle

L-1 Manager can disable erInspectionMode when it's Cooldown is not completed

Summary

use of wrong operator

Vulnerability Details

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L187 Due to the use of the wrong operator (&& instead of ||), manager can still disable erInspection Mode even when it's Cooldown is not completed. Also when not in cooldown, absolutely anyone can still disable it due to the && operator

Recommendation:

use || instead of &&.

L-2 Use a 2-step ownership transfer pattern

It’s possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/VaultProxy.sol#L70

Recommendation

Consider implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.