Revert "add and leveraged oz's re-entrancy guard"

This reverts commit 49588d6.

contracts/Comet.sol

@@ -4,14 +4,13 @@ pragma solidity 0.8.15;
 import "./CometMainInterface.sol";
 import "./IERC20NonStandard.sol";
 import "./IPriceFeed.sol";
-import "./ReentrancyGuard.sol";
 
 /**
  * @title Compound's Comet Contract
  * @notice An efficient monolithic money market protocol
  * @author Compound
  */
-contract Comet is CometMainInterface, ReentrancyGuard {
+contract Comet is CometMainInterface {
     /** General configuration constants **/
 
     /// @notice The admin of the protocol
@@ -842,7 +841,7 @@ contract Comet is CometMainInterface, ReentrancyGuard {
      * @dev Supply either collateral or base asset, depending on the asset, if operator is allowed
      * @dev Note: Specifying an `amount` of uint256.max will repay all of `dst`'s accrued base borrow balance
      */
-    function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal nonReentrant {
+    function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal {
         if (isSupplyPaused()) revert Paused();
         if (!hasPermission(from, operator)) revert Unauthorized();
 
@@ -953,7 +952,7 @@ contract Comet is CometMainInterface, ReentrancyGuard {
      * @dev Transfer either collateral or base asset, depending on the asset, if operator is allowed
      * @dev Note: Specifying an `amount` of uint256.max will transfer all of `src`'s accrued base balance
      */
-    function transferInternal(address operator, address src, address dst, address asset, uint amount) internal nonReentrant {
+    function transferInternal(address operator, address src, address dst, address asset, uint amount) internal {
         if (isTransferPaused()) revert Paused();
         if (!hasPermission(src, operator)) revert Unauthorized();
         if (src == dst) revert NoSelfTransfer();
@@ -1064,7 +1063,7 @@ contract Comet is CometMainInterface, ReentrancyGuard {
      * @dev Withdraw either collateral or base asset, depending on the asset, if operator is allowed
      * @dev Note: Specifying an `amount` of uint256.max will withdraw all of `src`'s accrued base balance
      */
-    function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal nonReentrant {
+    function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal {
         if (isWithdrawPaused()) revert Paused();
         if (!hasPermission(src, operator)) revert Unauthorized();
 
@@ -1225,7 +1224,7 @@ contract Comet is CometMainInterface, ReentrancyGuard {
      * @param baseAmount The amount of base tokens used to buy the collateral
      * @param recipient The recipient address
      */
-    function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external nonReentrant {
+    function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external {
         if (isBuyPaused()) revert Paused();
 
         int reserves = getReserves();
@@ -1271,7 +1270,7 @@ contract Comet is CometMainInterface, ReentrancyGuard {
      * @param to An address of the receiver of withdrawn reserves
      * @param amount The amount of reserves to be withdrawn from the protocol
      */
-    function withdrawReserves(address to, uint amount) override external nonReentrant {
+    function withdrawReserves(address to, uint amount) override external {
         if (msg.sender != governor) revert Unauthorized();
 
         int reserves = getReserves();

test/buy-collateral-test.ts

@@ -543,8 +543,8 @@ describe('buyCollateral', function () {
       expect(normalTotalsBasic.trackingSupplyIndex).to.equal(evilTotalsBasic.trackingSupplyIndex);
       expect(normalTotalsBasic.trackingBorrowIndex).to.equal(evilTotalsBasic.trackingBorrowIndex);
       expect(normalTotalsBasic.totalSupplyBase).to.equal(1e6);
-      // EvilToken attack will be blocked by re-entrancy guard, so totalSupplyBase should be 0e6 (under Bob's name)
-      expect(evilTotalsBasic.totalSupplyBase).to.equal(0e6);
+      // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should have 3000e6
+      expect(evilTotalsBasic.totalSupplyBase).to.equal(3000e6);
       expect(normalTotalsBasic.totalBorrowBase).to.equal(evilTotalsBasic.totalBorrowBase);
 
       expect(normalTotalsCollateral.totalSupplyAsset).to.eq(evilTotalsCollateral.totalSupplyAsset);
@@ -559,8 +559,8 @@ describe('buyCollateral', function () {
       const evilBobPortfolio = await portfolio(evilProtocol, evilBob.address);
 
       expect(normalBobPortfolio.internal.USDC).to.equal(1e6);
-      // EvilToken attack will be blocked by re-entrancy guard, so totalSupplyBase should be 0e6 (under Bob's name)
-      expect(evilBobPortfolio.internal.EVIL).to.equal(0e6);
+      // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should be 3000e6 (under Bob's name)
+      expect(evilBobPortfolio.internal.EVIL).to.equal(3000e6);
     });
   });
 });

test/supply-test.ts

@@ -466,7 +466,7 @@ describe('supplyTo', function () {
     expect(t1.totalSupplyBase).to.be.equal(t0.totalSupplyBase.add(999e6));
     expect(t1.totalBorrowBase).to.be.equal(t0.totalBorrowBase);
     // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation
-    expect(Number(s0.receipt.gasUsed)).to.be.lessThan(131000);
+    expect(Number(s0.receipt.gasUsed)).to.be.lessThan(128000);
   });
 
   it('supplies collateral the correct amount in a fee-like situation', async () => {
@@ -524,7 +524,7 @@ describe('supplyTo', function () {
     expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n });
     expect(t1.totalSupplyAsset).to.be.equal(t0.totalSupplyAsset.add(1998e8));
     // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation
-    expect(Number(s0.receipt.gasUsed)).to.be.lessThan(166000);
+    expect(Number(s0.receipt.gasUsed)).to.be.lessThan(163000);
   });
 
   it('prevents exceeding the supply cap via re-entrancy', async () => {
@@ -558,7 +558,47 @@ describe('supplyTo', function () {
     await EVIL.allocateTo(alice.address, 75e6);
     await expect(
       comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6)
-    ).to.be.revertedWith("ReentrancyGuard: reentrant call");
+    ).to.be.revertedWith("custom error 'SupplyCapExceeded()'");
+  });
+
+  // This test is purposely test the edge case where comet internal accounting will be incorrect when EvilToken re-entrancy attack
+  // is performed. The attack will cause comet to have inflated supply amount of token. 
+  //
+  // We decided that this won't be a concern for Comet, as in order to pull off this attack, the governance has to propose and vote to 
+  // add suspicious token in to Comet's market. As long as governance doesn't add suspicious token contract or erc-777 token to market, the 
+  // Comet should not be vulnerable to this type of attack.
+  it('incorrect accounting via re-entrancy', async () => {
+    const { comet, tokens, users: [alice, bob] } = await makeProtocol({
+      assets: {
+        USDC: {
+          decimals: 6
+        },
+        EVIL: {
+          decimals: 6,
+          initialPrice: 2,
+          factory: await ethers.getContractFactory('EvilToken') as EvilToken__factory,
+          supplyCap: 250e6
+        }
+      }
+    });
+    const { EVIL } = <{ EVIL: EvilToken }>tokens;
+
+    const attack = Object.assign({}, await EVIL.getAttack(), {
+      attackType: ReentryAttack.SupplyFrom,
+      source: alice.address,
+      destination: bob.address,
+      asset: EVIL.address,
+      amount: 75e6,
+      maxCalls: 1
+    });
+    await EVIL.setAttack(attack);
+
+    await comet.connect(alice).allow(EVIL.address, true);
+    await wait(EVIL.connect(alice).approve(comet.address, 75e6));
+    await EVIL.allocateTo(alice.address, 75e6);
+    await comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6);
+    // Re-entrancy attack 2 loops, expect to have 2X accounting balance 75*2 = 150
+    expect(await comet.collateralBalanceOf(bob.address, EVIL.address)).to.be.equal(150e6);
   });
 });
 

test/withdraw-test.ts

@@ -511,7 +511,7 @@ describe('withdraw', function () {
       // in callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6)
       await expect(
         comet.connect(alice).withdraw(EVIL.address, 1e6)
-      ).to.be.revertedWith("ReentrancyGuard: reentrant call");
+      ).to.be.revertedWith("custom error 'NotCollateralized()'");
 
       // no USDC transferred
       expect(await USDC.balanceOf(comet.address)).to.eq(100e6);
@@ -558,7 +558,7 @@ describe('withdraw', function () {
       // in callback, EvilToken attempts to withdraw USDC to bob's address
       await expect(
         comet.connect(alice).withdraw(EVIL.address, 1e6)
-      ).to.be.revertedWith("ReentrancyGuard: reentrant call");
+      ).to.be.revertedWith("custom error 'NotCollateralized()'");
 
       // no USDC transferred
       expect(await USDC.balanceOf(comet.address)).to.eq(100e6);