From 09d00eb069d6fd960eeacd4146b01c0a794c1e3a Mon Sep 17 00:00:00 2001 From: dapp-whisperer Date: Sat, 14 Oct 2023 12:16:34 -0400 Subject: [PATCH 1/5] _updateDebtRedistributionIndex naming consistency --- packages/contracts/contracts/CdpManager.sol | 2 +- packages/contracts/test/BorrowerOperationsTest.js | 10 +++++----- packages/contracts/test/CdpManagerTest.js | 4 ++-- packages/contracts/utils/testHelpers.js | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/contracts/contracts/CdpManager.sol b/packages/contracts/contracts/CdpManager.sol index 1037c2701..a9e57bbbf 100644 --- a/packages/contracts/contracts/CdpManager.sol +++ b/packages/contracts/contracts/CdpManager.sol @@ -878,7 +878,7 @@ contract CdpManager is CdpManagerStorage, ICdpManager, Proxy { Cdps[_cdpId].liquidatorRewardShares = _liquidatorRewardShares; _applyAccumulatedFeeSplit(_cdpId); - _updateRedistributedDebtSnapshot(_cdpId); + _updateDebtRedistributionIndex(_cdpId); uint256 stake = _updateStakeAndTotalStakes(_cdpId); uint256 index = _addCdpIdToArray(_cdpId); diff --git a/packages/contracts/test/BorrowerOperationsTest.js b/packages/contracts/test/BorrowerOperationsTest.js index 682efc66e..da6711acb 100644 --- a/packages/contracts/test/BorrowerOperationsTest.js +++ b/packages/contracts/test/BorrowerOperationsTest.js @@ -394,9 +394,9 @@ contract('BorrowerOperations', async accounts => { assert.equal(alice_EBTCDebtRewardSnapshot_Before, 0) assert.equal(bob_EBTCDebtRewardSnapshot_Before, 0) - const alicePendingEBTCDebtReward = (await cdpManager.getPendingRedistributedDebt(aliceIndex)) - const bobPendingEBTCDebtReward = (await cdpManager.getPendingRedistributedDebt(bobIndex)) - for (reward of [alicePendingEBTCDebtReward, bobPendingEBTCDebtReward]) { + const alicePendingRedistributedDebt = (await cdpManager.getPendingRedistributedDebt(aliceIndex)) + const bobPendingRedistributedDebt = (await cdpManager.getPendingRedistributedDebt(bobIndex)) + for (reward of [alicePendingRedistributedDebt, bobPendingRedistributedDebt]) { assert.isTrue(reward.gt(toBN('0'))) } @@ -414,9 +414,9 @@ contract('BorrowerOperations', async accounts => { const bobNewDebt = await getCdpEntireDebt(bobIndex) assert.isTrue(aliceNewColl.eq(aliceCollBefore.add(aliceTopUp))) - assert.isTrue(aliceNewDebt.eq(aliceDebtBefore.add(alicePendingEBTCDebtReward))) + assert.isTrue(aliceNewDebt.eq(aliceDebtBefore.add(alicePendingRedistributedDebt))) assert.isTrue(bobNewColl.eq(bobCollBefore.add(bobTopUp))) - assert.isTrue(bobNewDebt.eq(bobDebtBefore.add(bobPendingEBTCDebtReward))) + assert.isTrue(bobNewDebt.eq(bobDebtBefore.add(bobPendingRedistributedDebt))) /* Check that both Alice and Bob's snapshots of the rewards-per-unit-staked metrics should be updated to the latest values of L_STETHColl and systemDebtRedistributionIndex */ diff --git a/packages/contracts/test/CdpManagerTest.js b/packages/contracts/test/CdpManagerTest.js index 518f4018a..d874cb57d 100644 --- a/packages/contracts/test/CdpManagerTest.js +++ b/packages/contracts/test/CdpManagerTest.js @@ -4482,8 +4482,8 @@ contract('CdpManager', async accounts => { const carolSnapshot_L_EBTCDebt = (await cdpManager.cdpDebtRedistributionIndex(_carolCdpId)) assert.equal(carolSnapshot_L_EBTCDebt, 0) - const carol_PendingEBTCDebtReward = (await cdpManager.getPendingRedistributedDebt(_carolCdpId)) - assert.isTrue(carol_PendingEBTCDebtReward.gt(toBN('0'))) + const carol_PendingRedistributedDebt = (await cdpManager.getPendingRedistributedDebt(_carolCdpId)) + assert.isTrue(carol_PendingRedistributedDebt.gt(toBN('0'))) }) it("getPendingETHReward(): Returns 0 if there is no pending ETH reward", async () => { diff --git a/packages/contracts/utils/testHelpers.js b/packages/contracts/utils/testHelpers.js index f1363ac3c..0142e36ce 100644 --- a/packages/contracts/utils/testHelpers.js +++ b/packages/contracts/utils/testHelpers.js @@ -556,8 +556,8 @@ class TestHelper { // console.log(`account: ${account}`) const rawColl = (await contracts.cdpManager.Cdps(account))[1] const rawDebt = (await contracts.cdpManager.Cdps(account))[0] - const pendingEBTCDebtReward = (await contracts.cdpManager.getPendingRedistributedDebt(account)) - const entireDebt = rawDebt.add(pendingEBTCDebtReward) + const pendingRedistributedDebt = (await contracts.cdpManager.getPendingRedistributedDebt(account)) + const entireDebt = rawDebt.add(pendingRedistributedDebt) let entireColl = rawColl; return { entireColl, entireDebt } } From f7c4f40b42d76c176367b7fbfa2e95a917d6fc1f Mon Sep 17 00:00:00 2001 From: dapp-whisperer Date: Sun, 15 Oct 2023 18:57:46 -0400 Subject: [PATCH 2/5] syncaccounting cdpupdated event + small refactor --- packages/contracts/contracts/CdpManager.sol | 4 +- .../contracts/contracts/CdpManagerStorage.sol | 127 +++++++++--------- 2 files changed, 64 insertions(+), 67 deletions(-) diff --git a/packages/contracts/contracts/CdpManager.sol b/packages/contracts/contracts/CdpManager.sol index a9e57bbbf..5607c8f06 100644 --- a/packages/contracts/contracts/CdpManager.sol +++ b/packages/contracts/contracts/CdpManager.sol @@ -877,8 +877,8 @@ contract CdpManager is CdpManagerStorage, ICdpManager, Proxy { Cdps[_cdpId].status = Status.active; Cdps[_cdpId].liquidatorRewardShares = _liquidatorRewardShares; - _applyAccumulatedFeeSplit(_cdpId); - _updateDebtRedistributionIndex(_cdpId); + cdpStEthFeePerUnitIndex[_cdpId] = systemStEthFeePerUnitIndex; /// @audit We critically assume global accounting is synced here + _updateRedistributedDebtIndex(_cdpId); uint256 stake = _updateStakeAndTotalStakes(_cdpId); uint256 index = _addCdpIdToArray(_cdpId); diff --git a/packages/contracts/contracts/CdpManagerStorage.sol b/packages/contracts/contracts/CdpManagerStorage.sol index 8639a27c9..571a99298 100644 --- a/packages/contracts/contracts/CdpManagerStorage.sol +++ b/packages/contracts/contracts/CdpManagerStorage.sol @@ -332,54 +332,68 @@ contract CdpManagerStorage is EbtcBase, ReentrancyGuard, ICdpManagerData, AuthNo return (cdpDebtRedistributionIndex[_cdpId] < systemDebtRedistributionIndex); } - function _updateRedistributedDebtSnapshot(bytes32 _cdpId) internal { - uint256 _L_EBTCDebt = systemDebtRedistributionIndex; + function _updateRedistributedDebtIndex(bytes32 _cdpId) internal { + uint256 _systemDebtRedistributionIndex = systemDebtRedistributionIndex; - cdpDebtRedistributionIndex[_cdpId] = _L_EBTCDebt; - emit CdpDebtRedistributionIndexUpdated(_cdpId, _L_EBTCDebt); + cdpDebtRedistributionIndex[_cdpId] = _systemDebtRedistributionIndex; + emit CdpDebtRedistributionIndexUpdated(_cdpId, _systemDebtRedistributionIndex); } - // Add the borrowers's coll and debt rewards earned from redistributions, to their Cdp + /// @dev Calculate the new collateral and debt values for a given CDP, based on pending state changes function _syncAccounting(bytes32 _cdpId) internal { - (, uint _newDebt, , uint _pendingDebt, uint256 _debtIndexDelta) = _applyAccumulatedFeeSplit( - _cdpId - ); + // Ensure global states like systemStEthFeePerUnitIndex get updated in a timely fashion + // whenever there is a CDP modification operation, + // such as opening, closing, adding collateral, repaying debt, or liquidating + _syncGlobalAccounting(); - // Update if there is a debt redistribution index delta - if (_debtIndexDelta > 0) { - _updateRedistributedDebtSnapshot(_cdpId); + uint256 _oldPerUnitCdp = cdpStEthFeePerUnitIndex[_cdpId]; + uint256 _systemStEthFeePerUnitIndex = systemStEthFeePerUnitIndex; + + ( + uint256 _newColl, + uint256 _newDebt, + uint256 _feeSplitDistributed, + uint _pendingDebt, + uint256 _debtIndexDelta + ) = _calcSyncedAccounting(_cdpId, _oldPerUnitCdp, _systemStEthFeePerUnitIndex); + // If any collShares or debt changes occured + if (_feeSplitDistributed > 0 || _debtIndexDelta > 0) { Cdp storage _cdp = Cdps[_cdpId]; + + uint prevCollShares = _cdp.coll; uint256 prevDebt = _cdp.debt; - if (prevDebt != _newDebt) { - { - // Apply pending debt redistribution to this CDP - _cdp.debt = _newDebt; - _emitCdpUpdatedEventForSyncAccounting(_cdpId, _cdp, _newDebt, _cdp.coll); + + // Apply Fee Split + if (_feeSplitDistributed > 0) { + _applyAccumulatedFeeSplit(_cdpId, _newColl, _feeSplitDistributed, _oldPerUnitCdp, _systemStEthFeePerUnitIndex); + } + + // Apply Debt Redistribution + if (_debtIndexDelta > 0) { + _updateRedistributedDebtIndex(_cdpId); + + if (prevDebt != _newDebt) { + { + // Apply pending debt redistribution to this CDP + _cdp.debt = _newDebt; + } } } + emit CdpUpdated( + _cdpId, + ISortedCdps(sortedCdps).getOwnerAddress(_cdpId), + msg.sender, + prevDebt, + prevCollShares, + _newDebt, + _newColl, + _cdp.stake, + CdpOperation.syncAccounting + ); } } - function _emitCdpUpdatedEventForSyncAccounting( - bytes32 _cdpId, - Cdp storage _cdp, - uint256 _newDebt, - uint256 _newColl - ) internal { - emit CdpUpdated( - _cdpId, - ISortedCdps(sortedCdps).getOwnerAddress(_cdpId), - msg.sender, - _cdp.debt, - _cdp.coll, - _newDebt, - _newColl, - _cdp.stake, - CdpOperation.syncAccounting - ); - } - // Remove borrower's stake from the totalStakes sum, and set their stake to 0 function _removeStake(bytes32 _cdpId) internal { uint256 _newTotalStakes = totalStakes - Cdps[_cdpId].stake; @@ -560,44 +574,27 @@ contract CdpManagerStorage is EbtcBase, ReentrancyGuard, ICdpManagerData, AuthNo // Apply accumulated fee split distributed to the CDP // and update its accumulator tracker accordingly function _applyAccumulatedFeeSplit( - bytes32 _cdpId + bytes32 _cdpId, + uint256 _newColl, + uint256 _feeSplitDistributed, + uint256 _oldPerUnitCdp, + uint256 _systemStEthFeePerUnitIndex ) internal returns (uint256, uint256, uint256, uint256, uint256) { - // TODO Ensure global states like systemStEthFeePerUnitIndex get timely updated - // whenever there is a CDP modification operation, - // such as opening, closing, adding collateral, repaying debt, or liquidating - // OR Should we utilize some bot-keeper to work the routine job at fixed interval? - _syncGlobalAccounting(); - - uint256 _oldPerUnitCdp = cdpStEthFeePerUnitIndex[_cdpId]; - uint256 _systemStEthFeePerUnitIndex = systemStEthFeePerUnitIndex; - - ( - uint256 _newColl, - uint256 _newDebt, - uint256 _feeSplitDistributed, - uint _pendingDebt, - uint256 _debtIndexDelta - ) = _calcSyncedAccounting(_cdpId, _oldPerUnitCdp, _systemStEthFeePerUnitIndex); - // apply split fee to given CDP - if (_feeSplitDistributed > 0) { - Cdps[_cdpId].coll = _newColl; + Cdps[_cdpId].coll = _newColl; - emit CdpFeeSplitApplied( - _cdpId, - _oldPerUnitCdp, - _systemStEthFeePerUnitIndex, - _feeSplitDistributed, - _newColl - ); - } + emit CdpFeeSplitApplied( + _cdpId, + _oldPerUnitCdp, + _systemStEthFeePerUnitIndex, + _feeSplitDistributed, + _newColl + ); // sync per stake index for given CDP if (_oldPerUnitCdp != _systemStEthFeePerUnitIndex) { cdpStEthFeePerUnitIndex[_cdpId] = _systemStEthFeePerUnitIndex; } - - return (_newColl, _newDebt, _feeSplitDistributed, _pendingDebt, _debtIndexDelta); } // return the applied split fee(scaled by 1e18) and the resulting CDP collateral amount after applied From ca863c6c82d5146358c9260cf13d88098c17f892 Mon Sep 17 00:00:00 2001 From: dapp-whisperer Date: Sun, 15 Oct 2023 18:58:25 -0400 Subject: [PATCH 3/5] lint --- packages/contracts/contracts/CdpManagerStorage.sol | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/contracts/contracts/CdpManagerStorage.sol b/packages/contracts/contracts/CdpManagerStorage.sol index 571a99298..2cdfe384d 100644 --- a/packages/contracts/contracts/CdpManagerStorage.sol +++ b/packages/contracts/contracts/CdpManagerStorage.sol @@ -366,9 +366,15 @@ contract CdpManagerStorage is EbtcBase, ReentrancyGuard, ICdpManagerData, AuthNo // Apply Fee Split if (_feeSplitDistributed > 0) { - _applyAccumulatedFeeSplit(_cdpId, _newColl, _feeSplitDistributed, _oldPerUnitCdp, _systemStEthFeePerUnitIndex); + _applyAccumulatedFeeSplit( + _cdpId, + _newColl, + _feeSplitDistributed, + _oldPerUnitCdp, + _systemStEthFeePerUnitIndex + ); } - + // Apply Debt Redistribution if (_debtIndexDelta > 0) { _updateRedistributedDebtIndex(_cdpId); From ddbbf6d9cb8b48679fc58eb111a3972c57e644f4 Mon Sep 17 00:00:00 2001 From: dapp-whisperer Date: Sun, 15 Oct 2023 19:17:49 -0400 Subject: [PATCH 4/5] MultiCdpGetter uses synced values --- packages/contracts/contracts/MultiCdpGetter.sol | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/contracts/contracts/MultiCdpGetter.sol b/packages/contracts/contracts/MultiCdpGetter.sol index 04a023e87..22ba18972 100644 --- a/packages/contracts/contracts/MultiCdpGetter.sol +++ b/packages/contracts/contracts/MultiCdpGetter.sol @@ -84,8 +84,8 @@ contract MultiCdpGetter { for (uint256 idx = 0; idx < _count; ++idx) { _cdps[idx].id = currentCdpId; ( - _cdps[idx].debt, - _cdps[idx].coll, + , + , _cdps[idx].stake, /* status */ /* arrayIndex */ @@ -94,6 +94,7 @@ contract MultiCdpGetter { ) = cdpManager.Cdps(currentCdpId); + (_cdps[idx].debt, _cdps[idx].coll) = cdpManager.getSyncedDebtAndCollShares(currentCdpId); (_cdps[idx].snapshotEBTCDebt) = cdpManager.cdpDebtRedistributionIndex(currentCdpId); currentCdpId = sortedCdps.getNext(currentCdpId); @@ -119,8 +120,8 @@ contract MultiCdpGetter { for (uint256 idx = 0; idx < _count; ++idx) { _cdps[idx].id = currentCdpId; ( - _cdps[idx].debt, - _cdps[idx].coll, + , + , _cdps[idx].stake, /* status */ /* arrayIndex */ @@ -129,6 +130,7 @@ contract MultiCdpGetter { ) = cdpManager.Cdps(currentCdpId); + (_cdps[idx].debt, _cdps[idx].coll) = cdpManager.getSyncedDebtAndCollShares(currentCdpId); (_cdps[idx].snapshotEBTCDebt) = cdpManager.cdpDebtRedistributionIndex(currentCdpId); currentCdpId = sortedCdps.getPrev(currentCdpId); From 8a465fccae2080c9c7f8d325f453670c05d6defb Mon Sep 17 00:00:00 2001 From: dapp-whisperer Date: Mon, 16 Oct 2023 19:44:48 -0400 Subject: [PATCH 5/5] sync coll index regardless of fee split --- packages/contracts/contracts/CdpManagerStorage.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/contracts/contracts/CdpManagerStorage.sol b/packages/contracts/contracts/CdpManagerStorage.sol index 2cdfe384d..81e4d4c3c 100644 --- a/packages/contracts/contracts/CdpManagerStorage.sol +++ b/packages/contracts/contracts/CdpManagerStorage.sol @@ -398,6 +398,11 @@ contract CdpManagerStorage is EbtcBase, ReentrancyGuard, ICdpManagerData, AuthNo CdpOperation.syncAccounting ); } + + // sync per stake index for given CDP + if (_oldPerUnitCdp != _systemStEthFeePerUnitIndex) { + cdpStEthFeePerUnitIndex[_cdpId] = _systemStEthFeePerUnitIndex; + } } // Remove borrower's stake from the totalStakes sum, and set their stake to 0 @@ -596,11 +601,6 @@ contract CdpManagerStorage is EbtcBase, ReentrancyGuard, ICdpManagerData, AuthNo _feeSplitDistributed, _newColl ); - - // sync per stake index for given CDP - if (_oldPerUnitCdp != _systemStEthFeePerUnitIndex) { - cdpStEthFeePerUnitIndex[_cdpId] = _systemStEthFeePerUnitIndex; - } } // return the applied split fee(scaled by 1e18) and the resulting CDP collateral amount after applied