Deploy and proposal for USDT market on Goerli

contracts/Comet.sol

@@ -4,13 +4,14 @@ 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 {
+contract Comet is CometMainInterface, ReentrancyGuard {
     /** General configuration constants **/
 
     /// @notice The admin of the protocol
@@ -841,7 +842,7 @@ contract Comet is CometMainInterface {
      * @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 {
+    function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal nonReentrant {
         if (isSupplyPaused()) revert Paused();
         if (!hasPermission(from, operator)) revert Unauthorized();
 
@@ -952,7 +953,7 @@ contract Comet is CometMainInterface {
      * @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 {
+    function transferInternal(address operator, address src, address dst, address asset, uint amount) internal nonReentrant {
         if (isTransferPaused()) revert Paused();
         if (!hasPermission(src, operator)) revert Unauthorized();
         if (src == dst) revert NoSelfTransfer();
@@ -1063,7 +1064,7 @@ contract Comet is CometMainInterface {
      * @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 {
+    function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal nonReentrant {
         if (isWithdrawPaused()) revert Paused();
         if (!hasPermission(src, operator)) revert Unauthorized();
 
@@ -1224,7 +1225,7 @@ contract Comet is CometMainInterface {
      * @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 {
+    function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external nonReentrant {
         if (isBuyPaused()) revert Paused();
 
         int reserves = getReserves();
@@ -1270,7 +1271,7 @@ contract Comet is CometMainInterface {
      * @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 {
+    function withdrawReserves(address to, uint amount) override external nonReentrant {
         if (msg.sender != governor) revert Unauthorized();
 
         int reserves = getReserves();

contracts/ReentrancyGuard.sol

@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: MIT
+// OpenZeppelin Contracts (last updated v4.9.0) (security/ReentrancyGuard.sol)
+
+pragma solidity ^0.8.0;
+
+/**
+ * @dev Contract module that helps prevent reentrant calls to a function.
+ *
+ * Inheriting from `ReentrancyGuard` will make the {nonReentrant} modifier
+ * available, which can be applied to functions to make sure there are no nested
+ * (reentrant) calls to them.
+ *
+ * Note that because there is a single `nonReentrant` guard, functions marked as
+ * `nonReentrant` may not call one another. This can be worked around by making
+ * those functions `private`, and then adding `external` `nonReentrant` entry
+ * points to them.
+ *
+ * TIP: If you would like to learn more about reentrancy and alternative ways
+ * to protect against it, check out our blog post
+ * https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul].
+ */
+abstract contract ReentrancyGuard {
+    // Booleans are more expensive than uint256 or any type that takes up a full
+    // word because each write operation emits an extra SLOAD to first read the
+    // slot's contents, replace the bits taken up by the boolean, and then write
+    // back. This is the compiler's defense against contract upgrades and
+    // pointer aliasing, and it cannot be disabled.
+
+    // The values being non-zero value makes deployment a bit more expensive,
+    // but in exchange the refund on every call to nonReentrant will be lower in
+    // amount. Since refunds are capped to a percentage of the total
+    // transaction's gas, it is best to keep them low in cases like this one, to
+    // increase the likelihood of the full refund coming into effect.
+    uint256 private constant _NOT_ENTERED = 1;
+    uint256 private constant _ENTERED = 2;
+
+    uint256 private _status;
+
+    constructor() {
+        _status = _NOT_ENTERED;
+    }
+
+    /**
+     * @dev Prevents a contract from calling itself, directly or indirectly.
+     * Calling a `nonReentrant` function from another `nonReentrant`
+     * function is not supported. It is possible to prevent this from happening
+     * by making the `nonReentrant` function external, and making it call a
+     * `private` function that does the actual work.
+     */
+    modifier nonReentrant() {
+        _nonReentrantBefore();
+        _;
+        _nonReentrantAfter();
+    }
+
+    function _nonReentrantBefore() private {
+        // On the first call to nonReentrant, _status will be _NOT_ENTERED
+        require(_status != _ENTERED, "ReentrancyGuard: reentrant call");
+
+        // Any calls to nonReentrant after this point will fail
+        _status = _ENTERED;
+    }
+
+    function _nonReentrantAfter() private {
+        // By storing the original value once again, a refund is triggered (see
+        // https://eips.ethereum.org/EIPS/eip-2200)
+        _status = _NOT_ENTERED;
+    }
+
+    /**
+     * @dev Returns true if the reentrancy guard is currently set to "entered", which indicates there is a
+     * `nonReentrant` function in the call stack.
+     */
+    function _reentrancyGuardEntered() internal view returns (bool) {
+        return _status == _ENTERED;
+    }
+}

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 is using 3000 EVIL tokens, so totalSupplyBase should have 3000e6
-      expect(evilTotalsBasic.totalSupplyBase).to.equal(3000e6);
+      // EvilToken attack will be blocked by re-entrancy guard, so totalSupplyBase should be 0e6 (under Bob's name)
+      expect(evilTotalsBasic.totalSupplyBase).to.equal(0e6);
       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 is using 3000 EVIL tokens, so totalSupplyBase should be 3000e6 (under Bob's name)
-      expect(evilBobPortfolio.internal.EVIL).to.equal(3000e6);
+      // 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);
     });
   });
 });

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(128000);
+    expect(Number(s0.receipt.gasUsed)).to.be.lessThan(131000);
   });
 
   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(163000);
+    expect(Number(s0.receipt.gasUsed)).to.be.lessThan(166000);
   });
 
   it('prevents exceeding the supply cap via re-entrancy', async () => {
@@ -558,47 +558,7 @@ describe('supplyTo', function () {
     await EVIL.allocateTo(alice.address, 75e6);
     await expect(
       comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6)
-    ).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);
+    ).to.be.revertedWith("ReentrancyGuard: reentrant call");
   });
 });
 

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("custom error 'NotCollateralized()'");
+      ).to.be.revertedWith("ReentrancyGuard: reentrant call");
 
       // 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("custom error 'NotCollateralized()'");
+      ).to.be.revertedWith("ReentrancyGuard: reentrant call");
 
       // no USDC transferred
       expect(await USDC.balanceOf(comet.address)).to.eq(100e6);