Proposal 64: Fix COMP distribution bug (#165)

* 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 Co-authored-by: Aryeh Greenberg <[email protected]>
compound-finance · Oct 12, 2021 · fcf067f · fcf067f
1 parent 4aa526d
commit fcf067f
Show file tree

Hide file tree

Showing 5 changed files with 201 additions and 36 deletions.
diff --git a/contracts/Comptroller.sol b/contracts/Comptroller.sol
@@ -1077,39 +1077,6 @@ contract Comptroller is ComptrollerV6Storage, ComptrollerInterface, ComptrollerE
     function _become(Unitroller unitroller) public {
         require(msg.sender == unitroller.admin(), "only unitroller admin can change brains");
         require(unitroller._acceptImplementation() == 0, "change not authorized");
-
-        // TODO: Remove this post upgrade
-        Comptroller(address(unitroller))._upgradeSplitCompRewards();
-    }
-
-    function _upgradeSplitCompRewards() public {
-        require(msg.sender == comptrollerImplementation, "only brains can become itself");
-
-        uint32 blockNumber = safe32(getBlockNumber(), "block number exceeds 32 bits");
-
-        // compSpeeds -> compBorrowSpeeds & compSupplySpeeds t
-        for (uint i = 0; i < allMarkets.length; i ++) {
-            compBorrowSpeeds[address(allMarkets[i])] = compSupplySpeeds[address(allMarkets[i])] = compSpeeds[address(allMarkets[i])];
-            delete compSpeeds[address(allMarkets[i])];
-
-            /*
-             * Ensure supply and borrow state indices are all set. If not set, update to default value
-             */
-            CompMarketState storage supplyState = compSupplyState[address(allMarkets[i])];
-            CompMarketState storage borrowState = compBorrowState[address(allMarkets[i])];
-
-            if (supplyState.index == 0) {
-                // Initialize supply state index with default value
-                supplyState.index = compInitialIndex;
-                supplyState.block = blockNumber;
-            }
-
-            if (borrowState.index == 0) {
-                // Initialize borrow state index with default value
-                borrowState.index = compInitialIndex;
-                borrowState.block = blockNumber;
-            }
-        }
     }
 
     /**
@@ -1214,7 +1181,7 @@ contract Comptroller is ComptrollerV6Storage, ComptrollerInterface, ComptrollerE
         // Update supplier's index to the current index since we are distributing accrued COMP
         compSupplierIndex[cToken][supplier] = supplyIndex;
 
-        if (supplierIndex == 0 && supplyIndex > compInitialIndex) {
+        if (supplierIndex == 0 && supplyIndex >= compInitialIndex) {
             // Covers the case where users supplied tokens before the market's supply state index was set.
             // Rewards the user with COMP accrued from the start of when supplier rewards were first
             // set for the market.
@@ -1253,7 +1220,7 @@ contract Comptroller is ComptrollerV6Storage, ComptrollerInterface, ComptrollerE
         // Update borrowers's index to the current index since we are distributing accrued COMP
         compBorrowerIndex[cToken][borrower] = borrowIndex;
 
-        if (borrowerIndex == 0 && borrowIndex > compInitialIndex) {
+        if (borrowerIndex == 0 && borrowIndex >= compInitialIndex) {
             // Covers the case where users borrowed tokens before the market's borrow state index was set.
             // Rewards the user with COMP accrued from the start of when borrower rewards were first
             // set for the market.
@@ -1348,6 +1315,18 @@ 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/scenario/src/Contract/Comptroller.ts b/scenario/src/Contract/Comptroller.ts
@@ -58,7 +58,8 @@ interface ComptrollerMethods {
   compSpeeds(string): Callable<string>
   compSupplySpeeds(string): Callable<string>
   compBorrowSpeeds(string): Callable<string>
-  claimComp(string): Sendable<void>
+  claimComp(holder: string): Sendable<void>
+  claimComp(holder: string, cTokens: string[]): Sendable<void>
   updateContributorRewards(account: string): Sendable<void>
   _grantComp(account: string, encodedNumber): Sendable<void>
   _setCompRate(encodedNumber): Sendable<void>

diff --git a/scenario/src/Event/ComptrollerEvent.ts b/scenario/src/Event/ComptrollerEvent.ts
@@ -248,6 +248,18 @@ async function claimComp(world: World, from: string, comptroller: Comptroller, h
   return world;
 }
 
+async function claimCompInMarkets(world: World, from: string, comptroller: Comptroller, holder: string, cTokens: CToken[]): Promise<World> {
+  let invokation = await invoke(world, comptroller.methods.claimComp(holder, cTokens.map(c => c._address)), from, ComptrollerErrorReporter);
+
+  world = addAction(
+    world,
+    `Comp claimed by ${holder} in markets ${cTokens.map(c => c.name)}`,
+    invokation
+  );
+
+  return world;
+}
+
 async function updateContributorRewards(world: World, from: string, comptroller: Comptroller, contributor: string): Promise<World> {
   let invokation = await invoke(world, comptroller.methods.updateContributorRewards(contributor), from, ComptrollerErrorReporter);
 
@@ -760,6 +772,20 @@ export function comptrollerCommands() {
       ],
       (world, from, {comptroller, holder}) => claimComp(world, from, comptroller, holder.val)
     ),
+    new Command<{comptroller: Comptroller, holder: AddressV, cTokens: CToken[]}>(`
+      #### ClaimCompInMarkets
+
+      * "Comptroller ClaimComp <holder> (<CToken> ...)" - Claims comp
+      * E.g. "Comptroller ClaimCompInMarkets Geoff (cDAI cBAT)
+      `,
+      "ClaimCompInMarkets",
+      [
+        new Arg("comptroller", getComptroller, {implicit: true}),
+        new Arg("holder", getAddressV),
+        new Arg("cTokens", getCTokenV, {mapped: true})
+      ],
+      (world, from, {comptroller, holder, cTokens}) => claimCompInMarkets(world, from, comptroller, holder.val, cTokens)
+    ),
     new Command<{comptroller: Comptroller, contributor: AddressV}>(`
       #### UpdateContributorRewards
 

diff --git a/spec/scenario/Flywheel/Flywheel.scen b/spec/scenario/Flywheel/Flywheel.scen
@@ -261,6 +261,10 @@ Test "New COMP speeds apply to both prior borrowers+suppliers and later borrower
     Assert Equal (Erc20 COMP TokenBalance Geoff) 1000
     Assert Equal (Erc20 COMP TokenBalance Jared) 1000
     Assert Equal (Erc20 COMP TokenBalance Torrey) 1000
+    Assert Equal (Comptroller CompAccrued Coburn) 0
+    Assert Equal (Comptroller CompAccrued Geoff) 0
+    Assert Equal (Comptroller CompAccrued Jared) 0
+    Assert Equal (Comptroller CompAccrued Torrey) 0
     Assert Equal (Comptroller CompBorrowerIndex cYFI Coburn) (Comptroller CompBorrowerIndex cYFI Geoff)
     Assert Equal (Comptroller CompSupplierIndex cYFI Jared) (Comptroller CompSupplierIndex cYFI Torrey)
 
@@ -301,5 +305,51 @@ Test "New COMP speeds apply to both prior borrowers+suppliers and later borrower
     Assert Equal (Erc20 COMP TokenBalance Geoff) 1000
     Assert Equal (Erc20 COMP TokenBalance Jared) 1000
     Assert Equal (Erc20 COMP TokenBalance Torrey) 1000
+    Assert Equal (Comptroller CompAccrued Coburn) 0
+    Assert Equal (Comptroller CompAccrued Geoff) 0
+    Assert Equal (Comptroller CompAccrued Jared) 0
+    Assert Equal (Comptroller CompAccrued Torrey) 0
+    Assert Equal (Comptroller CompBorrowerIndex cYFI Coburn) (Comptroller CompBorrowerIndex cYFI Geoff)
+    Assert Equal (Comptroller CompSupplierIndex cYFI Jared) (Comptroller CompSupplierIndex cYFI Torrey)
+
+Test "Zero COMP speed markets don't accrue rewards w/ uninitialized borrower/supplier state indices"
+    -- Supplying
+    --   Torrey mints 100e18 before COMP speeds set
+    --   Jared mints 100e18 after COMP speeds set
+    -- Borrowing
+    --   Coburn borrows 10e18 before COMP speeds set
+    --   Geoff borrows 10e18 after COMP speeds set
+    FlywheelComptroller
+    InitUsage2
+    -- New token with no rewards
+    NewCToken YFI cYFI
+    Support cYFI collateralFactor:0.5
+    Prep Torrey 100e18 YFI cYFI
+    Mint Torrey 100e18 cYFI
+    Prep Jared 100e18 YFI cYFI
+    Borrow Coburn 10e18 cYFI
+    EnterMarkets Geoff cZRX
+    -- Set borrower and supplier state indices to 0 (uninitialized - before all market state indices were initialized properly)
+    Comptroller Send "setCompBorrowerIndex(address,address,uint256)" (Address cYFI) (Address Coburn) 0
+    Comptroller Send "setCompSupplierIndex(address,address,uint256)" (Address cYFI) (Address Torrey) 0
+    Mint Jared 100e18 cYFI
+    Borrow Geoff 10e18 cYFI
+    Assert Equal (Comptroller CompBorrowerIndex cYFI Coburn) 0
+    Assert Equal (Comptroller CompBorrowerIndex cYFI Geoff) 1e36
+    Assert Equal (Comptroller CompSupplierIndex cYFI Torrey) 0
+    Assert Equal (Comptroller CompSupplierIndex cYFI Jared) 1e36
+    FastForward 1000 blocks
+    Comptroller ClaimComp Jared
+    Comptroller ClaimComp Torrey
+    Comptroller ClaimComp Geoff
+    Comptroller ClaimComp Coburn
+    Assert Equal (Erc20 COMP TokenBalance Coburn) 0
+    Assert Equal (Erc20 COMP TokenBalance Geoff) 0
+    Assert Equal (Erc20 COMP TokenBalance Jared) 0
+    Assert Equal (Erc20 COMP TokenBalance Torrey) 0
+    Assert Equal (Comptroller CompAccrued Coburn) 0
+    Assert Equal (Comptroller CompAccrued Geoff) 0
+    Assert Equal (Comptroller CompAccrued Jared) 0
+    Assert Equal (Comptroller CompAccrued Torrey) 0
     Assert Equal (Comptroller CompBorrowerIndex cYFI Coburn) (Comptroller CompBorrowerIndex cYFI Geoff)
     Assert Equal (Comptroller CompSupplierIndex cYFI Jared) (Comptroller CompSupplierIndex cYFI Torrey)
diff --git a/spec/sim/0064-fix-comp-accruals/hypothetical_mainnet_upgrade.scen b/spec/sim/0064-fix-comp-accruals/hypothetical_mainnet_upgrade.scen
@@ -0,0 +1,109 @@
+#!/usr/bin/env -S yarn repl -s
+
+PrintTransactionLogs
+
+-- Token holder addresses for mocking
+Alias CompHolder "0x7587caefc8096f5f40acb83a09df031a018c66ec"
+Alias CUSDCHolder "0xF977814e90dA44bFA03b6295A0616a897441aceC" -- Binance 8
+Alias DaiHolder "0x5f65f7b609678448494De4C87521CdF6cEf1e932" -- Gemini 4
+
+Alias Timelock "0x6d903f6003cca6255D85CcA4D3B5E5146dC33925"
+
+-- Fork the block prior to proposal 62 executing
+Web3Fork "https://mainnet-eth.compound.finance/@13322797" (CompHolder CUSDCHolder DaiHolder Timelock)
+UseConfigs mainnet
+
+-- Disable USDC COMP rewards so that these rewards won't mess with our results
+Send Timelock 1e18
+From Timelock (Comptroller SetCompSpeed cUSDC 0)
+
+-- Send USDC from CUSDCHolder to other accounts
+From CUSDCHolder (Erc20 USDC Transfer Geoff 1000000e6)
+From CUSDCHolder (Erc20 USDC Transfer Torrey 1000000e6)
+From CUSDCHolder (Erc20 USDC Transfer Coburn 1000000e6)
+From CUSDCHolder (Erc20 USDC Transfer Jared 1000000e6)
+
+-- Send DAI from DaiHolder to other accounts
+From DaiHolder (Erc20 DAI Transfer Coburn 1000000e18)
+From DaiHolder (Erc20 DAI Transfer Jared 1000000e18)
+
+-- Pre Proposal 62: Deposit collateral and borrow SUSHI
+From Geoff (Erc20 USDC Approve (Address cUSDC) 1000000e6)
+From Geoff (CToken cUSDC Mint 1000000e6)
+From Geoff (Comptroller EnterMarkets (cUSDC))
+From Geoff (CToken cSUSHI Borrow 1000e18)
+
+From Torrey (Erc20 USDC Approve (Address cUSDC) 1000000e6)
+From Torrey (CToken cUSDC Mint 1000000e6)
+From Torrey (Comptroller EnterMarkets (cUSDC))
+From Torrey (CToken cSUSHI Borrow 1000e18)
+
+-- Execute proposal 62
+GovernorBravo GovernorBravo Proposal LastProposal Execute
+MineBlock
+
+-- Claim COMP for Geoff
+Comptroller ClaimComp Geoff
+
+-- Check Geoff COMP rewards - invalid accrual & COMP sent
+Assert Equal (Erc20 COMP TokenBalance Geoff) (988792275103122749560) -- Huge amount (988 COMP) -> Bug
+Assert Equal (Comptroller CompAccrued Geoff) (0)
+
+-- Cause Torrey to accrue COMP without claiming
+From Torrey (CToken cSUSHI Borrow 1e18) -- Causes COMP to be distributed but not claimed
+
+-- Deploy latest Comptroller
+ComptrollerImpl Deploy Standard NewComptroller
+
+-- Delegate and propose update (containing bug fix)
+From CompHolder (Comp Delegate CompHolder)
+From CompHolder (GovernorBravo GovernorBravo Propose "Upgrade Comptroller" [(Address Unitroller) (Address NewComptroller)] [0 0] ["_setPendingImplementation(address)" "_become(address)"] [[(Address NewComptroller)] [(Address Unitroller)]])
+
+-- Fast forward, vote, queue, execute
+MineBlock
+AdvanceBlocks 14000
+From CompHolder (GovernorBravo GovernorBravo Proposal LastProposal Vote For)
+AdvanceBlocks 20000
+GovernorBravo GovernorBravo Proposal LastProposal Queue
+IncreaseTime 604910
+GovernorBravo GovernorBravo Proposal LastProposal Execute
+MineBlock
+
+-- Claim COMP for Torrey
+Comptroller ClaimComp Torrey
+
+-- Check Torrey COMP balance changes - invalid accrual & COMP not sent
+Assert Equal (Erc20 COMP TokenBalance Torrey) (0)
+Assert Equal (Comptroller CompAccrued Torrey) (988792086947769887785) -- Huge amount (988 COMP) -> Bug
+
+-- Post bug fix: Deposit collateral (DAI) and borrow SUSHI - block COMP sending
+From Coburn (Erc20 DAI Approve (Address cDAI) 1000000e18)
+From Coburn (CToken cDAI Mint 1000000e18)
+From Coburn (Comptroller EnterMarkets (cDAI))
+From Coburn (CToken cSUSHI Borrow 1000e18)
+
+-- Post bug fix: Deposit collateral (DAI) and borrow BAT - don't block COMP sending
+From Jared (Erc20 DAI Approve (Address cDAI) 1000000e18)
+From Jared (CToken cDAI Mint 1000000e18)
+From Jared (Comptroller EnterMarkets (cDAI))
+From Jared (CToken cBAT Borrow 1000e18)
+
+-- Accrue rewards (if any)
+MineBlock
+AdvanceBlocks 14000
+
+-- Claim COMP for Coburn
+Comptroller ClaimCompInMarkets Coburn (cDAI cSUSHI)
+
+-- Claim COMP for Jared
+Comptroller ClaimCompInMarkets Jared (cDAI cBAT)
+
+-- Check Coburn COMP balance changes - valid accrual & COMP not sent (claim in affected market)
+Assert Equal (Erc20 COMP TokenBalance Coburn) (0)
+Assert Equal (Comptroller CompAccrued Coburn) (211455443766873733) -- 0.21 COMP accrued
+
+-- Check Jared COMP balance changes - valid accrual & COMP sent (no claim in affected market)
+Assert Equal (Erc20 COMP TokenBalance Jared) (212379370589809042) -- 0.21 COMP claimed
+Assert Equal (Comptroller CompAccrued Jared) (0)
+
+Print "Done"