From 9e5f477c1da80094769ce5e1ff41c8e0994f5b04 Mon Sep 17 00:00:00 2001 From: rayeaster Date: Thu, 21 Mar 2024 11:57:16 +0800 Subject: [PATCH] use same calculation as CDPManager for CDP split fee in invariant test --- .../TestContracts/invariants/BeforeAfter.sol | 21 ++++-- .../TestContracts/invariants/Properties.sol | 75 +++++++++++-------- 2 files changed, 58 insertions(+), 38 deletions(-) diff --git a/packages/contracts/contracts/TestContracts/invariants/BeforeAfter.sol b/packages/contracts/contracts/TestContracts/invariants/BeforeAfter.sol index f77a9e993..d66c0c001 100644 --- a/packages/contracts/contracts/TestContracts/invariants/BeforeAfter.sol +++ b/packages/contracts/contracts/TestContracts/invariants/BeforeAfter.sol @@ -96,6 +96,8 @@ abstract contract BeforeAfter is BaseStorageVariables { uint256 yieldStEthIndexAfter; uint256 yieldCdpDebtBefore; uint256 yieldCdpDebtAfter; + uint256 yieldActorSplitFeeIdxBefore; + uint256 yieldActorSplitFeeIdxAfter; } Vars vars; @@ -172,12 +174,16 @@ abstract contract BeforeAfter is BaseStorageVariables { vars.totalCollateralSnapshotBefore = cdpManager.totalCollateralSnapshot(); vars.yieldControlValueBefore = collateral.balanceOf(yieldControlAddress); - ( vars.yieldCdpDebtBefore, vars.yieldActorSharesBefore) = cdpManager.getSyncedDebtAndCollShares(_cdpId); + (vars.yieldCdpDebtBefore, vars.yieldActorSharesBefore) = cdpManager + .getSyncedDebtAndCollShares(_cdpId); vars.yieldActorValueBefore = collateral.getPooledEthByShares(vars.yieldActorSharesBefore); - vars.yieldProtocolValueBefore = collateral.getPooledEthByShares(activePool.getSystemCollShares()); + vars.yieldProtocolValueBefore = collateral.getPooledEthByShares( + activePool.getSystemCollShares() + ); vars.yieldProtocolCollSharesBefore = activePool.getSystemCollShares(); - + vars.yieldStEthIndexBefore = cdpManager.stEthIndex(); + vars.yieldActorSplitFeeIdxBefore = cdpManager.cdpStEthFeePerUnitIndex(_cdpId); } function _after(bytes32 _cdpId) internal { @@ -254,11 +260,16 @@ abstract contract BeforeAfter is BaseStorageVariables { // PYS vars.yieldControlValueAfter = collateral.balanceOf(yieldControlAddress); - (vars.yieldCdpDebtAfter, vars.yieldActorSharesAfter) = cdpManager.getSyncedDebtAndCollShares(_cdpId); + (vars.yieldCdpDebtAfter, vars.yieldActorSharesAfter) = cdpManager.getSyncedDebtAndCollShares( + _cdpId + ); vars.yieldActorValueAfter = collateral.getPooledEthByShares(vars.yieldActorSharesAfter); - vars.yieldProtocolValuePerNewIndex = collateral.getPooledEthByShares(vars.activePoolCollBefore); + vars.yieldProtocolValuePerNewIndex = collateral.getPooledEthByShares( + vars.activePoolCollBefore + ); vars.yieldProtocolCollSharesAfter = activePool.getSystemCollShares(); vars.yieldStEthIndexAfter = cdpManager.stEthIndex(); + vars.yieldActorSplitFeeIdxAfter = cdpManager.cdpStEthFeePerUnitIndex(_cdpId); } function _diff() internal view returns (string memory log) { diff --git a/packages/contracts/contracts/TestContracts/invariants/Properties.sol b/packages/contracts/contracts/TestContracts/invariants/Properties.sol index 42500f5ab..e74d4240e 100644 --- a/packages/contracts/contracts/TestContracts/invariants/Properties.sol +++ b/packages/contracts/contracts/TestContracts/invariants/Properties.sol @@ -588,10 +588,11 @@ abstract contract Properties is BeforeAfter, PropertiesDescriptions, Asserts, Pr // If the CDP has 0 shares then it has been liquidated if (_coll == 0) return true; - + // In case the Yield Actor closes or redeems, we check their address as well // Note: we must also remember the gas stipend gets subtracted from the shares at the start - uint256 yieldTargetCollateral = collateral.balanceOf(address(actors[yieldTarget])) + collateral.getPooledEthByShares(_coll); + uint256 yieldTargetCollateral = collateral.balanceOf(address(actors[yieldTarget])) + + collateral.getPooledEthByShares(_coll); /** If the collateral does not match then we prob have a cdp that has been partially redeemed by another party @@ -604,18 +605,15 @@ abstract contract Properties is BeforeAfter, PropertiesDescriptions, Asserts, Pr uint256 collValue; if (debt < eBtcAmount) { // Price is in btc/stETH - yieldTargetCollateral += (eBtcAmount - debt) * priceFeedMock.getPrice() / 1e18; + yieldTargetCollateral += ((eBtcAmount - debt) * priceFeedMock.getPrice()) / 1e18; } // Is approximate eq good here? 1e8 would be dust - return _assertApproximateEq(collateral.balanceOf(controlActor), yieldTargetCollateral, 1e8); + return _assertApproximateEq(collateral.balanceOf(controlActor), yieldTargetCollateral, 1e8); } // At all times the fees taken from systemCollShares should be added to feeRecipientCollShares - function invariant_PYS_02( - CdpManager cdpManager, - Vars memory vars - ) internal view returns (bool) { + function invariant_PYS_02(CdpManager cdpManager, Vars memory vars) internal view returns (bool) { // If yieldControlAddress is not set this invariant cannot be tested if (yieldControlAddress == address(0)) return true; // If our target CDP has no collateral then we cannot test this invariant @@ -624,19 +622,20 @@ abstract contract Properties is BeforeAfter, PropertiesDescriptions, Asserts, Pr // In total the yield growth percentage of the control must be equal to protocol growth percentage // Has there been yield? if (vars.prevStEthFeeIndex < vars.afterStEthFeeIndex) { - return _assertApproximateEq(vars.yieldProtocolCollSharesBefore - vars.yieldProtocolCollSharesAfter, vars.feeRecipientCollSharesAfter - vars.feeRecipientCollSharesBefore, 1e4); + return + _assertApproximateEq( + vars.yieldProtocolCollSharesBefore - vars.yieldProtocolCollSharesAfter, + vars.feeRecipientCollSharesAfter - vars.feeRecipientCollSharesBefore, + 1e4 + ); } // Implies there was no yield return true; - } // As a user, the value of my CDP after rebase should be equal to it's value prior to rebase + yield - fee, according to the PYS at the moment of rebase - function invariant_PYS_03( - CdpManager cdpManager, - Vars memory vars - ) internal view returns (bool) { + function invariant_PYS_03(CdpManager cdpManager, Vars memory vars) internal view returns (bool) { // If yieldControlAddress is not set this invariant cannot be tested if (yieldControlAddress == address(0)) return true; // If our target CDP has no collateral then we cannot test this invariant @@ -654,31 +653,42 @@ abstract contract Properties is BeforeAfter, PropertiesDescriptions, Asserts, Pr // Has there been yield? if (vars.yieldStEthIndexAfter > vars.yieldStEthIndexBefore) { // Determine the growth, adjusted to 1e18 precision - uint256 yieldGrowthPercent = (vars.yieldStEthIndexAfter - vars.yieldStEthIndexBefore) * 1e18 / vars.yieldStEthIndexBefore; + // uint256 yieldGrowthPercent = (vars.yieldStEthIndexAfter - vars.yieldStEthIndexBefore) * 1e18 / vars.yieldStEthIndexBefore; // Just calling here to check if there is a change (which is expected) // Chasing down a bug, please excuse the calls - collateral.getPooledEthByShares(vars.yieldStEthIndexBefore); - collateral.getPooledEthByShares(vars.yieldStEthIndexAfter); - collateral.getPooledEthByShares(vars.yieldActorSharesBefore); - collateral.getPooledEthByShares(vars.yieldActorSharesAfter); - collateral._getTotalPooledEther(); - collateral._getTotalShares(); + // collateral.getPooledEthByShares(vars.yieldStEthIndexBefore); + // collateral.getPooledEthByShares(vars.yieldStEthIndexAfter); + // collateral.getPooledEthByShares(vars.yieldActorSharesBefore); + // collateral.getPooledEthByShares(vars.yieldActorSharesAfter); + // collateral._getTotalPooledEther(); + // collateral._getTotalShares(); // Expected fee is (growth % * startingVal) * feeSplit % - uint256 expectedFee = (vars.yieldActorValueBefore * yieldGrowthPercent / 1e18) * cdpManager.stakingRewardSplit() / cdpManager.MAX_REWARD_SPLIT(); - uint256 feeInShares = collateral.getSharesByPooledEth(expectedFee); - - return _assertApproximateEq( - // It seems that the actual fees taken are always slightly higher than expected. Why? - collateral.getPooledEthByShares(vars.yieldActorSharesBefore - feeInShares), - collateral.getPooledEthByShares(vars.yieldActorSharesAfter), - 1e6 // @audit more acceptable precision loss - // TO DO: tweak and decrease tolerance until acceptable + // uint256 expectedFee = (vars.yieldActorValueBefore * yieldGrowthPercent / 1e18) * cdpManager.stakingRewardSplit() / cdpManager.MAX_REWARD_SPLIT(); + uint256 feeInShares = vars.cdpStakeBefore * + (vars.afterStEthFeeIndex - vars.yieldActorSplitFeeIdxBefore); //collateral.getSharesByPooledEth(expectedFee); + require( + vars.yieldActorSplitFeeIdxAfter == vars.afterStEthFeeIndex, + "!fee index should be synced" + ); + uint256 _collAfterFeeApplied = (vars.yieldActorSharesBefore * 1e18 - feeInShares) / 1e18; + // CDP collateral value should increase after positive rebase even with fee split + require( + (vars.yieldActorSharesBefore * vars.yieldStEthIndexBefore) < + (vars.yieldActorSharesAfter * vars.yieldStEthIndexAfter), + "!CDP collateral value should increase after positive rebase" ); - } - + return + _assertApproximateEq( + // It seems that the actual fees taken are always slightly higher than expected. Why? + (_collAfterFeeApplied), + (vars.yieldActorSharesAfter), + 1e6 // @audit more acceptable precision loss + // TO DO: tweak and decrease tolerance until acceptable + ); + } /*** deltaIndex = 1027643607040687991 - 1000000000000000000 @@ -727,7 +737,6 @@ abstract contract Properties is BeforeAfter, PropertiesDescriptions, Asserts, Pr // Implies there was no yield return true; - } // }