Skip to content

Commit

Permalink
Merge pull request #636 from lidofinance/fix/preliminary-audit-fixes
Browse files Browse the repository at this point in the history 
Fixes related to `handleOracleReport` and shares burning
  • Loading branch information
@TheDZhon
TheDZhon committed Feb 20, 2023
2 parents a7c7bfb + 55c2b09 commit fdbc5c1
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 58 deletions.
32 changes: 18 additions & 14 deletions contracts/0.4.24/Lido.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@ contract Lido is Versioned, StETHPermit, AragonApp {

uint256 private constant DEPOSIT_SIZE = 32 ether;
uint256 public constant TOTAL_BASIS_POINTS = 10000;
/// @dev special value for the last finalizable withdrawal request id
/// @dev special value to not finalize withdrawal requests
/// see the `_lastFinalizableRequestId` arg for `handleOracleReport()`
uint256 private constant DONT_FINALIZE_WITHDRAWALS = 0;

/// @dev storage slot position for the Lido protocol contracts locator
Expand Down Expand Up @@ -564,10 +565,9 @@ contract Lido is Versioned, StETHPermit, AragonApp {
* @param _lastFinalizableRequestId right boundary of requestId range if equals 0, no requests should be finalized
* @param _simulatedShareRate share rate that was simulated by oracle when the report data created (1e27 precision)
*
* NB: `_simulatedShareRate` should be calculated by the Oracle daemon
* invoking the method with static call and passing `_lastFinalizableRequestId` == `_simulatedShareRate` == 0
* plugging the returned values to the following formula:
* `_simulatedShareRate = (postTotalPooledEther * 1e27) / postTotalShares`
* NB: `_simulatedShareRate` should be calculated off-chain by calling the method with `eth_call` JSON-RPC API
* while passing `_lastFinalizableRequestId` == `_simulatedShareRate` == 0, and plugging the returned values
* to the following formula: `_simulatedShareRate = (postTotalPooledEther * 1e27) / postTotalShares`
*
* @return postTotalPooledEther amount of ether in the protocol after report
* @return postTotalShares amount of shares in the protocol after report
Expand Down Expand Up @@ -1210,7 +1210,7 @@ contract Lido is Versioned, StETHPermit, AragonApp {
* (i.e., postpone the extra rewards to be applied during the next rounds)
* 5. Invoke finalization of the withdrawal requests
* 6. Distribute protocol fee (treasury & node operators)
* 7. Burn excess shares (withdrawn stETH at least)
* 7. Burn excess shares within the allowed limit (can postpone some shares to be burnt later)
* 8. Complete token rebase by informing observers (emit an event and call the external receivers if any)
* 9. Sanity check for the provided simulated share rate
*/
Expand Down Expand Up @@ -1295,8 +1295,9 @@ contract Lido is Versioned, StETHPermit, AragonApp {
);

// Step 7.
// Burn excess shares (withdrawn stETH at least)
uint256 burntWithdrawalQueueShares = _burnSharesLimited(
// Burn excess shares within the allowed limit (can postpone some shares to be burnt later)
// Return actually burnt shares of the current report's finalized withdrawal requests to use in sanity checks
uint256 burntCurrentWithdrawalShares = _burnSharesLimited(
IBurner(_contracts.burner),
_contracts.withdrawalQueue,
reportContext.sharesToBurnFromWithdrawalQueue,
Expand All @@ -1320,7 +1321,7 @@ contract Lido is Versioned, StETHPermit, AragonApp {
postTotalPooledEther,
postTotalShares,
reportContext.etherToLockOnWithdrawalQueue,
burntWithdrawalQueueShares,
burntCurrentWithdrawalShares,
_reportedData.simulatedShareRate
);
}
Expand Down Expand Up @@ -1384,17 +1385,20 @@ contract Lido is Versioned, StETHPermit, AragonApp {
/*
* @dev Perform burning of `stETH` shares via the dedicated `Burner` contract.
*
* NB: some of the burning amount can be postponed for the next reports
* if positive token rebase smoothened.
* NB: some of the burning amount can be postponed for the next reports if positive token rebase smoothened.
* It's possible that underlying shares of the current oracle report's finalized withdrawals won't be burnt
* completely in a single oracle report round due to the provided `_sharesToBurnLimit` limit
*
* @return burnt shares from withdrawals queue (when some requests finalized)
* @return shares actually burnt for the current oracle report's finalized withdrawals
* these shares are assigned to be burnt most recently, so the amount can be calculated deducting
* `postponedSharesToBurn` shares (if any) after the burn commitment & execution
*/
function _burnSharesLimited(
IBurner _burner,
address _withdrawalQueue,
uint256 _sharesToBurnFromWithdrawalQueue,
uint256 _sharesToBurnLimit
) internal returns (uint256 burntWithdrawalsShares) {
) internal returns (uint256 burntCurrentWithdrawalShares) {
if (_sharesToBurnFromWithdrawalQueue > 0) {
_burner.requestBurnShares(_withdrawalQueue, _sharesToBurnFromWithdrawalQueue);
}
Expand All @@ -1410,7 +1414,7 @@ contract Lido is Versioned, StETHPermit, AragonApp {
(uint256 coverShares, uint256 nonCoverShares) = _burner.getSharesRequestedToBurn();
uint256 postponedSharesToBurn = coverShares.add(nonCoverShares);

burntWithdrawalsShares =
burntCurrentWithdrawalShares =
postponedSharesToBurn < _sharesToBurnFromWithdrawalQueue ?
_sharesToBurnFromWithdrawalQueue - postponedSharesToBurn : 0;
}
Expand Down
56 changes: 28 additions & 28 deletions contracts/0.8.9/sanity_checks/OracleReportSanityChecker.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ struct LimitsList {
/// @dev Represented in the Basis Points (100% == 10_000)
uint256 annualBalanceIncreaseBPLimit;

/// @notice The max deviation of stETH.totalPooledEther() / stETH.totalShares() ratio since
/// the previous oracle report
/// @notice The max deviation of the provided `simulatedShareRate`
/// and the actual one within the currently processing oracle report
/// @dev Represented in the Basis Points (100% == 10_000)
uint256 shareRateDeviationBPLimit;
uint256 simulatedShareRateDeviationBPLimit;

/// @notice The max number of exit requests allowed in report to ValidatorsExitBusOracle
uint256 maxValidatorExitRequestsPerReport;
Expand All @@ -73,7 +73,7 @@ struct LimitsListPacked {
uint16 churnValidatorsPerDayLimit;
uint16 oneOffCLBalanceDecreaseBPLimit;
uint16 annualBalanceIncreaseBPLimit;
uint16 shareRateDeviationBPLimit;
uint16 simulatedShareRateDeviationBPLimit;
uint16 maxValidatorExitRequestsPerReport;
uint16 maxAccountingExtraDataListItemsCount;
uint16 maxNodeOperatorsPerExtraDataItemCount;
Expand Down Expand Up @@ -242,15 +242,15 @@ contract OracleReportSanityChecker is AccessControlEnumerable {
_updateLimits(limitsList);
}

/// @notice Sets the new value for the shareRateDeviationBPLimit
/// @param _shareRateDeviationBPLimit new shareRateDeviationBPLimit value
function setShareRateDeviationBPLimit(uint256 _shareRateDeviationBPLimit)
/// @notice Sets the new value for the simulatedShareRateDeviationBPLimit
/// @param _simulatedShareRateDeviationBPLimit new simulatedShareRateDeviationBPLimit value
function setSimulatedShareRateDeviationBPLimit(uint256 _simulatedShareRateDeviationBPLimit)
external
onlyRole(SHARE_RATE_DEVIATION_LIMIT_MANAGER_ROLE)
{
_checkLimitValue(_shareRateDeviationBPLimit, MAX_BASIS_POINTS);
_checkLimitValue(_simulatedShareRateDeviationBPLimit, MAX_BASIS_POINTS);
LimitsList memory limitsList = _limits.unpack();
limitsList.shareRateDeviationBPLimit = _shareRateDeviationBPLimit;
limitsList.simulatedShareRateDeviationBPLimit = _simulatedShareRateDeviationBPLimit;
_updateLimits(limitsList);
}

Expand Down Expand Up @@ -472,9 +472,9 @@ contract OracleReportSanityChecker is AccessControlEnumerable {
/// @notice Applies sanity checks to the simulated share rate for withdrawal requests finalization
/// @param _postTotalPooledEther total pooled ether after report applied
/// @param _postTotalShares total shares after report applied
/// @param _etherLockedOnWithdrawalQueue ether to lock on withdrawal queue
/// @param _sharesBurntFromWithdrawalQueue shares assigned to burn from withdrawal queue
/// @param _simulatedShareRate share rate provided with the oracle report (simulated via static call)
/// @param _etherLockedOnWithdrawalQueue ether locked on withdrawal queue for the current oracle report
/// @param _sharesBurntFromWithdrawalQueue shares burnt from withdrawal queue for the current oracle report
/// @param _simulatedShareRate share rate provided with the oracle report (simulated via off-chain "eth_call")
function checkSimulatedShareRate(
uint256 _postTotalPooledEther,
uint256 _postTotalShares,
Expand All @@ -485,9 +485,9 @@ contract OracleReportSanityChecker is AccessControlEnumerable {
LimitsList memory limitsList = _limits.unpack();

// Pretending that withdrawals were not processed
// virtually return locked ether back to postTotalPooledEther
// virtually return burnt shares back to postTotalShares
_checkFinalizationShareRate(
// virtually return locked ether back to `_postTotalPooledEther`
// virtually return burnt just finalized withdrawals shares back to `_postTotalShares`
_checkSimulatedShareRate(
limitsList,
_postTotalPooledEther + _etherLockedOnWithdrawalQueue,
_postTotalShares + _sharesBurntFromWithdrawalQueue,
Expand Down Expand Up @@ -580,7 +580,7 @@ contract OracleReportSanityChecker is AccessControlEnumerable {
revert IncorrectRequestFinalization(requestTimestampToFinalizeUpTo);
}

function _checkFinalizationShareRate(
function _checkSimulatedShareRate(
LimitsList memory _limitsList,
uint256 _noWithdrawalsPostTotalPooledEther,
uint256 _noWithdrawalsPostTotalShares,
Expand All @@ -592,16 +592,16 @@ contract OracleReportSanityChecker is AccessControlEnumerable {

if (actualShareRate == 0) {
// can't finalize anything if the actual share rate is zero
revert IncorrectFinalizationShareRate(MAX_BASIS_POINTS);
revert IncorrectSimulatedShareRate(MAX_BASIS_POINTS);
}

uint256 finalizationShareDiff = Math256.abs(
uint256 simulatedShareDiff = Math256.abs(
SafeCast.toInt256(_simulatedShareRate) - SafeCast.toInt256(actualShareRate)
);
uint256 finalizationShareDeviation = (MAX_BASIS_POINTS * finalizationShareDiff) / actualShareRate;
uint256 simulatedShareDeviation = (MAX_BASIS_POINTS * simulatedShareDiff) / actualShareRate;

if (finalizationShareDeviation > _limitsList.shareRateDeviationBPLimit) {
revert IncorrectFinalizationShareRate(finalizationShareDeviation);
if (simulatedShareDeviation > _limitsList.simulatedShareRateDeviationBPLimit) {
revert IncorrectSimulatedShareRate(simulatedShareDeviation);
}
}

Expand All @@ -625,9 +625,9 @@ contract OracleReportSanityChecker is AccessControlEnumerable {
_checkLimitValue(_newLimitsList.annualBalanceIncreaseBPLimit, type(uint16).max);
emit AnnualBalanceIncreaseBPLimitSet(_newLimitsList.annualBalanceIncreaseBPLimit);
}
if (_oldLimitsList.shareRateDeviationBPLimit != _newLimitsList.shareRateDeviationBPLimit) {
_checkLimitValue(_newLimitsList.shareRateDeviationBPLimit, type(uint16).max);
emit ShareRateDeviationBPLimitSet(_newLimitsList.shareRateDeviationBPLimit);
if (_oldLimitsList.simulatedShareRateDeviationBPLimit != _newLimitsList.simulatedShareRateDeviationBPLimit) {
_checkLimitValue(_newLimitsList.simulatedShareRateDeviationBPLimit, type(uint16).max);
emit SimulatedShareRateDeviationBPLimitSet(_newLimitsList.simulatedShareRateDeviationBPLimit);
}
if (_oldLimitsList.maxValidatorExitRequestsPerReport != _newLimitsList.maxValidatorExitRequestsPerReport) {
_checkLimitValue(_newLimitsList.maxValidatorExitRequestsPerReport, type(uint16).max);
Expand Down Expand Up @@ -661,7 +661,7 @@ contract OracleReportSanityChecker is AccessControlEnumerable {
event ChurnValidatorsPerDayLimitSet(uint256 churnValidatorsPerDayLimit);
event OneOffCLBalanceDecreaseBPLimitSet(uint256 oneOffCLBalanceDecreaseBPLimit);
event AnnualBalanceIncreaseBPLimitSet(uint256 annualBalanceIncreaseBPLimit);
event ShareRateDeviationBPLimitSet(uint256 shareRateDeviationBPLimit);
event SimulatedShareRateDeviationBPLimitSet(uint256 simulatedShareRateDeviationBPLimit);
event MaxPositiveTokenRebaseSet(uint256 maxPositiveTokenRebase);
event MaxValidatorExitRequestsPerReportSet(uint256 maxValidatorExitRequestsPerReport);
event MaxAccountingExtraDataListItemsCountSet(uint256 maxAccountingExtraDataListItemsCount);
Expand All @@ -677,7 +677,7 @@ contract OracleReportSanityChecker is AccessControlEnumerable {
error IncorrectNumberOfExitRequestsPerReport(uint256 maxRequestsCount);
error IncorrectExitedValidators(uint256 churnLimit);
error IncorrectRequestFinalization(uint256 requestCreationBlock);
error IncorrectFinalizationShareRate(uint256 finalizationShareDeviation);
error IncorrectSimulatedShareRate(uint256 simulatedShareDeviation);
error MaxAccountingExtraDataItemsCountExceeded(uint256 maxItemsCount, uint256 receivedItemsCount);
error ExitedValidatorsLimitExceeded(uint256 limitPerDay, uint256 exitedPerDay);
error TooManyNodeOpsPerExtraDataItem(uint256 itemIndex, uint256 nodeOpsCount);
Expand All @@ -688,7 +688,7 @@ library LimitsListPacker {
res.churnValidatorsPerDayLimit = SafeCast.toUint16(_limitsList.churnValidatorsPerDayLimit);
res.oneOffCLBalanceDecreaseBPLimit = _toBasisPoints(_limitsList.oneOffCLBalanceDecreaseBPLimit);
res.annualBalanceIncreaseBPLimit = _toBasisPoints(_limitsList.annualBalanceIncreaseBPLimit);
res.shareRateDeviationBPLimit = _toBasisPoints(_limitsList.shareRateDeviationBPLimit);
res.simulatedShareRateDeviationBPLimit = _toBasisPoints(_limitsList.simulatedShareRateDeviationBPLimit);
res.requestTimestampMargin = SafeCast.toUint64(_limitsList.requestTimestampMargin);
res.maxPositiveTokenRebase = SafeCast.toUint64(_limitsList.maxPositiveTokenRebase);
res.maxValidatorExitRequestsPerReport = SafeCast.toUint16(_limitsList.maxValidatorExitRequestsPerReport);
Expand All @@ -707,7 +707,7 @@ library LimitsListUnpacker {
res.churnValidatorsPerDayLimit = _limitsList.churnValidatorsPerDayLimit;
res.oneOffCLBalanceDecreaseBPLimit = _limitsList.oneOffCLBalanceDecreaseBPLimit;
res.annualBalanceIncreaseBPLimit = _limitsList.annualBalanceIncreaseBPLimit;
res.shareRateDeviationBPLimit = _limitsList.shareRateDeviationBPLimit;
res.simulatedShareRateDeviationBPLimit = _limitsList.simulatedShareRateDeviationBPLimit;
res.requestTimestampMargin = _limitsList.requestTimestampMargin;
res.maxPositiveTokenRebase = _limitsList.maxPositiveTokenRebase;
res.maxValidatorExitRequestsPerReport = _limitsList.maxValidatorExitRequestsPerReport;
Expand Down
2 changes: 1 addition & 1 deletion lib/abi/OracleReportSanityChecker.json
View file

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion test/0.4.24/lido-handle-oracle-report.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const ORACLE_REPORT_LIMITS_BOILERPLATE = {
churnValidatorsPerDayLimit: 255,
oneOffCLBalanceDecreaseBPLimit: 100,
annualBalanceIncreaseBPLimit: 10000,
shareRateDeviationBPLimit: 10000,
simulatedShareRateDeviationBPLimit: 10000,
maxValidatorExitRequestsPerReport: 10000,
maxAccountingExtraDataListItemsCount: 10000,
maxNodeOperatorsPerExtraDataItemCount: 10000,
Expand Down
22 changes: 11 additions & 11 deletions test/0.8.9/oracle-report-sanity-checker.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ contract('OracleReportSanityChecker', ([deployer, admin, withdrawalVault, elRewa
churnValidatorsPerDayLimit: 55,
oneOffCLBalanceDecreaseBPLimit: 5_00, // 5%
annualBalanceIncreaseBPLimit: 10_00, // 10%
shareRateDeviationBPLimit: 2_50, // 2.5%
simulatedShareRateDeviationBPLimit: 2_50, // 2.5%
maxValidatorExitRequestsPerReport: 2000,
maxAccountingExtraDataListItemsCount: 15,
maxNodeOperatorsPerExtraDataItemCount: 16,
Expand Down Expand Up @@ -82,7 +82,7 @@ contract('OracleReportSanityChecker', ([deployer, admin, withdrawalVault, elRewa
churnValidatorsPerDayLimit: 50,
oneOffCLBalanceDecreaseBPLimit: 10_00,
annualBalanceIncreaseBPLimit: 15_00,
shareRateDeviationBPLimit: 1_50, // 1.5%
simulatedShareRateDeviationBPLimit: 1_50, // 1.5%
maxValidatorExitRequestsPerReport: 3000,
maxAccountingExtraDataListItemsCount: 15 + 1,
maxNodeOperatorsPerExtraDataItemCount: 16 + 1,
Expand All @@ -93,7 +93,7 @@ contract('OracleReportSanityChecker', ([deployer, admin, withdrawalVault, elRewa
assert.notEquals(limitsBefore.churnValidatorsPerDayLimit, newLimitsList.churnValidatorsPerDayLimit)
assert.notEquals(limitsBefore.oneOffCLBalanceDecreaseBPLimit, newLimitsList.oneOffCLBalanceDecreaseBPLimit)
assert.notEquals(limitsBefore.annualBalanceIncreaseBPLimit, newLimitsList.annualBalanceIncreaseBPLimit)
assert.notEquals(limitsBefore.shareRateDeviationBPLimit, newLimitsList.shareRateDeviationBPLimit)
assert.notEquals(limitsBefore.simulatedShareRateDeviationBPLimit, newLimitsList.simulatedShareRateDeviationBPLimit)
assert.notEquals(limitsBefore.maxValidatorExitRequestsPerReport, newLimitsList.maxValidatorExitRequestsPerReport)
assert.notEquals(limitsBefore.maxAccountingExtraDataListItemsCount, newLimitsList.maxAccountingExtraDataListItemsCount)
assert.notEquals(limitsBefore.maxNodeOperatorsPerExtraDataItemCount, newLimitsList.maxNodeOperatorsPerExtraDataItemCount)
Expand All @@ -108,7 +108,7 @@ contract('OracleReportSanityChecker', ([deployer, admin, withdrawalVault, elRewa
assert.equals(limitsAfter.churnValidatorsPerDayLimit, newLimitsList.churnValidatorsPerDayLimit)
assert.equals(limitsAfter.oneOffCLBalanceDecreaseBPLimit, newLimitsList.oneOffCLBalanceDecreaseBPLimit)
assert.equals(limitsAfter.annualBalanceIncreaseBPLimit, newLimitsList.annualBalanceIncreaseBPLimit)
assert.equals(limitsAfter.shareRateDeviationBPLimit, newLimitsList.shareRateDeviationBPLimit)
assert.equals(limitsAfter.simulatedShareRateDeviationBPLimit, newLimitsList.simulatedShareRateDeviationBPLimit)
assert.equals(limitsAfter.maxValidatorExitRequestsPerReport, newLimitsList.maxValidatorExitRequestsPerReport)
assert.equals(limitsAfter.maxAccountingExtraDataListItemsCount, newLimitsList.maxAccountingExtraDataListItemsCount)
assert.equals(limitsAfter.maxNodeOperatorsPerExtraDataItemCount, newLimitsList.maxNodeOperatorsPerExtraDataItemCount)
Expand Down Expand Up @@ -260,22 +260,22 @@ contract('OracleReportSanityChecker', ([deployer, admin, withdrawalVault, elRewa
simulatedShareRate: (BigInt(2) * 10n ** 27n).toString()
}

it('reverts with error IncorrectFinalizationShareRate() when reported and onchain share rate differs', async () => {
const finalizationShareRate = BigInt(ETH(2.10)) * 10n ** 9n
it('reverts with error IncorrectSimulatedShareRate() when reported and onchain share rate differs', async () => {
const simulatedShareRate = BigInt(ETH(2.10)) * 10n ** 9n
const actualShareRate = BigInt(2) * 10n ** 27n
const deviation = (100_00n * (finalizationShareRate - actualShareRate)) / actualShareRate
const deviation = (100_00n * (simulatedShareRate - actualShareRate)) / actualShareRate
await assert.revertsWithCustomError(
oracleReportSanityChecker.checkSimulatedShareRate(
...Object.values({
...correctSimulatedShareRate,
simulatedShareRate: finalizationShareRate.toString()
simulatedShareRate: simulatedShareRate.toString()
})
),
`IncorrectFinalizationShareRate(${deviation.toString()})`
`IncorrectSimulatedShareRate(${deviation.toString()})`
)
})

it('reverts with error IncorrectFinalizationShareRate() when actual share rate is zero', async () => {
it('reverts with error IncorrectSimulatedShareRate() when actual share rate is zero', async () => {
const deviation = 100_00n
await assert.revertsWithCustomError(
oracleReportSanityChecker.checkSimulatedShareRate(
Expand All @@ -285,7 +285,7 @@ contract('OracleReportSanityChecker', ([deployer, admin, withdrawalVault, elRewa
postTotalPooledEther: ETH(0)
})
),
`IncorrectFinalizationShareRate(${deviation.toString()})`
`IncorrectSimulatedShareRate(${deviation.toString()})`
)
})

Expand Down

0 comments on commit fdbc5c1

Please sign in to comment.