Patches and proposal for mainnet USDC market upgrade (compound-financ…

…e#599) * Emit erc20 transfer (mint) event when absorbed account ends up with a positive balance Calculate amount by converting from principal instead of directly using present value Move Transfer event down Fix unit test * Proposal to patch in the absorb Transfer event * Add a scenario to check for new Transfer event * Add goerli and fuji migrations * Automatically include excess balance in collateral reserves Also adds a reserves fn which it checks when collateral is bought. Note: we could add a fn to convert any (prior) collateral balance of the contract into reserves by clearing it out and removing from total. However, collateral reserves are currently 0, and we expect them to remain 0 if liquidation is working properly. If somehow collateral is acquired and held by the protocol (i.e. from an absorb) before this patch gets applied, the protocol will simply hold that balance as a supplier, until governance decides to move it, for which a fn can always be added (at that time) to assist. * Rename migrations, update description and tidying * Remove irrelavant setup from unit test and fix proposer eth balance * Modified migration from GitHub Actions * Modified migration from GitHub Actions * Add a script to help testnet gov and improve interface for scripts (compound-finance#605) Co-authored-by: kevincheng96 <[email protected]> Co-authored-by: GitHub Actions Bot <>
melotik · Nov 1, 2022 · 376fdeb · 376fdeb
1 parent 61941ae
commit 376fdeb
Show file tree

Hide file tree

Showing 15 changed files with 499 additions and 79 deletions.
diff --git a/contracts/Comet.sol b/contracts/Comet.sol
@@ -477,7 +477,16 @@ contract Comet is CometMainInterface {
     }
 
     /**
-     * @notice Gets the total amount of protocol reserves, denominated in the number of base tokens
+     * @notice Gets the total balance of protocol collateral reserves for an asset
+     * @dev Note: Reverts if collateral reserves are somehow negative, which should not be possible
+     * @param asset The collateral asset
+     */
+    function getCollateralReserves(address asset) override public view returns (uint) {
+        return ERC20(asset).balanceOf(address(this)) - totalsCollateral[asset].totalSupplyAsset;
+    }
+
+    /**
+     * @notice Gets the total amount of protocol reserves of the base asset
      */
     function getReserves() override public view returns (int) {
         (uint64 baseSupplyIndex_, uint64 baseBorrowIndex_) = accruedInterestIndices(getNowInternal() - lastAccrualTime);
@@ -1136,7 +1145,7 @@ contract Comet is CometMainInterface {
                 address asset = assetInfo.asset;
                 uint128 seizeAmount = userCollateral[account][asset].balance;
                 userCollateral[account][asset].balance = 0;
-                userCollateral[address(this)][asset].balance += seizeAmount;
+                totalsCollateral[asset].totalSupplyAsset -= seizeAmount;
 
                 uint256 value = mulPrice(seizeAmount, getPrice(assetInfo.priceFeed), assetInfo.scale);
                 deltaValue += mulFactor(value, assetInfo.liquidationFactor);
@@ -1169,6 +1178,10 @@ contract Comet is CometMainInterface {
         uint256 basePaidOut = unsigned256(newBalance - oldBalance);
         uint256 valueOfBasePaidOut = mulPrice(basePaidOut, basePrice, uint64(baseScale));
         emit AbsorbDebt(absorber, account, basePaidOut, valueOfBasePaidOut);
+
+        if (newPrincipal > 0) {
+            emit Transfer(address(0), account, presentValueSupply(baseSupplyIndex, unsigned104(newPrincipal)));
+        }
     }
 
     /**
@@ -1190,10 +1203,12 @@ contract Comet is CometMainInterface {
 
         uint collateralAmount = quoteCollateral(asset, baseAmount);
         if (collateralAmount < minAmount) revert TooMuchSlippage();
+        if (collateralAmount > getCollateralReserves(asset)) revert InsufficientReserves();
 
         // Note: Pre-transfer hook can re-enter buyCollateral with a stale collateral ERC20 balance.
-        //       This is a problem if quoteCollateral derives its discount from the collateral ERC20 balance.
-        withdrawCollateral(address(this), recipient, asset, safe128(collateralAmount));
+        //  Assets should not be listed which allow re-entry from pre-transfer now, as too much collateral could be bought.
+        //  This is also a problem if quoteCollateral derives its discount from the collateral ERC20 balance.
+        doTransferOut(asset, recipient, safe128(collateralAmount));
 
         emit BuyCollateral(msg.sender, asset, baseAmount, collateralAmount);
     }

diff --git a/contracts/CometMainInterface.sol b/contracts/CometMainInterface.sol
@@ -79,6 +79,7 @@ abstract contract CometMainInterface is CometCore {
 
     function getAssetInfo(uint8 i) virtual public view returns (AssetInfo memory);
     function getAssetInfoByAddress(address asset) virtual public view returns (AssetInfo memory);
+    function getCollateralReserves(address asset) virtual public view returns (uint);
     function getReserves() virtual public view returns (int);
     function getPrice(address priceFeed) virtual public view returns (uint);
 

diff --git a/contracts/liquidator/Liquidator.sol b/contracts/liquidator/Liquidator.sol
@@ -259,7 +259,7 @@ contract Liquidator is IUniswapV3FlashCallback, PeripheryImmutableState, Periphe
         for (uint8 i = 0; i < numAssets; i++) {
             address asset = comet.getAssetInfo(i).asset;
             cometAssets[i] = asset;
-            uint256 collateralBalance = comet.collateralBalanceOf(address(comet), asset);
+            uint256 collateralBalance = comet.getCollateralReserves(asset);
 
             if (collateralBalance == 0) continue;
 

diff --git a/deployments/fuji/usdc/migrations/1665028496_absorb_transfer_event_and_auto_collateral.ts b/deployments/fuji/usdc/migrations/1665028496_absorb_transfer_event_and_auto_collateral.ts
@@ -0,0 +1,58 @@
+import { DeploymentManager, migration } from '../../../../plugins/deployment_manager';
+import { calldata, exp, proposal } from '../../../../src/deploy';
+
+import { expect } from 'chai';
+
+export default migration('1665028496_absorb_transfer_event_and_auto_collateral', {
+  async prepare(deploymentManager: DeploymentManager) {
+    const cometFactory = await deploymentManager.deploy('cometFactory', 'CometFactory.sol', [], true);
+    return { newFactoryAddress: cometFactory.address };
+  },
+
+  async enact(deploymentManager: DeploymentManager, { newFactoryAddress }) {
+    const trace = deploymentManager.tracer();
+    const ethers = deploymentManager.hre.ethers;
+
+    const {
+      governor,
+      comet,
+      configurator,
+      cometAdmin,
+      COMP,
+    } = await deploymentManager.getContracts();
+
+    const actions = [
+      // 1. Set comet factory to newly deployed factory
+      {
+        contract: configurator,
+        signature: 'setFactory(address,address)',
+        args: [comet.address, newFactoryAddress],
+      },
+
+      // 2. Deploy and upgrade to a new version of Comet
+      {
+        contract: cometAdmin,
+        signature: 'deployAndUpgradeTo(address,address)',
+        args: [configurator.address, comet.address],
+      },
+    ];
+    const description = "# Liquidation Event Handling And Collateral Reserves\n";
+    const txn = await deploymentManager.retry(
+      async () => governor.propose(...await proposal(actions, description))
+    );
+    trace(txn);
+
+    const event = (await txn.wait()).events.find(event => event.event === 'ProposalCreated');
+    const [proposalId] = event.args;
+    trace(`Created proposal ${proposalId}.`);
+  },
+
+  async enacted(deploymentManager: DeploymentManager): Promise<boolean> {
+    return true;
+  },
+
+  async verify(deploymentManager: DeploymentManager) {
+    // 1. & 2.
+    //  added a scenario to check for new Transfer event
+  }
+});
diff --git a/deployments/goerli/usdc/migrations/1665028496_absorb_transfer_event_and_auto_collateral.ts b/deployments/goerli/usdc/migrations/1665028496_absorb_transfer_event_and_auto_collateral.ts
@@ -0,0 +1,58 @@
+import { DeploymentManager, migration } from '../../../../plugins/deployment_manager';
+import { calldata, exp, proposal } from '../../../../src/deploy';
+
+import { expect } from 'chai';
+
+export default migration('1665028496_absorb_transfer_event_and_auto_collateral', {
+  async prepare(deploymentManager: DeploymentManager) {
+    const cometFactory = await deploymentManager.deploy('cometFactory', 'CometFactory.sol', [], true);
+    return { newFactoryAddress: cometFactory.address };
+  },
+
+  async enact(deploymentManager: DeploymentManager, { newFactoryAddress }) {
+    const trace = deploymentManager.tracer();
+    const ethers = deploymentManager.hre.ethers;
+
+    const {
+      governor,
+      comet,
+      configurator,
+      cometAdmin,
+      COMP,
+    } = await deploymentManager.getContracts();
+
+    const actions = [
+      // 1. Set comet factory to newly deployed factory
+      {
+        contract: configurator,
+        signature: 'setFactory(address,address)',
+        args: [comet.address, newFactoryAddress],
+      },
+
+      // 2. Deploy and upgrade to a new version of Comet
+      {
+        contract: cometAdmin,
+        signature: 'deployAndUpgradeTo(address,address)',
+        args: [configurator.address, comet.address],
+      },
+    ];
+    const description = "# Liquidation Event Handling And Collateral Reserves\n";
+    const txn = await deploymentManager.retry(
+      async () => governor.propose(...await proposal(actions, description))
+    );
+    trace(txn);
+
+    const event = (await txn.wait()).events.find(event => event.event === 'ProposalCreated');
+    const [proposalId] = event.args;
+    trace(`Created proposal ${proposalId}.`);
+  },
+
+  async enacted(deploymentManager: DeploymentManager): Promise<boolean> {
+    return true;
+  },
+
+  async verify(deploymentManager: DeploymentManager) {
+    // 1. & 2.
+    //  added a scenario to check for new Transfer event
+  }
+});
diff --git a/deployments/hardhat/dai/deploy.ts b/deployments/hardhat/dai/deploy.ts
@@ -44,7 +44,7 @@ export default async function deploy(deploymentManager: DeploymentManager, deplo
     priceFeed: goldPriceFeed.address,
     decimals: (8).toString(),
     borrowCollateralFactor: (0.9e18).toString(),
-    liquidateCollateralFactor: (1e18).toString(),
+    liquidateCollateralFactor: (0.91e18).toString(),
     liquidationFactor: (0.95e18).toString(),
     supplyCap: (1000000e8).toString(),
   };

diff --git a/deployments/mainnet/usdc/migrations/1665028496_absorb_transfer_event_and_auto_collateral.ts b/deployments/mainnet/usdc/migrations/1665028496_absorb_transfer_event_and_auto_collateral.ts
@@ -0,0 +1,75 @@
+import { DeploymentManager, migration } from '../../../../plugins/deployment_manager';
+import { calldata, exp, proposal } from '../../../../src/deploy';
+
+import { expect } from 'chai';
+
+const andrew = '0x2Ae8c972fB2E6c00ddED8986E2dc672ED190DA06';
+
+export default migration('1665028496_absorb_transfer_event_and_auto_collateral', {
+  async prepare(deploymentManager: DeploymentManager) {
+    const cometFactory = await deploymentManager.deploy('cometFactory', 'CometFactory.sol', [], true);
+    return { newFactoryAddress: cometFactory.address };
+  },
+
+  async enact(deploymentManager: DeploymentManager, { newFactoryAddress }) {
+    const trace = deploymentManager.tracer();
+    const ethers = deploymentManager.hre.ethers;
+
+    const {
+      governor,
+      comptrollerV2,
+      comet,
+      configurator,
+      cometAdmin,
+      COMP,
+    } = await deploymentManager.getContracts();
+
+    const actions = [
+      // 1. Set comet factory to newly deployed factory
+      {
+        contract: configurator,
+        signature: 'setFactory(address,address)',
+        args: [comet.address, newFactoryAddress],
+      },
+
+      // 2. Deploy and upgrade to a new version of Comet
+      {
+        contract: cometAdmin,
+        signature: 'deployAndUpgradeTo(address,address)',
+        args: [configurator.address, comet.address],
+      },
+
+      // 3. Transfer COMP to Andrew
+      {
+        contract: comptrollerV2,
+        signature: '_grantComp(address,uint256)',
+        args: [andrew, exp(10, 18)],
+      },
+    ];
+    const description = "# Liquidation Event Handling And Collateral Reserves\n\n## Explanation\n\n### Emit `Transfer` event from `absorb`\nWhen a user is liquidated in Compound III, all of their collateral is absorbed into the protocol, and they are typically left with a positive balance of the base asset (USDC) and no debt (or collateral dust).\n\nShortly after the first market launched, [a community developer noticed](https://twitter.com/andrewhong5297/status/1568994083380535296) that the `absorb` function was missing an event log in the case when an account is left with a positive balance.\n\nWhile this doesn\u2019t have any economic impact, adding this event log will improve the user experience on Etherscan and blockchain explorers, and make analytics easier.\n\n### Implicit collateral reserves\n\nWithout this patch, the collateral which is bought using `buyCollateral` must be part of the protocol's balance explicitly, which can happen during `absorb`. Excess collateral simply transferred to the protocol will not be available as collateral reserves to be sold by the protocol automatically.\n\nWith this patch, all of the excess collateral asset available using the ERC20 `balanceOf` function is implicitly considered part of collateral reserves. This means that accidentally transferring the ERC20 to the protocol will automatically become reserves. It also means that interest accrued implicitly, e.g. when the collateral is the token of another Compound III market, will automatically become part of reserves, which can be sold by the protocol and bought using `buyCollateral`.\n\nThis patch also formalizes the idea of collateral reserves in general, adding a `getCollateralReserves(asset)` function.\n\nThe associated forum post for this proposal can be found [here](https://www.comp.xyz/t/liquidation-event-handling/3684).\n\n## Proposal\n\nThe proposal itself is to be made from [this pull request](https://github.com/compound-finance/comet/pull/599). \n\nThe first step is to deploy a new CometFactory, using the patched version of the contract, which adds the Transfer event to `absorb` and modifies the total collateral accounting. This is done as a \u2018prepare\u2019 step of the migration script.\n\nThe first action of the proposal is to set the factory for cUSDCv3 to the newly deployed factory.\n\nThe second action is to deploy and upgrade to a new implementation of Comet, using the newly configured factory.\n\nThe third action is to transfer 10 COMP to ilemi.eth (0x2Ae8c972fB2E6c00ddED8986E2dc672ED190DA06), as a reward for identifying the issue and suggesting the `Transfer` event improvement.\n"
+    const txn = await deploymentManager.retry(
+      async () => governor.propose(...await proposal(actions, description))
+    );
+    trace(txn);
+
+    const event = (await txn.wait()).events.find(event => event.event === 'ProposalCreated');
+    const [proposalId] = event.args;
+    trace(`Created proposal ${proposalId}.`);
+  },
+
+  async enacted(deploymentManager: DeploymentManager): Promise<boolean> {
+    return true;
+  },
+
+  async verify(deploymentManager: DeploymentManager) {
+    const {
+      COMP,
+    } = await deploymentManager.getContracts();
+
+    // 1. & 2.
+    //  added a scenario to check for new Transfer event
+
+    // 3.
+    expect(await COMP.balanceOf(andrew)).to.be.equal(exp(10, 18));
+  }
+});
diff --git a/hardhat.config.ts b/hardhat.config.ts
@@ -43,17 +43,21 @@ function *deriveAccounts(pk: string, n: number = 10) {
     yield (BigInt('0x' + pk) + BigInt(i)).toString(16);
 }
 
-export function throwIfMissing(envVariable, msg: string) {
-  if (!envVariable) {
-    throw new Error(msg);
+export function requireEnv(varName, msg?: string): string {
+  const varVal = process.env[varName];
+  if (!varVal) {
+    throw new Error(msg ?? `Missing required environment variable '${varName}'`);
   }
+  return varVal;
 }
 
 // required environment variables
-throwIfMissing(ETHERSCAN_KEY, 'Missing required environment variable: ETHERSCAN_KEY');
-throwIfMissing(SNOWTRACE_KEY, 'Missing required environment variable: SNOWTRACE_KEY');
-throwIfMissing(INFURA_KEY, 'Missing required environment variable: INFURA_KEY');
-throwIfMissing(POLYGONSCAN_KEY, 'Missing required environment variable: POLYGONSCAN_KEY');
+[
+  'ETHERSCAN_KEY',
+  'SNOWTRACE_KEY',
+  'INFURA_KEY',
+  'POLYGONSCAN_KEY'
+].map(v => requireEnv(v))
 
 // Networks
 interface NetworkConfig {
@@ -189,7 +193,7 @@ const config: HardhatUserConfig = {
         name: 'mainnet',
         network: 'mainnet',
         deployment: 'usdc',
-        allocation: 0.1, // eth
+        allocation: 1.0, // eth
       },
       {
         name: 'development',

diff --git a/scenario/LiquidationScenario.ts b/scenario/LiquidationScenario.ts
@@ -161,6 +161,37 @@ scenario(
   }
 );
 
+scenario(
+  'Comet#liquidation > user can end up with a minted supply',
+  {
+    tokenBalances: {
+      $comet: { $base: 1000 },
+    },
+    cometBalances: {
+      albert: {
+        $base: -1000,
+        $asset0: 0.001
+      },
+    },
+  },
+  async ({ comet, actors }, context, world) => {
+    const { albert, betty } = actors;
+
+    await world.increaseTime(
+      await timeUntilUnderwater({
+        comet,
+        actor: albert,
+      })
+    );
+
+    const ab0 = await betty.absorb({ absorber: betty.address, accounts: [albert.address] });
+    expect(ab0.events?.[2]?.event).to.be.equal('Transfer');
+
+    const baseBalance = await albert.getCometBaseBalance();
+    expect(Number(baseBalance)).to.be.greaterThan(0);
+  }
+);
+
 // XXX Skipping temporarily because testnet is in a weird state where an EOA ('admin') still
 // has permission to withdraw Comet's collateral, while Timelock does not. This is because the
 // permission was set up in the initialize() function. There is currently no way to update this

diff --git a/scenario/context/CometContext.ts b/scenario/context/CometContext.ts
@@ -67,7 +67,7 @@ export class CometContext {
   }
 
   async getProposer(): Promise<SignerWithAddress> {
-    return this.world.impersonateAddress((await this.getCompWhales())[0]);
+    return this.world.impersonateAddress((await this.getCompWhales())[0], 10n ** 18n);
   }
 
   async getComet(): Promise<CometInterface> {

diff --git a/scripts/liquidation_bot/deploy.ts b/scripts/liquidation_bot/deploy.ts
@@ -1,18 +1,16 @@
 import hre from 'hardhat';
 import { DeploymentManager } from '../../plugins/deployment_manager/DeploymentManager';
 import { CometInterface } from '../../build/types';
-import { throwIfMissing } from '../../hardhat.config';
+import { requireEnv } from '../../hardhat.config';
 
 // https://docs.uniswap.org/protocol/reference/deployments
 const UNISWAP_V3_FACTORY_ADDRESS = '0x1F98431c8aD98523631AE4a59f267346ea31F984';
 const WETH9 = '0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2';
 const SWAP_ROUTER = '0xe592427a0aece92de3edee1f18e0157c05861564';
 
 async function main() {
-  const { DEPLOYMENT, RECIPIENT } = process.env;
-  throwIfMissing(DEPLOYMENT, 'missing required env variable: DEPLOYMENT');
-  throwIfMissing(RECIPIENT, 'missing required env variable: RECIPIENT');
-
+  const DEPLOYMENT = requireEnv('DEPLOYMENT');
+  const RECIPIENT = requireEnv('RECIPIENT');
   const network = hre.network.name;
   if (['hardhat', 'fuji'].includes(network)) {
     throw new Error(`Uniswap unavailable on network: ${network}`);