Merge pull request #14 from pie-dao/maint/audit-fixes

Audit fixes

contracts/callManagers/LendingManager/LendingLogicAave.sol

@@ -13,6 +13,7 @@ contract LendingLogicAave is ILendingLogic {
     uint16 public referralCode;
 
     constructor(address _lendingPool, uint16 _referralCode) {
+        require(_lendingPool != address(0), "LENDING_POOL_INVALID");
         lendingPool = IAaveLendingPool(_lendingPool);
         referralCode = _referralCode;
     }

contracts/callManagers/LendingManager/LendingLogicCompound.sol

@@ -13,6 +13,7 @@ contract LendingLogicCompound is ILendingLogic {
     bytes32 public constant PROTOCOL = keccak256(abi.encodePacked("Compound"));
 
     constructor(address _lendingRegistry) {
+        require(_lendingRegistry != address(0), "INVALID_LENDING_REGISTRY");
         lendingRegistry = LendingRegistry(_lendingRegistry);
     }
 

contracts/callManagers/LendingManager/LendingManager.sol

@@ -3,11 +3,12 @@ pragma experimental ABIEncoderV2;
 pragma solidity ^0.7.1;
 
 import "@openzeppelin/contracts/access/Ownable.sol";
+import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
 import "@openzeppelin/contracts/math/Math.sol";
 import "./LendingRegistry.sol";
 import "../../interfaces/IExperiPie.sol";
 
-contract LendingManager is Ownable {
+contract LendingManager is Ownable, ReentrancyGuard {
     using Math for uint256;
 
     LendingRegistry public lendingRegistry;
@@ -21,6 +22,8 @@ contract LendingManager is Ownable {
         @param _basket Address of the pool/pie/basket to manage
     */
     constructor(address _lendingRegistry, address _basket) public {
+        require(_lendingRegistry != address(0), "INVALID_LENDING_REGISTRY");
+        require(_basket != address(0), "INVALID_BASKET");
         lendingRegistry = LendingRegistry(_lendingRegistry);
         basket = IExperiPie(_basket);
     }
@@ -31,7 +34,7 @@ contract LendingManager is Ownable {
         @param _amount Amount of underlying to lend
         @param _protocol Bytes32 protocol key to lend to
     */
-    function lend(address _underlying, uint256 _amount, bytes32 _protocol) public onlyOwner {
+    function lend(address _underlying, uint256 _amount, bytes32 _protocol) public onlyOwner nonReentrant {
         // _amount or actual balance, whatever is less
         uint256 amount = _amount.min(IERC20(_underlying).balanceOf(address(basket)));
 
@@ -57,7 +60,7 @@ contract LendingManager is Ownable {
         @param _wrapped Address of the wrapped token
         @param _amount Amount of the wrapped token to unlend
     */
-    function unlend(address _wrapped, uint256 _amount) public onlyOwner {
+    function unlend(address _wrapped, uint256 _amount) public onlyOwner nonReentrant {
         // unlend token
          // _amount or actual balance, whatever is less
         uint256 amount = _amount.min(IERC20(_wrapped).balanceOf(address(basket)));
@@ -83,6 +86,7 @@ contract LendingManager is Ownable {
         @param _wrapped Address of the wrapped token to bounce to another protocol
         @param _amount Amount of the wrapped token to bounce to the other protocol
         @param _toProtocol Protocol to deposit bounced tokens in
+        @dev Uses reentrency protection of unlend() and lend()
     */
     function bounce(address _wrapped, uint256 _amount, bytes32 _toProtocol) external {
        unlend(_wrapped, _amount);

contracts/callManagers/RSIManager.sol

@@ -14,6 +14,11 @@ contract RSISynthetixManager {
     bytes32 public immutable assetShortKey;
     bytes32 public immutable assetLongKey;
 
+    // Value under which to go long (30 * 10**18 == 30)
+    int256 public immutable rsiBottom;
+    // Value under which to go short
+    int256 public immutable rsiTop;
+
     IPriceReferenceFeed public immutable priceFeed;
     IExperiPie public immutable basket;
     ISynthetix public immutable synthetix;
@@ -33,6 +38,8 @@ contract RSISynthetixManager {
         address _assetLong,
         bytes32 _assetShortKey,
         bytes32 _assetLongKey,
+        int256 _rsiBottom,
+        int256 _rsiTop,
         address _priceFeed,
         address _basket,
         address _synthetix
@@ -41,6 +48,23 @@ contract RSISynthetixManager {
         assetLong = _assetLong;
         assetShortKey = _assetShortKey;
         assetLongKey = _assetLongKey;
+
+        require(_assetShort != address(0), "INVALID_ASSET_SHORT");
+        require(_assetLong != address(0), "INVALID_ASSET_LONG");
+        require(_assetShortKey != bytes32(0), "INVALID_ASSET_SHORT_KEY");
+        require(_assetLongKey != bytes32(0), "INVALID_ASSET_LONG_KEY");
+
+        require(_rsiBottom < _rsiTop, "RSI bottom should be bigger than RSI top");
+        require(_rsiBottom > 0, "RSI bottom should be bigger than 0");
+        require(_rsiTop < 100 * 10**18, "RSI top should be less than 100");
+
+        require(_priceFeed != address(0), "INVALID_PRICE_FEED");
+        require(_basket != address(0), "INVALID_BASKET");
+        require(_synthetix != address(0), "INVALID_SYNTHETIX");
+
+        rsiBottom = _rsiBottom;
+        rsiTop = _rsiTop;
+
         priceFeed = IPriceReferenceFeed(_priceFeed);
         basket = IExperiPie(_basket);
         synthetix = ISynthetix(_synthetix);
@@ -51,11 +75,11 @@ contract RSISynthetixManager {
         RoundData memory roundData = readLatestRound();
         require(roundData.updatedAt > 0, "Round not complete");
 
-        if(roundData.answer <= 30 * 10**18) {
+        if(roundData.answer <= rsiBottom) {
             // long
             long();
             return;
-        } else if(roundData.answer >= 70 * 10**18) {
+        } else if(roundData.answer >= rsiTop) {
             // Short
             short();
             return;

contracts/facets/Basket/BasketFacet.sol

@@ -18,12 +18,15 @@ contract BasketFacet is ReentryProtection, CallProtection, IBasketFacet {
     uint256 public constant MAX_ENTRY_FEE = 10**17; // 10%
     uint256 public constant MAX_EXIT_FEE = 10**17; // 10%
     uint256 public constant MAX_ANNUAL_FEE = 10**17; // 10%
+    uint256 public constant HUNDRED_PERCENT = 10 ** 18;
 
-
+    // Assuming a block gas limit of 12M this allows for a gas consumption per token of roughly 333k allowing 2M of overhead for addtional operations
+    uint256 public constant MAX_TOKENS = 30;
 
     function addToken(address _token) external override protectedCall {
         LibBasketStorage.BasketStorage storage bs = LibBasketStorage.basketStorage();
         require(!bs.inPool[_token], "TOKEN_ALREADY_IN_POOL");
+        require(bs.tokens.length < MAX_TOKENS, "TOKEN_LIMIT_REACHED");
         // Enforce minimum to avoid rounding errors; (Minimum value is the same as in Balancer)
         require(balance(_token) >= MIN_AMOUNT, "BALANCE_TOO_SMALL");
 
@@ -41,17 +44,14 @@ contract BasketFacet is ReentryProtection, CallProtection, IBasketFacet {
         bs.inPool[_token] = false;
 
         // remove token from array
-        // TODO consider limiting max amount of tokens to mitigate running out of gas.
         for(uint256 i; i < bs.tokens.length; i ++) {
             if(address(bs.tokens[i]) == _token) {
                 bs.tokens[i] = bs.tokens[bs.tokens.length - 1];
                 bs.tokens.pop();
-
+                emit TokenRemoved(_token);
                 break;
             }
         }
-
-        emit TokenRemoved(_token);
     }
 
     function setEntryFee(uint256 _fee) external override protectedCall {
@@ -75,6 +75,7 @@ contract BasketFacet is ReentryProtection, CallProtection, IBasketFacet {
     }
 
     function setAnnualizedFee(uint256 _fee) external override protectedCall {
+        chargeOutstandingAnnualizedFee();
         require(_fee <= MAX_ANNUAL_FEE, "FEE_TOO_BIG");
         LibBasketStorage.basketStorage().annualizedFee = _fee;
         emit AnnualizedFeeSet(_fee);
@@ -85,6 +86,7 @@ contract BasketFacet is ReentryProtection, CallProtection, IBasketFacet {
     }
 
     function setFeeBeneficiary(address _beneficiary) external override protectedCall {
+        chargeOutstandingAnnualizedFee();
         LibBasketStorage.basketStorage().feeBeneficiary = _beneficiary;
         emit FeeBeneficiarySet(_beneficiary);
     }
@@ -94,7 +96,7 @@ contract BasketFacet is ReentryProtection, CallProtection, IBasketFacet {
     }
 
     function setEntryFeeBeneficiaryShare(uint256 _share) external override protectedCall {
-        require(_share <= 10**18, "FEE_SHARE_TOO_BIG");
+        require(_share <= HUNDRED_PERCENT, "FEE_SHARE_TOO_BIG");
         LibBasketStorage.basketStorage().entryFeeBeneficiaryShare = _share;
         emit EntryFeeBeneficiaryShareSet(_share);
     }
@@ -104,7 +106,7 @@ contract BasketFacet is ReentryProtection, CallProtection, IBasketFacet {
     }
 
     function setExitFeeBeneficiaryShare(uint256 _share) external override protectedCall {
-        require(_share <= 10**18, "FEE_SHARE_TOO_BIG");
+        require(_share <= HUNDRED_PERCENT, "FEE_SHARE_TOO_BIG");
         LibBasketStorage.basketStorage().exitFeeBeneficiaryShare = _share;
         emit ExitFeeBeneficiaryShareSet(_share);
     }
@@ -119,7 +121,7 @@ contract BasketFacet is ReentryProtection, CallProtection, IBasketFacet {
         chargeOutstandingAnnualizedFee();
         LibBasketStorage.BasketStorage storage bs = LibBasketStorage.basketStorage();
         uint256 totalSupply = LibERC20Storage.erc20Storage().totalSupply;
-        require(totalSupply.add(_amount) < this.getCap(), "MAX_POOL_CAP_REACHED");
+        require(totalSupply.add(_amount) <= this.getCap(), "MAX_POOL_CAP_REACHED");
 
         uint256 feeAmount = _amount.mul(bs.entryFee).div(10**18);
 

contracts/facets/Call/CallFacet.sol

@@ -10,6 +10,8 @@ import "./LibCallStorage.sol";
 
 contract CallFacet is ReentryProtection, ICallFacet {
 
+  uint256 public constant MAX_CALLERS = 50;
+
   // uses modified call protection modifier to also allow whitelisted addresses to call
   modifier protectedCall() {
     require(
@@ -20,20 +22,25 @@ contract CallFacet is ReentryProtection, ICallFacet {
     _;
   }
 
-  function addCaller(address _caller) external override {
+  modifier onlyOwner() {
     require(msg.sender == LibDiamond.diamondStorage().contractOwner, "NOT_ALLOWED");
+    _;
+  }
+
+  function addCaller(address _caller) external override onlyOwner {
     LibCallStorage.CallStorage storage callStorage = LibCallStorage.callStorage();
 
+    require(callStorage.callers.length < MAX_CALLERS, "TOO_MANY_CALLERS");
     require(!callStorage.canCall[_caller], "IS_ALREADY_CALLER");
+    require(_caller != address(0), "INVALID_CALLER");
 
     callStorage.callers.push(_caller);
     callStorage.canCall[_caller] = true;
 
     emit CallerAdded(_caller);
   }
 
-  function removeCaller(address _caller) external override {
-    require(msg.sender == LibDiamond.diamondStorage().contractOwner, "NOT_ALLOWED");
+  function removeCaller(address _caller) external override onlyOwner {
     LibCallStorage.CallStorage storage callStorage = LibCallStorage.callStorage();
 
     require(callStorage.canCall[_caller], "IS_NOT_CALLER");
@@ -96,9 +103,10 @@ contract CallFacet is ReentryProtection, ICallFacet {
     bytes memory _calldata,
     uint256 _value
   ) internal {
+    require(address(this).balance >= _value, "ETH_BALANCE_TOO_LOW");
     (bool success, ) = _target.call{ value: _value }(_calldata);
     require(success, "CALL_FAILED");
-    emit Call(_target, _calldata, _value);
+    emit Call(msg.sender, _target, _calldata, _value);
   }
 
   function canCall(address _caller) external view override returns (bool) {

contracts/facets/ERC20/ERC20Facet.sol

@@ -21,6 +21,18 @@ contract ERC20Facet is IERC20, IERC20Facet, CallProtection {
     LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
     LibERC20Storage.ERC20Storage storage es = LibERC20Storage.erc20Storage();
 
+    require(
+      bytes(es.name).length == 0 &&
+      bytes(es.symbol).length == 0,
+      "ALREADY_INITIALIZED"
+    );
+
+    require(
+      bytes(_name).length != 0 &&
+      bytes(_symbol).length != 0,
+      "INVALID_PARAMS"
+    );
+
     require(msg.sender == ds.contractOwner, "Must own the contract.");
 
     LibERC20.mint(msg.sender, _initialSupply);
@@ -54,11 +66,33 @@ contract ERC20Facet is IERC20, IERC20Facet, CallProtection {
     override
     returns (bool)
   {
+    require(_spender != address(0), "SPENDER_INVALID");
     LibERC20Storage.erc20Storage().allowances[msg.sender][_spender] = _amount;
     emit Approval(msg.sender, _spender, _amount);
     return true;
   }
 
+  function increaseApproval(address _spender, uint256 _amount) external override returns (bool) {
+    require(_spender != address(0), "SPENDER_INVALID");
+    LibERC20Storage.ERC20Storage storage es = LibERC20Storage.erc20Storage();
+    es.allowances[msg.sender][_spender] = es.allowances[msg.sender][_spender].add(_amount);
+    emit Approval(msg.sender, _spender, es.allowances[msg.sender][_spender]);
+    return true;
+  }
+
+  function decreaseApproval(address _spender, uint256 _amount) external override returns (bool) {
+    require(_spender != address(0), "SPENDER_INVALID");
+    LibERC20Storage.ERC20Storage storage es = LibERC20Storage.erc20Storage();
+    uint256 oldValue = es.allowances[msg.sender][_spender];
+    if (_amount > oldValue) {
+      es.allowances[msg.sender][_spender] = 0;
+    } else {
+      es.allowances[msg.sender][_spender] = oldValue.sub(_amount);
+    }
+    emit Approval(msg.sender, _spender, es.allowances[msg.sender][_spender]);
+    return true;
+  }
+
   function transfer(address _to, uint256 _amount)
     external
     override
@@ -74,6 +108,7 @@ contract ERC20Facet is IERC20, IERC20Facet, CallProtection {
     uint256 _amount
   ) external override returns (bool) {
     LibERC20Storage.ERC20Storage storage es = LibERC20Storage.erc20Storage();
+    require(_from != address(0), "FROM_INVALID");
 
     // Update approval if not set to max uint256
     if (es.allowances[_from][msg.sender] != uint256(-1)) {
@@ -83,6 +118,7 @@ contract ERC20Facet is IERC20, IERC20Facet, CallProtection {
     }
 
     _transfer(_from, _to, _amount);
+    return true;
   }
 
   function allowance(address _owner, address _spender)
@@ -107,9 +143,6 @@ contract ERC20Facet is IERC20, IERC20Facet, CallProtection {
     address _to,
     uint256 _amount
   ) internal {
-    if (_to == address(0)) {
-      return LibERC20.burn(msg.sender, _amount);
-    }
     LibERC20Storage.ERC20Storage storage es = LibERC20Storage.erc20Storage();
 
     es.balances[_from] = es.balances[_from].sub(_amount);

contracts/facets/ERC20/LibERC20.sol

@@ -12,6 +12,8 @@ library LibERC20 {
   event Transfer(address indexed from, address indexed to, uint256 amount);
 
   function mint(address _to, uint256 _amount) internal {
+    require(_to != address(0), "INVALID_TO_ADDRESS");
+
     LibERC20Storage.ERC20Storage storage es = LibERC20Storage.erc20Storage();
 
     es.balances[_to] = es.balances[_to].add(_amount);

contracts/factories/PieFactoryContract.sol

@@ -37,6 +37,7 @@ contract PieFactoryContract is Ownable {
     }
 
     function removeFacet(uint256 _index) external onlyOwner {
+        require(_index < defaultCut.length, "INVALID_INDEX");
         emit FacetRemoved(defaultCut[_index]);
         defaultCut[_index] = defaultCut[defaultCut.length - 1];
         defaultCut.pop();

contracts/interfaces/ICallFacet.sol

@@ -6,7 +6,7 @@ interface ICallFacet {
 
     event CallerAdded(address indexed caller);
     event CallerRemoved(address indexed caller);
-    event Call(address indexed target, bytes data, uint256 value);
+    event Call(address indexed caller, address indexed target, bytes data, uint256 value);
 
     /**
         @notice Lets whitelisted callers execute a batch of arbitrary calls from the pool. Reverts if one of the calls fails

contracts/interfaces/IERC20Facet.sol

@@ -47,4 +47,18 @@ interface IERC20Facet {
         string memory _name,
         string memory _symbol
     ) external;
+
+    /**
+        @notice Increase the amount of tokens another address can spend
+        @param _spender Spender
+        @param _amount Amount to increase by
+    */
+    function increaseApproval(address _spender, uint256 _amount) external returns (bool);
+
+    /**
+        @notice Decrease the amount of tokens another address can spend
+        @param _spender Spender
+        @param _amount Amount to decrease by
+    */
+    function decreaseApproval(address _spender, uint256 _amount) external returns (bool);
 }

test/LendingManager.ts

@@ -176,6 +176,17 @@ describe("LendingManager", function() {
         await timeTraveler.revertSnapshot();
     });
 
+    describe("Constructor", async() => {
+        it("Should fail when lending registry is set to zero address", async() => {
+            const promise = deployContract(signers[0], LendingManagerArtifact, [constants.AddressZero, pie.address]);
+            await expect(promise).to.be.revertedWith("INVALID_LENDING_REGISTRY");
+        });
+        it("Should fail when basket is set to zero address", async() => {
+            const promise = deployContract(signers[0], LendingManagerArtifact, [lendingRegistry.address, constants.AddressZero]);
+            await expect(promise).to.be.revertedWith("INVALID_BASKET");
+        });
+    });
+
     describe("Lending", async() => {
         describe("Aave", async() => {
             it("Lending less than max should work", async() => {