Merge pull request #132 from CreamFi/handle_erc777

contracts/, tests/: handle erc777
CreamFi · Sep 17, 2021 · b709d39 · b709d39
2 parents c8e33a8 + da25143
commit b709d39
Show file tree

Hide file tree

Showing 7 changed files with 181 additions and 26 deletions.
diff --git a/contracts/CCollateralCapErc20.sol b/contracts/CCollateralCapErc20.sol
@@ -264,6 +264,10 @@ contract CCollateralCapErc20 is CToken, CCollateralCapErc20Interface {
         initializeAccountCollateralTokens(account);
 
         require(msg.sender == address(comptroller), "only comptroller may unregister collateral for user");
+        require(
+            comptroller.redeemAllowed(address(this), account, accountCollateralTokens[account]) == 0,
+            "comptroller rejection"
+        );
 
         decreaseUserCollateralInternal(account, accountCollateralTokens[account]);
     }
@@ -528,8 +532,6 @@ contract CCollateralCapErc20 is CToken, CCollateralCapErc20Interface {
      * @param amount The amount of collateral user wants to decrease
      */
     function decreaseUserCollateralInternal(address account, uint256 amount) internal {
-        require(comptroller.redeemAllowed(address(this), account, amount) == 0, "comptroller rejection");
-
         /*
          * Return if amount is zero.
          * Put behind `redeemAllowed` for accuring potential COMP rewards.
@@ -696,6 +698,10 @@ contract CCollateralCapErc20 is CToken, CCollateralCapErc20Interface {
             collateralTokens = vars.redeemTokens - bufferTokens;
         }
 
+        if (collateralTokens > 0) {
+            require(comptroller.redeemAllowed(address(this), redeemer, collateralTokens) == 0, "comptroller rejection");
+        }
+
         /* Verify market's block number equals current block number */
         if (accrualBlockNumber != getBlockNumber()) {
             return fail(Error.MARKET_NOT_FRESH, FailureInfo.REDEEM_FRESHNESS_CHECK);
@@ -710,14 +716,6 @@ contract CCollateralCapErc20 is CToken, CCollateralCapErc20Interface {
         // EFFECTS & INTERACTIONS
         // (No safe failures beyond this point)
 
-        /*
-         * We invoke doTransferOut for the redeemer and the redeemAmount.
-         *  Note: The cToken must handle variations between ERC-20 and ETH underlying.
-         *  On success, the cToken has redeemAmount less of cash.
-         *  doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred.
-         */
-        doTransferOut(redeemer, vars.redeemAmount, isNative);
-
         /*
          * We calculate the new total supply and redeemer balance, checking for underflow:
          *  totalSupplyNew = totalSupply - redeemTokens
@@ -729,9 +727,15 @@ contract CCollateralCapErc20 is CToken, CCollateralCapErc20Interface {
         /*
          * We only deallocate collateral tokens if the redeemer needs to redeem them.
          */
-        if (collateralTokens > 0) {
-            decreaseUserCollateralInternal(redeemer, collateralTokens);
-        }
+        decreaseUserCollateralInternal(redeemer, collateralTokens);
+
+        /*
+         * We invoke doTransferOut for the redeemer and the redeemAmount.
+         *  Note: The cToken must handle variations between ERC-20 and ETH underlying.
+         *  On success, the cToken has redeemAmount less of cash.
+         *  doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred.
+         */
+        doTransferOut(redeemer, vars.redeemAmount, isNative);
 
         /* We emit a Transfer event, and a Redeem event */
         emit Transfer(redeemer, address(this), vars.redeemTokens);

diff --git a/contracts/CErc20.sol b/contracts/CErc20.sol
@@ -443,6 +443,10 @@ contract CErc20 is CToken, CErc20Interface {
         // EFFECTS & INTERACTIONS
         // (No safe failures beyond this point)
 
+        /* We write previously calculated values into storage */
+        totalSupply = vars.totalSupplyNew;
+        accountTokens[redeemer] = vars.accountTokensNew;
+
         /*
          * We invoke doTransferOut for the redeemer and the redeemAmount.
          *  Note: The cToken must handle variations between ERC-20 and ETH underlying.
@@ -451,10 +455,6 @@ contract CErc20 is CToken, CErc20Interface {
          */
         doTransferOut(redeemer, vars.redeemAmount, isNative);
 
-        /* We write previously calculated values into storage */
-        totalSupply = vars.totalSupplyNew;
-        accountTokens[redeemer] = vars.accountTokensNew;
-
         /* We emit a Transfer event, and a Redeem event */
         emit Transfer(redeemer, address(this), vars.redeemTokens);
         emit Redeem(redeemer, vars.redeemAmount, vars.redeemTokens);

diff --git a/contracts/CToken.sol b/contracts/CToken.sol
@@ -503,6 +503,11 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter {
         // EFFECTS & INTERACTIONS
         // (No safe failures beyond this point)
 
+        /* We write the previously calculated values into storage */
+        accountBorrows[borrower].principal = vars.accountBorrowsNew;
+        accountBorrows[borrower].interestIndex = borrowIndex;
+        totalBorrows = vars.totalBorrowsNew;
+
         /*
          * We invoke doTransferOut for the borrower and the borrowAmount.
          *  Note: The cToken must handle variations between ERC-20 and ETH underlying.
@@ -511,11 +516,6 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter {
          */
         doTransferOut(borrower, borrowAmount, isNative);
 
-        /* We write the previously calculated values into storage */
-        accountBorrows[borrower].principal = vars.accountBorrowsNew;
-        accountBorrows[borrower].interestIndex = borrowIndex;
-        totalBorrows = vars.totalBorrowsNew;
-
         /* We emit a Borrow event */
         emit Borrow(borrower, borrowAmount, vars.accountBorrowsNew, vars.totalBorrowsNew);
 

diff --git a/contracts/CWrappedNative.sol b/contracts/CWrappedNative.sol
@@ -666,6 +666,10 @@ contract CWrappedNative is CToken, CWrappedNativeInterface {
         // EFFECTS & INTERACTIONS
         // (No safe failures beyond this point)
 
+        /* We write previously calculated values into storage */
+        totalSupply = vars.totalSupplyNew;
+        accountTokens[redeemer] = vars.accountTokensNew;
+
         /*
          * We invoke doTransferOut for the redeemer and the redeemAmount.
          *  Note: The cToken must handle variations between ERC-20 and ETH underlying.
@@ -674,10 +678,6 @@ contract CWrappedNative is CToken, CWrappedNativeInterface {
          */
         doTransferOut(redeemer, vars.redeemAmount, isNative);
 
-        /* We write previously calculated values into storage */
-        totalSupply = vars.totalSupplyNew;
-        accountTokens[redeemer] = vars.accountTokensNew;
-
         /* We emit a Transfer event, and a Redeem event */
         emit Transfer(redeemer, address(this), vars.redeemTokens);
         emit Redeem(redeemer, vars.redeemAmount, vars.redeemTokens);

diff --git a/tests/Contracts/EvilToken.sol b/tests/Contracts/EvilToken.sol
@@ -1,6 +1,8 @@
 pragma solidity ^0.5.16;
 
 import "./FaucetToken.sol";
+import "../../contracts/CEther.sol";
+import "../../contracts/CCollateralCapErc20.sol";
 
 /**
  * @title The Compound Evil Test Token
@@ -48,3 +50,77 @@ contract EvilToken is FaucetToken {
         return true;
     }
 }
+
+interface RecipientInterface {
+    /**
+     * @dev Hook executed upon a transfer to the recipient
+     */
+    function tokensReceived() external;
+}
+
+contract EvilAccount is RecipientInterface {
+    address payable private crEth;
+    address private crEvilToken;
+    uint256 private borrowAmount;
+
+    constructor(
+        address payable _crEth,
+        address _crEvilToken,
+        uint256 _borrowAmount
+    ) public {
+        crEth = _crEth;
+        crEvilToken = _crEvilToken;
+        borrowAmount = _borrowAmount;
+    }
+
+    function attack() external payable {
+        // Mint crEth.
+        CEther(crEth).mint.value(msg.value)();
+        ComptrollerInterface comptroller = CEther(crEth).comptroller();
+
+        // Enter the market.
+        address[] memory markets = new address[](1);
+        markets[0] = crEth;
+        comptroller.enterMarkets(markets);
+
+        // Borrow EvilTransferToken.
+        require(CCollateralCapErc20(crEvilToken).borrow(borrowAmount) == 0, "first borrow failed");
+    }
+
+    function tokensReceived() external {
+        // Borrow Eth.
+        require(CEther(crEth).borrow(borrowAmount) != 0, "reentry borrow succeed");
+    }
+
+    function() external payable {}
+}
+
+contract EvilTransferToken is FaucetToken {
+    constructor(
+        uint256 _initialAmount,
+        string memory _tokenName,
+        uint8 _decimalUnits,
+        string memory _tokenSymbol
+    ) public FaucetToken(_initialAmount, _tokenName, _decimalUnits, _tokenSymbol) {}
+
+    function transfer(address dst, uint256 amount) external returns (bool) {
+        balanceOf[msg.sender] = balanceOf[msg.sender].sub(amount);
+        balanceOf[dst] = balanceOf[dst].add(amount);
+        emit Transfer(msg.sender, dst, amount);
+
+        RecipientInterface(dst).tokensReceived();
+        return true;
+    }
+
+    function transferFrom(
+        address src,
+        address dst,
+        uint256 amount
+    ) external returns (bool) {
+        balanceOf[src] = balanceOf[src].sub(amount);
+        balanceOf[dst] = balanceOf[dst].add(amount);
+        allowance[src][msg.sender] = allowance[src][msg.sender].sub(amount);
+        emit Transfer(src, dst, amount);
+        return true;
+    }
+}
diff --git a/tests/Tokens/attack.js b/tests/Tokens/attack.js
@@ -0,0 +1,41 @@
+const {
+  etherMantissa
+} = require('../Utils/Ethereum');
+
+const {
+  makeCToken,
+  makeComptroller,
+  makeEvilAccount
+} = require('../Utils/Compound');
+
+const collateralFactor = 0.5, underlyingPrice = 1, mintAmount = 2, borrowAmount = 1;
+
+describe('Attack', function () {
+  let root, accounts;
+  let comptroller;
+  let cEth, cEvil;
+  let evilAccount;
+
+  beforeEach(async () => {
+    [root, ...accounts] = saddle.accounts;
+    comptroller = await makeComptroller();
+    cEth = await makeCToken({comptroller, kind: 'cether', supportMarket: true, collateralFactor});
+    cEvil = await makeCToken({comptroller, kind: 'cevil', supportMarket: true, collateralFactor, underlyingPrice});
+    evilAccount = await makeEvilAccount({crEth: cEth, crEvil: cEvil, borrowAmount: etherMantissa(borrowAmount)});
+  });
+
+  it('reentry borrow attack', async () => {
+    await send(cEvil.underlying, 'allocateTo', [accounts[0], etherMantissa(100)]);
+    await send(cEvil.underlying, 'approve', [cEvil._address, etherMantissa(100)], {from: accounts[0]});
+    await send(cEvil, 'mint', [etherMantissa(100)], {from: accounts[0]});
+
+    // Actually, this attack will emit a Failure event with value (3: COMPTROLLER_REJECTION, 6: BORROW_COMPTROLLER_REJECTION, 4: INSUFFICIENT_LIQUIDITY).
+    // However, somehow it failed to parse the event.
+    await send(evilAccount, 'attack', [], {value: etherMantissa(mintAmount)});
+
+    // The attack should have no effect.
+    ({1: liquidity, 2: shortfall} = await call(comptroller, 'getAccountLiquidity', [evilAccount._address]));
+    expect(liquidity).toEqualNumber(0);
+    expect(shortfall).toEqualNumber(0);
+  });
+});
diff --git a/tests/Utils/Compound.js b/tests/Utils/Compound.js
@@ -230,6 +230,26 @@ async function makeCToken(opts = {}) {
       version = 2; // cwrappednative's version is 2
       break;
 
+    case 'cevil':
+      underlying = await makeToken({kind: "evil"});
+      cDelegatee = await deploy('CErc20DelegateHarness');
+      cDelegator = await deploy('CErc20Delegator',
+        [
+          underlying._address,
+          comptroller._address,
+          interestRateModel._address,
+          exchangeRate,
+          name,
+          symbol,
+          decimals,
+          admin,
+          cDelegatee._address,
+          "0x0"
+        ]
+      );
+      cToken = await saddle.getContractAt('CErc20DelegateHarness', cDelegator._address); // XXXS at
+      break;
+
     case 'cerc20':
     default:
       underlying = opts.underlying || await makeToken(opts.underlyingOpts);
@@ -385,6 +405,12 @@ async function makeToken(opts = {}) {
     const symbol = opts.symbol || 'UNI-V2-LP';
     const name = opts.name || `Uniswap v2 LP`;
     return await deploy('LPTokenHarness', [quantity, name, decimals, symbol]);
+  } else if (kind == 'evil') {
+    const quantity = etherUnsigned(dfn(opts.quantity, 1e25));
+    const decimals = etherUnsigned(dfn(opts.decimals, 18));
+    const symbol = opts.symbol || 'Evil';
+    const name = opts.name || `Evil Token`;
+    return await deploy('EvilTransferToken', [quantity, name, decimals, symbol]);
   }
 }
 
@@ -408,6 +434,13 @@ async function makeLiquidityMining(opts = {}) {
   return await deploy('MockLiquidityMining', [comptroller._address]);
 }
 
+async function makeEvilAccount(opts = {}) {
+  const crEth = opts.crEth || await makeCToken({kind: 'cether'});
+  const crEvil = opts.crEvil || await makeCToken({kind: 'cevil'});
+  const borrowAmount = opts.borrowAmount || etherMantissa(1);
+  return await deploy('EvilAccount', [crEth._address, crEvil._address, borrowAmount]);
+}
+
 async function preCSLP(underlying) {
   const sushiToken = await deploy('SushiToken');
   const masterChef = await deploy('MasterChef', [sushiToken._address]);
@@ -624,6 +657,7 @@ module.exports = {
   makeToken,
   makeCurveSwap,
   makeLiquidityMining,
+  makeEvilAccount,
   makeCTokenAdmin,
 
   balanceOf,