Fuse fixed accounting

.gitignore

@@ -39,3 +39,5 @@ junit.xml
 .build
 .last_confs
 .saddle_history
+artifacts
+cache

contracts/CEther.sol

@@ -135,8 +135,7 @@ contract CEther is CToken, CEtherInterface {
 
     function doTransferOut(address payable to, uint amount) internal {
         // Send the Ether and revert on failure
-        (bool success, ) = to.call.value(amount)("");
-        require(success, "doTransferOut failed");
+        to.transfer(amount);
     }
 
     function requireNoError(uint errCode, string memory message) internal pure {

contracts/CEtherDelegateTempExploitAccounting.sol

@@ -0,0 +1,162 @@
+pragma solidity ^0.5.17;
+
+import "./CEther.sol";
+
+/**
+ * @title Compound's CEtherDelegate Contract
+ * @notice CTokens which wrap Ether and are delegated to
+ * @author Compound
+ */
+contract CEtherDelegateTempExploitAccounting is CDelegateInterface, CEther {
+    /**
+     * @notice Construct an empty delegate
+     */
+    constructor() public {}
+
+    /**
+     * @notice Called by the delegator on a delegate to initialize it for duty
+     * @param data The encoded bytes data for any initialization
+     */
+    function _becomeImplementation(bytes calldata data) external {
+        // Shh -- currently unused
+        data;
+
+        // Shh -- we don't ever want this hook to be marked pure
+        if (false) {
+            implementation = address(0);
+        }
+
+        require(msg.sender == address(this) || hasAdminRights(), "!self");
+        require(accrueInterest() == uint(Error.NO_ERROR), "accrue interest failed");
+
+        // Get secondary accounts from data
+        (address[] memory secondaryAccounts) = abi.decode(data, (address[]));
+        uint256 secondaryAccountsBorrowBalance = 0;
+
+        for (uint256 i = 0; i < secondaryAccounts.length; i++) {
+            address secondaryAccount = secondaryAccounts[i];
+
+            // Get account #2 borrow balance
+            uint256 secondaryAccountBorrowBalance = div_(mul_(accountBorrows[secondaryAccount].principal, borrowIndex), accountBorrows[secondaryAccount].interestIndex);
+            secondaryAccountsBorrowBalance = add_(secondaryAccountsBorrowBalance, secondaryAccountBorrowBalance);
+
+            // Set account #2 borrow balance to 0
+            accountBorrows[secondaryAccount].principal = 0;
+        }
+
+        // Get account #1 supply balance
+        uint256 account1SupplyShares = accountTokens[0x32075bAd9050d4767018084F0Cb87b3182D36C45];
+        uint256 account1SupplyBalance = mul_ScalarTruncate(Exp({mantissa: exchangeRateStored()}), account1SupplyShares);
+
+        // Set account #1 supply balance to 0
+        accountTokens[0x32075bAd9050d4767018084F0Cb87b3182D36C45] = 0;
+
+        // Set account #1 borrow balance = secondary accounts' borrow balance - account #1 supply balance
+        require(secondaryAccountsBorrowBalance >= account1SupplyBalance, "Expected secondary accounts' combined borrow balance to be greater than or equal to account #1 supply balance.");
+        require(accountBorrows[0x32075bAd9050d4767018084F0Cb87b3182D36C45].principal == 0, "Expected account #1 borrow balance to start at 0.");
+        accountBorrows[0x32075bAd9050d4767018084F0Cb87b3182D36C45].principal = sub_(secondaryAccountsBorrowBalance, account1SupplyBalance);
+        accountBorrows[0x32075bAd9050d4767018084F0Cb87b3182D36C45].interestIndex = borrowIndex;
+
+        // Subtract from total supply
+        totalSupply = sub_(totalSupply, account1SupplyShares);
+
+        // Subtract from total borrows
+        totalBorrows = sub_(totalBorrows, account1SupplyBalance);
+    }
+
+    /**
+     * @notice Called by the delegator on a delegate to forfeit its responsibility
+     */
+    function _resignImplementation() internal {
+        // Shh -- we don't ever want this hook to be marked pure
+        if (false) {
+            implementation = address(0);
+        }
+    }
+
+    /**
+     * @dev Internal function to update the implementation of the delegator
+     * @param implementation_ The address of the new implementation for delegation
+     * @param allowResign Flag to indicate whether to call _resignImplementation on the old implementation
+     * @param becomeImplementationData The encoded bytes data to be passed to _becomeImplementation
+     */
+    function _setImplementationInternal(address implementation_, bool allowResign, bytes memory becomeImplementationData) internal {
+        // Check whitelist
+        require(fuseAdmin.cEtherDelegateWhitelist(implementation, implementation_, allowResign), "!impl");
+
+        // Call _resignImplementation internally (this delegate's code)
+        if (allowResign) _resignImplementation();
+
+        // Get old implementation
+        address oldImplementation = implementation;
+
+        // Store new implementation
+        implementation = implementation_;
+
+        // Call _becomeImplementation externally (delegating to new delegate's code)
+        _functionCall(address(this), abi.encodeWithSignature("_becomeImplementation(bytes)", becomeImplementationData), "!become");
+
+        // Emit event
+        emit NewImplementation(oldImplementation, implementation);
+    }
+
+    /**
+     * @notice Called by the admin to update the implementation of the delegator
+     * @param implementation_ The address of the new implementation for delegation
+     * @param allowResign Flag to indicate whether to call _resignImplementation on the old implementation
+     * @param becomeImplementationData The encoded bytes data to be passed to _becomeImplementation
+     */
+    function _setImplementationSafe(address implementation_, bool allowResign, bytes calldata becomeImplementationData) external {
+        // Check admin rights
+        require(hasAdminRights(), "!admin");
+
+        // Set implementation
+        _setImplementationInternal(implementation_, allowResign, becomeImplementationData);
+    }
+
+    /**
+     * @dev Performs a Solidity function call using a low level `call`. A
+     * plain `call` is an unsafe replacement for a function call: use this
+     * function instead.
+     * If `target` reverts with a revert reason, it is bubbled up by this
+     * function (like regular Solidity function calls).
+     * Returns the raw returned data. To convert to the expected return value,
+     * use https://solidity.readthedocs.io/en/latest/units-and-global-variables.html?highlight=abi.decode#abi-encoding-and-decoding-functions[`abi.decode`].
+     * Copied from `CErc20.sol`.
+     * @param data The call data (encoded using abi.encode or one of its variants).
+     * @param errorMessage The revert string to return on failure.
+     */
+    function _functionCall(address target, bytes memory data, string memory errorMessage) internal returns (bytes memory) {
+        (bool success, bytes memory returndata) = target.call(data);
+
+        if (!success) {
+            // Look for revert reason and bubble it up if present
+            if (returndata.length > 0) {
+                // The easiest way to bubble the revert reason is using memory via assembly
+
+                // solhint-disable-next-line no-inline-assembly
+                assembly {
+                    let returndata_size := mload(returndata)
+                    revert(add(32, returndata), returndata_size)
+                }
+            } else {
+                revert(errorMessage);
+            }
+        }
+
+        return returndata;
+    }
+
+    /**
+     * @notice Function called before all delegator functions
+     */
+    function _prepare() external payable {}
+
+    /**
+     * @notice Returns a boolean indicating if the sender has admin rights
+     */
+    function hasAdminRights() internal view returns (bool) {
+        ComptrollerV3Storage comptrollerStorage = ComptrollerV3Storage(address(comptroller));
+        return (msg.sender == comptrollerStorage.admin() && comptrollerStorage.adminHasRights()) || (msg.sender == address(fuseAdmin) && comptrollerStorage.fuseAdminHasRights());
+    }
+}

contracts/CToken.sol

@@ -703,6 +703,10 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter {
         // 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.
@@ -711,10 +715,6 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter {
          */
         doTransferOut(redeemer, vars.redeemAmount);
 
-        /* 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);
@@ -803,6 +803,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.
@@ -811,11 +816,6 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter {
          */
         doTransferOut(borrower, borrowAmount);
 
-        /* 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);
 

hardhat.config.js

@@ -0,0 +1,30 @@
+require("@nomiclabs/hardhat-waffle");
+
+/**
+ * @type import('hardhat/config').HardhatUserConfig
+ */
+module.exports = {
+  solidity: {
+    version: "0.5.17",
+    settings: {
+      optimizer: {
+        enabled: true,
+        runs: 200
+      }
+    }
+  },
+  networks: {
+    hardhat: {
+      forking: {
+        url: process.env.MAINNET_WEB3_PROVIDER,
+        blockNumber: process.env.FORK_BLOCK_NUMBER !== undefined ? parseInt(process.env.FORK_BLOCK_NUMBER) : undefined // Block before attack on pool 8 = 14684685
+      }
+    },
+    development: {
+      url: "http://localhost:8546"
+    }
+  },
+  paths: {
+    tests: "./hardhat/test"
+  }
+};

hardhat/scripts/fix-exploit.js

@@ -0,0 +1,71 @@
+// We require the Hardhat Runtime Environment explicitly here. This is optional
+// but useful for running the script in a standalone fashion through `node <script>`.
+//
+// When running the script with `npx hardhat run <script>` you'll find the Hardhat
+// Runtime Environment's members available in the global scope.
+const hre = require("hardhat");
+
+async function main() {
+  // Hardhat always runs the compile task when running scripts with its command
+  // line interface.
+  //
+  // If this script is run directly using `node` you may want to call compile
+  // manually to make sure everything is compiled
+  // await hre.run('compile');
+
+  // Enable using 0 gas price
+  await hre.network.provider.send("hardhat_setNextBlockBaseFeePerGas", ["0x0"]);
+
+  // Deploy new CEtherDelegateTempExploitAccounting
+  const CEtherDelegateTempExploitAccounting = await ethers.getContractFactory("CEtherDelegateTempExploitAccounting");
+  var cEtherDelegateTempExploitAccounting = await CEtherDelegateTempExploitAccounting.deploy();
+
+  // Deploy new CEtherDelegate
+  const CEtherDelegate = await ethers.getContractFactory("CEtherDelegate");
+  var cEtherDelegate = await CEtherDelegate.deploy();
+
+  // Get pool 8 admin
+  const Comptroller = await ethers.getContractFactory("Comptroller");
+  var comptroller = Comptroller.attach("0xc54172e34046c1653d1920d40333dd358c7a1af4");
+  var comptrollerAdmin = await comptroller.admin();
+
+  // Impersonate admin of pool 8
+  await hre.network.provider.request({
+      method: "hardhat_impersonateAccount",
+      params: [comptrollerAdmin],
+  });
+
+  // Get FuseFeeDistributor owner
+  var ffd = new ethers.Contract("0xa731585ab05fC9f83555cf9Bff8F58ee94e18F85", [{"inputs":[{"internalType":"address[]","name":"oldImplementations","type":"address[]"},{"internalType":"address[]","name":"newImplementations","type":"address[]"},{"internalType":"bool[]","name":"allowResign","type":"bool[]"},{"internalType":"bool[]","name":"statuses","type":"bool[]"}],"name":"_editCEtherDelegateWhitelist","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[],"name":"owner","outputs":[{"internalType":"address","name":"","type":"address"}],"stateMutability":"view","type":"function"}], ethers.provider);
+  var ffdOwner = await ffd.owner();
+
+  // Impersonate FuseFeeDistributor owner
+  await hre.network.provider.request({
+      method: "hardhat_impersonateAccount",
+      params: [ffdOwner],
+  });
+
+  // Call FuseFeeDistributor._editCEtherDelegateWhitelist
+  await ffd.connect(await ethers.getSigner(ffdOwner))._editCEtherDelegateWhitelist(["0xd77e28a1b9a9cfe1fc2eee70e391c05d25853cbf", cEtherDelegateTempExploitAccounting.address], [cEtherDelegateTempExploitAccounting.address, cEtherDelegate.address], [false, false], [true, true], { gasPrice: "0" });
+
+  // Call CEther._setImplementationSafe for temp impl
+  var cEther = CEtherDelegate.attach("0xbB025D470162CC5eA24daF7d4566064EE7f5F111");
+  var secondaryExploiterAddresses = ["0x3686657208883d016971c7395edaed73c107383e"];
+  var becomeImplData = ethers.utils.defaultAbiCoder.encode(["address[]"], [secondaryExploiterAddresses]);
+  await cEther.connect(await ethers.getSigner(comptrollerAdmin))._setImplementationSafe(cEtherDelegateTempExploitAccounting.address, false, becomeImplData, { gasPrice: "0" });
+
+  // Call CEther._setImplementationSafe for final impl
+  await cEther.connect(await ethers.getSigner(comptrollerAdmin))._setImplementationSafe(cEtherDelegate.address, false, "0x", { gasPrice: "0" });
+
+  // Call FuseFeeDistributor._editCEtherDelegateWhitelist again
+  await ffd.connect(await ethers.getSigner(ffdOwner))._editCEtherDelegateWhitelist(["0xd77e28a1b9a9cfe1fc2eee70e391c05d25853cbf", "0xd77e28a1b9a9cfe1fc2eee70e391c05d25853cbf"], [cEtherDelegateTempExploitAccounting.address, cEtherDelegate.address], [false, false], [false, true], { gasPrice: "0" });
+}
+
+// We recommend this pattern to be able to use async/await everywhere
+// and properly handle errors.
+main()
+  .then(() => process.exit(0))
+  .catch((error) => {
+    console.error(error);
+    process.exit(1);
+  });