Proposal 65: Correct bad COMP accruals (#173)

* Add Arr00's changes for split COMP distributions - Adds compBorrowSpeeds and compSupplySpeeds to ComptrollerV6Storage - Change _setCompSpeed to _setCompSpeeds - one function call to update distribution speeds for multiple markets - Change signature of setCompSpeedInternal - compSpeed split up into two parts: compSupplySpeed and compBorrowSpeed * Update setCompSpeedInternal to update COMP speeds and accruals using compSupplySpeeds and compBorrowSpeeds * Fix compilation errors in Comptroller * Update tests to use new split COMP distribution changes * Update tests and frameworks to support the new COMP distributions * Add tests for setting varying supply and borrow rates * Fix bugs in Comptroller#updateCompSupplyIndex and Comptroller#updateCompBorrowIndex - compSupplySpeeds and compBorrowSpeeds were flipped * Add tests for one-sided COMP distribution rates - These tests have been manually verified to fail when they are expected to fail => they work well * Add upgrade hooks for split COMP reward distributions * Add TODO * Update gas costs * Add market initialization and update market state info - Supply state and borrow state now initialized when a market is added, ensuring market state indices are always set * Modify upgrade hook to ensure all market state indices are set * Simplify setCompSpeedInternal logic - Now that market state indices are non lazily set, we can simply update market states when updating COMP speeds * Adjust comments in Comptroller * Add COMP accrual optimizations - Adds changes from PR #173 - Second mint with comp accrued: reduced by 695 - Claim comp: reduced by 1268 * Add comments to accrual functions in Comptroller * Update FlywheelTest to account for initialization of borrow/supply state indices * Update Flywheel scenario test to use the latest Comptroller - Old scen test used ComptrollerG3 - Removed test for removed function which never actually worked * Update mainnet network config and abi to current (taken from compound-config) * Fix split COMP rewards upgrade hook - We need to call the upgrade function using the delegator address to use the storage at that contract address * Add Comptroller upgrade simulation - Asserts that the upgrade proposal passes - Asserts that old comp speeds are deleted - Asserts that new comp speeds are the same as they were previously - Asserts that market state indices are the same post upgrade, with 0 values initialized to 1e36 - Asserts that the 2 comp speed bugs were fixed - Asserts that the new _setCompSpeeds function works - Asserts that comp rewards accrual works * Refactor updateCompSupplyIndex and updateCompBorrowIndex slightly - Stores blockNumber as uint32 for safety * Update gas costs * Make style adjustment and fix typo * Update uninitialized rewards market block numbers in _upgradeSplitCompRewards * new branch * add new scen * add cases to new flywheel scen * Fix bug in distributeBorrowerComp identified by Coburn - Note: This change significantly increases gas usage (claim gas usage increased by 12K iirc) * Improve distributeBorrowerComp efficiency - Don't distribute borrower COMP (or update the user's borrow state) if the user is not in the borrower market. - Decreases gas usage of claim by 35K * Simplify distributeBorrowerComp - Removed the (borrowerIndex > 0) check - Since market state indices are always set now, this condition will always evaluate to true * Improve logic in borrowAllowed - It's possible that a borrower can be added to a market even if borrowAllowed returns an error. This can lead to inefficiences and possibly other problems. - This commit prevents this from happening * Add number fetcher to scenario runner * Move test from CompSpeed to Flywheel * Add new CompSpeed scenario tests * Remove old Flywheel scenario tests * Rename new Flywheel scen test to the old name * Add tests to Flywheel scenario tests - Ensures supply and borrow states properly initialized - Ensures COMP is accrued correctly when COMP rewards are added (after market activation), removed, then added again - This test covers the COMP speed bug identified on August 09, 2021 * Revert market membership changes - The logic didn't flow properly * Fix subtraction underflow problem - Underflow occurs when calling distributeBorrowerComp on markets whose borrowState.index didn't start at 1e36. - The COMP rewards on those markets work fine, so we just need to make a fix for people borrowing cMKR, cAAVE, and the other new markets whose indices haven't been set yet, which this commit does. * Increase gas limit in claimComp test - The change to distributeBorrowerComp to account for borrowers with uninitialized state indices increased the gas costs unfortunately * Add Coburn's scenario test for the flywheel - Ensures new COMP speeds apply to both prior borrowers+suppliers and later borrowers+suppliers correctly * Add another test to Flywheel scen tests - Ensures new COMP speeds apply to both prior borrowers+suppliers and later borrowers+suppliers correctly w/ uninitialized prior borrower/supplier state indices * Upgrade Grants simulation tests * Make distributeSupplierComp logic match distributeBorrowerComp logic * Update gas costs - Oof, the necessary change to distributeSupplierComp increased the gas cost of claim - Lastly, gm * Remove upgrade hook * Temporarily pause COMP rewards * Add failing test case for the proposal 62 bug * Fix the bug introduced in proposal 62 - The failing test of the prior commit is now passing * Add pessimistic authorizaion of transferring COMP rewards - There are COMP accrual values we need to fix before we can fully allow this - Allows COMP to be transferred to only the users which haven't interacted with the problematic markets * Remove forgotten merge conflict * Remove upgrade hook * Add ClaimCompInMarkets to the scenario runner * Add proposal 64 scenario test - Ensures proposal 64 does what it's expected to do - Please use the current mainnet configurations for this to run properly * Add additional checks of COMP accrued in the flywheel scenario tests * Add storage variable for proposal 65 fix execution * Add fixBadAccruals function to correct the accounting errors introduced in proposal 62 * Add accounting storage mapping (compReceivable) - Keeps track of the amount of COMP users owe the protocol * Add CompAccruedAdjusted and CompReceivableUpdated events * Add PrintNumber and DebugNumber to repl * Only adjust compAccrued if amountToSubtract > 0 * Make compReceivable public * Add CompReceivable view to repl * Add proposal 65 simulation - Verified that it passes * Add revert assertion to proposal 65 simulation * Revert grantCompInternal restrictions * Update proposal 65 simulation - Adds proposal 64 execution - Tests for restored claimComp functionality - Verified that the simulation passes * Add mainnet upgrade simulation - This simulation executes proposal 65 and verifies that it works as expected Co-authored-by: TRiLeZ <[email protected]>
compound-finance · Nov 16, 2021 · 9e93e85 · 9e93e85
1 parent 3bb6de7
commit 9e93e85
Show file tree

Hide file tree

Showing 7 changed files with 287 additions and 13 deletions.
diff --git a/contracts/Comptroller.sol b/contracts/Comptroller.sol
@@ -12,7 +12,7 @@ import "./Governance/Comp.sol";
  * @title Compound's Comptroller Contract
  * @author Compound
  */
-contract Comptroller is ComptrollerV6Storage, ComptrollerInterface, ComptrollerErrorReporter, ExponentialNoError {
+contract Comptroller is ComptrollerV7Storage, ComptrollerInterface, ComptrollerErrorReporter, ExponentialNoError {
     /// @notice Emitted when an admin supports a market
     event MarketListed(CToken cToken);
 
@@ -67,6 +67,12 @@ contract Comptroller is ComptrollerV6Storage, ComptrollerInterface, ComptrollerE
     /// @notice Emitted when COMP is granted by admin
     event CompGranted(address recipient, uint amount);
 
+    /// @notice Emitted when COMP accrued for a user has been manually adjusted.
+    event CompAccruedAdjusted(address indexed user, uint oldCompAccrued, uint newCompAccrued);
+
+    /// @notice Emitted when COMP receivable for a user has been updated.
+    event CompReceivableUpdated(address indexed user, uint oldCompReceivable, uint newCompReceivable);
+
     /// @notice The initial COMP index for a market
     uint224 public constant compInitialIndex = 1e36;
 
@@ -1079,6 +1085,54 @@ contract Comptroller is ComptrollerV6Storage, ComptrollerInterface, ComptrollerE
         require(unitroller._acceptImplementation() == 0, "change not authorized");
     }
 
+    /// @notice Delete this function after proposal 65 is executed
+    function fixBadAccruals(address[] calldata affectedUsers, uint[] calldata amounts) external {
+        require(msg.sender == admin, "Only admin can call this function"); // Only the timelock can call this function
+        require(!proposal65FixExecuted, "Already executed this one-off function"); // Require that this function is only called once
+        require(affectedUsers.length == amounts.length, "Invalid input");
+
+        // Loop variables
+        address user;
+        uint currentAccrual;
+        uint amountToSubtract;
+        uint newAccrual;
+
+        // Iterate through all affected users
+        for (uint i = 0; i < affectedUsers.length; ++i) {
+            user = affectedUsers[i];
+            currentAccrual = compAccrued[user];
+
+            amountToSubtract = amounts[i];
+
+            // The case where the user has claimed and received an incorrect amount of COMP.
+            // The user has less currently accrued than the amount they incorrectly received.
+            if (amountToSubtract > currentAccrual) {
+                // Amount of COMP the user owes the protocol
+                uint accountReceivable = amountToSubtract - currentAccrual; // Underflow safe since amountToSubtract > currentAccrual
+
+                uint oldReceivable = compReceivable[user];
+                uint newReceivable = add_(oldReceivable, accountReceivable);
+
+                // Accounting: record the COMP debt for the user
+                compReceivable[user] = newReceivable;
+
+                emit CompReceivableUpdated(user, oldReceivable, newReceivable);
+
+                amountToSubtract = currentAccrual;
+            }
+
+            if (amountToSubtract > 0) {
+                // Subtract the bad accrual amount from what they have accrued.
+                // Users will keep whatever they have correctly accrued.
+                compAccrued[user] = newAccrual = sub_(currentAccrual, amountToSubtract);
+
+                emit CompAccruedAdjusted(user, currentAccrual, newAccrual);
+            }
+        }
+
+        proposal65FixExecuted = true; // Makes it so that this function cannot be called again
+    }
+
     /**
      * @notice Checks caller is admin, or this contract is becoming the new implementation
      */
@@ -1315,18 +1369,6 @@ contract Comptroller is ComptrollerV6Storage, ComptrollerInterface, ComptrollerE
      * @return The amount of COMP which was NOT transferred to the user
      */
     function grantCompInternal(address user, uint amount) internal returns (uint) {
-        for (uint i = 0; i < allMarkets.length; ++i) {
-            address market = address(allMarkets[i]);
-
-            bool noOriginalSpeed = compBorrowSpeeds[market] == 0;
-            bool invalidSupply = noOriginalSpeed && compSupplierIndex[market][user] > 0;
-            bool invalidBorrow = noOriginalSpeed && compBorrowerIndex[market][user] > 0;
-
-            if (invalidSupply || invalidBorrow) {
-                return amount;
-            }
-        }
-
         Comp comp = Comp(getCompAddress());
         uint compRemaining = comp.balanceOf(address(this));
         if (amount > 0 && amount <= compRemaining) {

diff --git a/contracts/ComptrollerStorage.sol b/contracts/ComptrollerStorage.sol
@@ -151,3 +151,11 @@ contract ComptrollerV6Storage is ComptrollerV5Storage {
     /// @notice The rate at which comp is distributed to the corresponding supply market (per block)
     mapping(address => uint) public compSupplySpeeds;
 }
+
+contract ComptrollerV7Storage is ComptrollerV6Storage {
+    /// @notice Flag indicating whether the function to fix COMP accruals has been executed (RE: proposal 62 bug)
+    bool public proposal65FixExecuted;
+
+    /// @notice Accounting storage mapping account addresses to how much COMP they owe the protocol.
+    mapping(address => uint) public compReceivable;
+}
diff --git a/scenario/src/Contract/Comptroller.ts b/scenario/src/Contract/Comptroller.ts
@@ -53,6 +53,7 @@ interface ComptrollerMethods {
   compSupplyState(string): Callable<string>
   compBorrowState(string): Callable<string>
   compAccrued(string): Callable<string>
+  compReceivable(string): Callable<string>
   compSupplierIndex(market: string, account: string): Callable<string>
   compBorrowerIndex(market: string, account: string): Callable<string>
   compSpeeds(string): Callable<string>

diff --git a/scenario/src/CoreEvent.ts b/scenario/src/CoreEvent.ts
@@ -244,6 +244,17 @@ export const commands: (View<any> | ((world: World) => Promise<View<any>>))[] =
     [new Arg('message', getStringV)],
     async (world, { message }) => print(world, message.val)
   ),
+  new View<{ num: NumberV }>(
+    `
+      #### PrintNumber
+
+      * "Print ..." - Prints given number
+        * E.g. "Print \"Hello there\""
+    `,
+    'PrintNumber',
+    [new Arg('num', getNumberV)],
+    async (world, { num }) => print(world, num.toString())
+  ),
   new View<{}>(
     `
       #### PrintTransactionLogs
@@ -487,6 +498,17 @@ export const commands: (View<any> | ((world: World) => Promise<View<any>>))[] =
     async (world, { message }) => inspect(world, message.val)
   ),
 
+  new View<{ num: NumberV }>(
+    `
+      #### DebugNumber
+
+      * "Debug num:<Number>" - Same as inspect but prepends with a number
+    `,
+    'DebugNumber',
+    [new Arg('num', getNumberV)],
+    async (world, { num }) => inspect(world, num.toString())
+  ),
+
   new View<{ account: AddressV; event: EventV }>(
     `
       #### From

diff --git a/scenario/src/Value/ComptrollerValue.ts b/scenario/src/Value/ComptrollerValue.ts
@@ -504,6 +504,21 @@ export function comptrollerFetchers() {
         return new NumberV(result);
       }
     ),
+    new Fetcher<{comptroller: Comptroller, account: AddressV, key: StringV}, NumberV>(`
+        #### CompReceivable(address)
+
+        * "Comptroller CompReceivable Coburn
+      `,
+      "CompReceivable",
+      [
+        new Arg("comptroller", getComptroller, {implicit: true}),
+        new Arg("account", getAddressV),
+      ],
+      async (world, {comptroller,account}) => {
+        const result = await comptroller.methods.compReceivable(account.val).call();
+        return new NumberV(result);
+      }
+    ),
     new Fetcher<{comptroller: Comptroller, CToken: CToken, account: AddressV}, NumberV>(`
         #### compSupplierIndex