Add Reentrancy-Guard modifier to external MP methods

ci/run_slither.sh

@@ -14,7 +14,7 @@ run_slither() {
 
     # print out slither version for debugging
     slither --version
-    slither --exclude=divide-before-multiply,locked-ether,unused-return,timestamp,naming-convention,pragma,solc-version,external-function,reentrancy-benign,reentrancy-no-eth,arbitrary-send,incorrect-equality,reentrancy-events,assembly --filter-paths="@openzeppelin|WETH9.sol" $1
+    slither --exclude=divide-before-multiply,locked-ether,unused-return,timestamp,naming-convention,pragma,solc-version,external-function,reentrancy-benign,reentrancy-no-eth,arbitrary-send,incorrect-equality,reentrancy-events,assembly --filter-paths="@openzeppelin|WETH9.sol|ReentrancyAttack.sol|ReentrancyMock.sol" $1
 }
 
 run_slither $PROTOCOL_DIR/core

core/contracts/common/implementation/AddressWhitelist.sol

@@ -1,12 +1,13 @@
 pragma solidity ^0.6.0;
 
 import "@openzeppelin/contracts/access/Ownable.sol";
+import "./Lockable.sol";
 
 
 /**
  * @title A contract to track a whitelist of addresses.
  */
-contract AddressWhitelist is Ownable {
+contract AddressWhitelist is Ownable, Lockable {
     enum Status { None, In, Out }
     mapping(address => Status) public whitelist;
 
@@ -19,7 +20,7 @@ contract AddressWhitelist is Ownable {
      * @notice Adds an address to the whitelist.
      * @param newElement the new address to add.
      */
-    function addToWhitelist(address newElement) external onlyOwner {
+    function addToWhitelist(address newElement) external nonReentrant() onlyOwner {
         // Ignore if address is already included
         if (whitelist[newElement] == Status.In) {
             return;
@@ -39,7 +40,7 @@ contract AddressWhitelist is Ownable {
      * @notice Removes an address from the whitelist.
      * @param elementToRemove the existing address to remove.
      */
-    function removeFromWhitelist(address elementToRemove) external onlyOwner {
+    function removeFromWhitelist(address elementToRemove) external nonReentrant() onlyOwner {
         if (whitelist[elementToRemove] != Status.Out) {
             whitelist[elementToRemove] = Status.Out;
             emit RemovedFromWhitelist(elementToRemove);
@@ -51,7 +52,7 @@ contract AddressWhitelist is Ownable {
      * @param elementToCheck the address to check.
      * @return True if `elementToCheck` is on the whitelist, or False.
      */
-    function isOnWhitelist(address elementToCheck) external view returns (bool) {
+    function isOnWhitelist(address elementToCheck) external view nonReentrantView() returns (bool) {
         return whitelist[elementToCheck] == Status.In;
     }
 
@@ -63,7 +64,7 @@ contract AddressWhitelist is Ownable {
      * the empty index.
      * @return activeWhitelist the list of addresses on the whitelist.
      */
-    function getWhitelist() external view returns (address[] memory activeWhitelist) {
+    function getWhitelist() external view nonReentrantView() returns (address[] memory activeWhitelist) {
         // Determine size of whitelist first
         uint256 activeCount = 0;
         for (uint256 i = 0; i < whitelistIndices.length; i++) {

core/contracts/common/implementation/Lockable.sol

@@ -0,0 +1,63 @@
+pragma solidity ^0.6.0;
+
+
+/**
+ * @title A contract that provides modifiers to prevent reentrancy to state-changing and view-only methods. This contract
+ * is inspired by https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/ReentrancyGuard.sol
+ * and https://github.com/balancer-labs/balancer-core/blob/master/contracts/BPool.sol.
+ */
+contract Lockable {
+    bool private _notEntered;
+
+    constructor() internal {
+        // Storing an initial 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 percetange 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.
+        _notEntered = true;
+    }
+
+    /**
+     * @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 make it call a
+     * `private` function that does the actual work.
+     */
+    modifier nonReentrant() {
+        _preEntranceCheck();
+        _preEntranceSet();
+        _;
+        _postEntranceReset();
+    }
+
+    /**
+     * @dev Designed to prevent a view-only method from being re-entered during a call to a `nonReentrant()` state-changing method.
+     */
+    modifier nonReentrantView() {
+        _preEntranceCheck();
+        _;
+    }
+
+    // Internal methods are used to avoid copying the require statement's bytecode to every `nonReentrant()` method.
+    // On entry into a function, `_preEntranceCheck()` should always be called to check if the function is being re-entered.
+    // Then, if the function modifies state, it should call `_postEntranceSet()`, perform its logic, and then call `_postEntranceReset()`.
+    // View-only methods can simply call `_preEntranceCheck()` to make sure that it is not being re-entered.
+    function _preEntranceCheck() internal view {
+        // On the first call to nonReentrant, _notEntered will be true
+        require(_notEntered, "ReentrancyGuard: reentrant call");
+    }
+
+    function _preEntranceSet() internal {
+        // Any calls to nonReentrant after this point will fail
+        _notEntered = false;
+    }
+
+    function _postEntranceReset() internal {
+        // By storing the original value once again, a refund is triggered (see
+        // https://eips.ethereum.org/EIPS/eip-2200)
+        _notEntered = true;
+    }
+}

core/contracts/common/test/ReentrancyAttack.sol

@@ -0,0 +1,12 @@
+pragma solidity ^0.6.0;
+
+
+// Tests reentrancy guards defined in Lockable.sol.
+// Copied from https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.0.1/contracts/mocks/ReentrancyAttack.sol.
+contract ReentrancyAttack {
+    function callSender(bytes4 data) public {
+        // solhint-disable-next-line avoid-low-level-calls
+        (bool success, ) = msg.sender.call(abi.encodeWithSelector(data));
+        require(success, "ReentrancyAttack: failed call");
+    }
+}

core/contracts/common/test/ReentrancyMock.sol

@@ -0,0 +1,65 @@
+pragma solidity ^0.6.0;
+
+import "../implementation/Lockable.sol";
+import "./ReentrancyAttack.sol";
+
+
+// Tests reentrancy guards defined in Lockable.sol.
+// Extends https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.0.1/contracts/mocks/ReentrancyMock.sol.
+contract ReentrancyMock is Lockable {
+    uint256 public counter;
+
+    constructor() public {
+        counter = 0;
+    }
+
+    function callback() external nonReentrant {
+        _count();
+    }
+
+    function countAndSend(ReentrancyAttack attacker) external nonReentrant {
+        _count();
+        bytes4 func = bytes4(keccak256("callback()"));
+        attacker.callSender(func);
+    }
+
+    function countAndCall(ReentrancyAttack attacker) external nonReentrant {
+        _count();
+        bytes4 func = bytes4(keccak256("getCount()"));
+        attacker.callSender(func);
+    }
+
+    function countLocalRecursive(uint256 n) public nonReentrant {
+        if (n > 0) {
+            _count();
+            countLocalRecursive(n - 1);
+        }
+    }
+
+    function countThisRecursive(uint256 n) public nonReentrant {
+        if (n > 0) {
+            _count();
+            // solhint-disable-next-line avoid-low-level-calls
+            (bool success, ) = address(this).call(abi.encodeWithSignature("countThisRecursive(uint256)", n - 1));
+            require(success, "ReentrancyMock: failed call");
+        }
+    }
+
+    function countLocalCall() public nonReentrant {
+        getCount();
+    }
+
+    function countThisCall() public nonReentrant {
+        // solhint-disable-next-line avoid-low-level-calls
+        (bool success, ) = address(this).call(abi.encodeWithSignature("getCount()"));
+        require(success, "ReentrancyMock: failed call");
+    }
+
+    function getCount() public view nonReentrantView returns (uint256) {
+        return counter;
+    }
+
+    function _count() private {
+        counter += 1;
+    }
+}

core/contracts/financial-templates/implementation/ExpiringMultiParty.sol

@@ -5,5 +5,5 @@ import "./Liquidatable.sol";
 
 
 contract ExpiringMultiParty is Liquidatable {
-    constructor(ConstructorParams memory params) public Liquidatable(params) {}
+    constructor(ConstructorParams memory params) public Liquidatable(params) nonReentrant() {}
 }

core/contracts/financial-templates/implementation/ExpiringMultiPartyCreator.sol

@@ -4,6 +4,7 @@ pragma experimental ABIEncoderV2;
 import "../../oracle/implementation/ContractCreator.sol";
 import "../../common/implementation/Testable.sol";
 import "../../common/implementation/AddressWhitelist.sol";
+import "../../common/implementation/Lockable.sol";
 import "./ExpiringMultiPartyLib.sol";
 
 
@@ -12,7 +13,7 @@ import "./ExpiringMultiPartyLib.sol";
  * @notice Factory contract to create and register new instances of expiring multiparty contracts.
  * Responsible for constraining the parameters used to construct a new EMP.
  */
-contract ExpiringMultiPartyCreator is ContractCreator, Testable {
+contract ExpiringMultiPartyCreator is ContractCreator, Testable, Lockable {
     using FixedPoint for FixedPoint.Unsigned;
 
     /****************************************
@@ -82,7 +83,7 @@ contract ExpiringMultiPartyCreator is ContractCreator, Testable {
         address _collateralTokenWhitelist,
         address _tokenFactoryAddress,
         address _timerAddress
-    ) public ContractCreator(_finderAddress) Testable(_timerAddress) {
+    ) public ContractCreator(_finderAddress) Testable(_timerAddress) nonReentrant() {
         collateralTokenWhitelist = AddressWhitelist(_collateralTokenWhitelist);
         tokenFactoryAddress = _tokenFactoryAddress;
         uint32[16] memory timestamps = [
@@ -113,7 +114,7 @@ contract ExpiringMultiPartyCreator is ContractCreator, Testable {
      * @param params is a `ConstructorParams` object from ExpiringMultiParty.
      * @return address of the deployed ExpiringMultiParty contract
      */
-    function createExpiringMultiParty(Params memory params) public returns (address) {
+    function createExpiringMultiParty(Params memory params) public nonReentrant() returns (address) {
         address derivative = ExpiringMultiPartyLib.deploy(_convertParams(params));
 
         _registerContract(new address[](0), address(derivative));

core/contracts/financial-templates/implementation/FeePayer.sol

@@ -3,6 +3,7 @@ pragma experimental ABIEncoderV2;
 
 import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";
+import "../../common/implementation/Lockable.sol";
 import "../../common/implementation/FixedPoint.sol";
 import "../../common/implementation/Testable.sol";
 import "../../oracle/interfaces/StoreInterface.sol";
@@ -16,7 +17,7 @@ import "../../oracle/implementation/Constants.sol";
  * contract is abstract as each derived contract that inherits `FeePayer` must implement `pfc()`.
  */
 
-abstract contract FeePayer is Testable {
+abstract contract FeePayer is Testable, Lockable {
     using SafeMath for uint256;
     using FixedPoint for FixedPoint.Unsigned;
     using SafeERC20 for IERC20;
@@ -71,7 +72,7 @@ abstract contract FeePayer is Testable {
         address _collateralAddress,
         address _finderAddress,
         address _timerAddress
-    ) public Testable(_timerAddress) {
+    ) public Testable(_timerAddress) nonReentrant() {
         collateralCurrency = IERC20(_collateralAddress);
         finder = FinderInterface(_finderAddress);
         lastPaymentTime = getCurrentTime();
@@ -83,18 +84,18 @@ abstract contract FeePayer is Testable {
      ****************************************/
 
     /**
-     * @notice Pays UMA DVM regular fees to the Store contract.
+     * @notice Pays UMA DVM regular fees (as a % of the collateral pool) to the Store contract.
      * @dev These must be paid periodically for the life of the contract. If the contract has not paid its
      * regular fee in a week or more then a late penalty is applied which is sent to the caller.
      * @return totalPaid Amount of collateral that the contract paid (sum of the amount paid to the Store and caller).
      */
-    function payRegularFees() public returns (FixedPoint.Unsigned memory totalPaid) {
+    function payRegularFees() public nonReentrant() returns (FixedPoint.Unsigned memory totalPaid) {
         StoreInterface store = _getStore();
         uint256 time = getCurrentTime();
-        FixedPoint.Unsigned memory _pfc = pfc();
+        FixedPoint.Unsigned memory collateralPool = _pfc();
 
-        // Exit early if there is no pfc (thus, no fees to be paid).
-        if (_pfc.isEqual(0)) {
+        // Exit early if there is no collateral from which to pay fees.
+        if (collateralPool.isEqual(0)) {
             return totalPaid;
         }
 
@@ -106,16 +107,15 @@ abstract contract FeePayer is Testable {
         (FixedPoint.Unsigned memory regularFee, FixedPoint.Unsigned memory latePenalty) = store.computeRegularFee(
             lastPaymentTime,
             time,
-            _pfc
+            collateralPool
         );
         lastPaymentTime = time;
 
         emit RegularFeesPaid(regularFee.rawValue, latePenalty.rawValue);
 
         totalPaid = regularFee.add(latePenalty);
 
-        // Adjust the cumulative fee multiplier by the fee paid and the current PFC.
-        _adjustCumulativeFeeMultiplier(totalPaid, _pfc);
+        _adjustCumulativeFeeMultiplier(totalPaid, collateralPool);
 
         if (regularFee.isGreaterThan(0)) {
             collateralCurrency.safeIncreaseAllowance(address(store), regularFee.rawValue);
@@ -140,13 +140,12 @@ abstract contract FeePayer is Testable {
             collateralCurrency.safeTransferFrom(payer, address(this), amount.rawValue);
         } else {
             // If the payer is the contract, adjust the cumulativeFeeMultiplier to compensate.
-            FixedPoint.Unsigned memory _pfc = pfc();
+            FixedPoint.Unsigned memory collateralPool = _pfc();
 
-            // The final fee must be < pfc or the fee will be larger than 100%.
-            require(_pfc.isGreaterThan(amount), "Final fee is more than PfC");
+            // The final fee must be < available collateral or the fee will be larger than 100%.
+            require(collateralPool.isGreaterThan(amount), "Final fee is more than PfC");
 
-            // Adjust the cumulative fee multiplier by the fee paid and the current PFC.
-            _adjustCumulativeFeeMultiplier(amount, _pfc);
+            _adjustCumulativeFeeMultiplier(amount, collateralPool);
         }
 
         emit FinalFeesPaid(amount.rawValue);
@@ -158,15 +157,20 @@ abstract contract FeePayer is Testable {
 
     /**
      * @notice Gets the current profit from corruption for this contract in terms of the collateral currency.
-     * @dev Derived contracts are expected to implement this function so the payRegularFees()
-     * method can correctly compute the owed regular fees.
+     * @dev This will be equivalent to the collateral pool available from which to pay fees.
+     * Therefore, derived contracts are expected to implement this so that pay-fee methods
+     * can correctly compute the owed fees as a % of PfC.
      */
-    function pfc() public virtual view returns (FixedPoint.Unsigned memory);
+    function pfc() public view nonReentrantView() returns (FixedPoint.Unsigned memory) {
+        return _pfc();
+    }
 
     /****************************************
      *         INTERNAL FUNCTIONS           *
      ****************************************/
 
+    function _pfc() internal virtual view returns (FixedPoint.Unsigned memory);
+
     function _getStore() internal view returns (StoreInterface) {
         return StoreInterface(finder.getImplementationAddress(OracleInterfaces.Store));
     }
@@ -230,7 +234,7 @@ abstract contract FeePayer is Testable {
         addedCollateral = _getFeeAdjustedCollateral(rawCollateral).sub(initialBalance);
     }
 
-    // Scale the cumulativeFeeMultiplier by the ratio of fees paid to the current profit from corruption.
+    // Scale the cumulativeFeeMultiplier by the ratio of fees paid to the current available collateral.
     function _adjustCumulativeFeeMultiplier(FixedPoint.Unsigned memory amount, FixedPoint.Unsigned memory currentPfc)
         internal
     {