VAL-139 Missing Sanity Checks (#176)

* Add validation that Loans cannot be created with a liquidity asset that does not match its corresponding Pool asset.

* Add validation that serviceFeeBps for Pools cannot exceed 100%

* Add semi-duplicative require statement enforcing maxRedeem() constraint in redeem()

* Add missing require statements to the Factories. Update mock contracts to reflect added validation done in the Loan in prior commit.

* Code review

contracts/Loan.sol

@@ -138,6 +138,7 @@ contract Loan is ILoan, BeaconImplementation {
 
         LoanLib.validateLoan(
             _serviceConfiguration,
+            IPool(_pool),
             settings.duration,
             settings.paymentPeriod,
             settings.principal,

contracts/Pool.sol

@@ -898,6 +898,7 @@ contract Pool is IPool, ERC20Upgradeable, BeaconImplementation {
         require(receiver == owner, "Pool: Withdrawal to unrelated address");
         require(receiver == msg.sender, "Pool: Must transfer to msg.sender");
         require(shares > 0, "Pool: 0 redeem not allowed");
+        require(maxRedeem(owner) >= shares, "Pool: InsufficientBalance");
 
         // Update the withdraw state
         assets = withdrawController.redeem(owner, shares);

contracts/factories/PoolFactory.sol

@@ -89,6 +89,10 @@ contract PoolFactory is IPoolFactory, BeaconProxyFactory {
             settings.requestCancellationFeeBps <= 10_000,
             "PoolFactory: Invalid request cancellation fee"
         );
+        require(
+            settings.serviceFeeBps <= 10_000,
+            "PoolFactory: Invalid service fee"
+        );
         require(
             _serviceConfiguration.isLiquidityAsset(liquidityAsset),
             "PoolFactory: invalid asset"

contracts/factories/VaultFactory.sol

@@ -25,6 +25,10 @@ contract VaultFactory is IVaultFactory, BeaconProxyFactory {
      * @inheritdoc IVaultFactory
      */
     function createVault(address owner) public override returns (address addr) {
+        require(
+            implementation != address(0),
+            "VaultFactory: no implementation set"
+        );
         BeaconProxy proxy = new BeaconProxy(
             address(this),
             abi.encodeWithSelector(

contracts/factories/WithdrawControllerFactory.sol

@@ -36,6 +36,10 @@ contract WithdrawControllerFactory is
             _serviceConfiguration.paused() == false,
             "WithdrawControllerFactory: Protocol paused"
         );
+        require(
+            implementation != address(0),
+            "WithdrawControllerFactory: no impl"
+        );
 
         BeaconProxy proxy = new BeaconProxy(
             address(this),

contracts/libraries/LoanLib.sol

@@ -74,6 +74,7 @@ library LoanLib {
      */
     function validateLoan(
         IServiceConfiguration config,
+        IPool pool,
         uint256 duration,
         uint256 paymentPeriod,
         uint256 principal,
@@ -91,6 +92,10 @@ library LoanLib {
             config.isLiquidityAsset(liquidityAsset),
             "LoanLib: Liquidity asset not allowed"
         );
+        require(
+            pool.asset() == liquidityAsset,
+            "LoanLib: Not allowed asset for pool"
+        );
     }
 
     /**

contracts/mocks/PoolLibTestWrapper.sol

@@ -13,6 +13,7 @@ import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet
 contract PoolLibTestWrapper is ERC20("PoolLibTest", "PLT") {
     using EnumerableSet for EnumerableSet.AddressSet;
 
+    address public asset;
     EnumerableSet.AddressSet private _activeLoans;
     IPoolAccountings private _accountings;
 
@@ -34,6 +35,10 @@ contract PoolLibTestWrapper is ERC20("PoolLibTest", "PLT") {
         uint256 shares
     );
 
+    constructor(address _asset) {
+        asset = _asset;
+    }
+
     function executeFirstLossDeposit(
         address liquidityAsset,
         address spender,
@@ -106,27 +111,27 @@ contract PoolLibTestWrapper is ERC20("PoolLibTest", "PLT") {
     }
 
     function calculateTotalAssets(
-        address asset,
+        address asset_,
         address vault,
         uint256 outstandingLoanPrincipals
     ) external view returns (uint256) {
         return
             PoolLib.calculateTotalAssets(
-                asset,
+                asset_,
                 vault,
                 outstandingLoanPrincipals
             );
     }
 
     function calculateTotalAvailableAssets(
-        address asset,
+        address asset_,
         address vault,
         uint256 outstandingLoanPrincipals,
         uint256 withdrawableAssets
     ) external view returns (uint256) {
         return
             PoolLib.calculateTotalAvailableAssets(
-                asset,
+                asset_,
                 vault,
                 outstandingLoanPrincipals,
                 withdrawableAssets
@@ -173,7 +178,7 @@ contract PoolLibTestWrapper is ERC20("PoolLibTest", "PLT") {
     }
 
     function executeDeposit(
-        address asset,
+        address asset_,
         address vault,
         address sharesReceiver,
         uint256 assets,
@@ -182,7 +187,7 @@ contract PoolLibTestWrapper is ERC20("PoolLibTest", "PLT") {
     ) external returns (uint256) {
         return
             PoolLib.executeDeposit(
-                asset,
+                asset_,
                 vault,
                 sharesReceiver,
                 assets,

test/factories/LoanFactory.test.ts

@@ -0,0 +1,94 @@
+import { loadFixture } from "@nomicfoundation/hardhat-network-helpers";
+import { expect } from "chai";
+import { ethers } from "hardhat";
+import { activatePool, deployPool, deployVaultFactory } from "../support/pool";
+import { DEFAULT_LOAN_SETTINGS } from "../support/loan";
+import { getCommonSigners } from "../support/utils";
+import { deployMockERC20 } from "../support/erc20";
+
+describe("LoanFactory", () => {
+  async function deployLoanFactoryFixture() {
+    const { deployer, operator, pauser, poolAdmin, borrower } =
+      await getCommonSigners();
+
+    // Create a pool
+    const { pool, liquidityAsset, serviceConfiguration } = await deployPool({
+      poolAdmin: poolAdmin,
+      pauser
+    });
+
+    // Create a difference ERC20
+    const { mockERC20: otherMockERC2O } = await deployMockERC20(
+      "OtherTestToken",
+      "OTT"
+    );
+
+    await activatePool(pool, poolAdmin, liquidityAsset);
+
+    const LoanLib = await ethers.getContractFactory("LoanLib");
+    const loanLib = await LoanLib.deploy();
+
+    const vaultFactory = await deployVaultFactory(serviceConfiguration.address);
+
+    const LoanFactory = await ethers.getContractFactory("LoanFactory");
+    const loanFactory = await LoanFactory.deploy(
+      serviceConfiguration.address,
+      vaultFactory.address
+    );
+    await loanFactory.deployed();
+
+    await serviceConfiguration
+      .connect(operator)
+      .setLoanFactory(loanFactory.address, true);
+
+    // Deploy Loan implementation contract
+    const LoanImpl = await ethers.getContractFactory("Loan", {
+      libraries: {
+        LoanLib: loanLib.address
+      }
+    });
+    const loanImpl = await LoanImpl.deploy();
+
+    // Set implementation on the LoanFactory
+    await loanFactory.connect(deployer).setImplementation(loanImpl.address);
+
+    return {
+      loanFactory,
+      borrower,
+      pool,
+      liquidityAsset,
+      otherMockERC2O,
+      serviceConfiguration,
+      operator
+    };
+  }
+
+  describe("createLoan()", () => {
+    it("reverts if liquidity asset doesn't match the pool", async () => {
+      const {
+        loanFactory,
+        borrower,
+        pool,
+        otherMockERC2O,
+        serviceConfiguration,
+        operator
+      } = await loadFixture(deployLoanFactoryFixture);
+
+      // Set otherMockERC20 as a supported currency in the protocol
+      // However, it's still mismatched with the pool, so we expect creating the loan to fail
+      await serviceConfiguration
+        .connect(operator)
+        .setLiquidityAsset(otherMockERC2O.address, true);
+      expect(await pool.asset()).to.not.equal(otherMockERC2O.address);
+
+      await expect(
+        loanFactory.createLoan(
+          borrower.address,
+          pool.address,
+          otherMockERC2O.address,
+          DEFAULT_LOAN_SETTINGS
+        )
+      ).to.be.revertedWith("LoanLib: Not allowed asset for pool");
+    });
+  });
+});

test/factories/PoolFactory.test.ts

@@ -197,6 +197,18 @@ describe("PoolFactory", () => {
     await expect(tx).to.be.revertedWith("PoolFactory: Protocol paused");
   });
 
+  it("reverts if serviceFeeBps exceeds 100%", async () => {
+    const { poolFactory, liquidityAsset } = await loadFixture(deployFixture);
+
+    // Attempt to create a pool with > 100% withdraw gate
+    const poolSettings = Object.assign({}, DEFAULT_POOL_SETTINGS, {
+      serviceFeeBps: 10_001
+    });
+
+    const tx = poolFactory.createPool(liquidityAsset.address, poolSettings);
+    await expect(tx).to.be.revertedWith("PoolFactory: Invalid service fee");
+  });
+
   it("emits PoolCreated", async () => {
     const { poolFactory, liquidityAsset } = await loadFixture(deployFixture);
 

test/factories/VaultFactory.test.ts

@@ -0,0 +1,35 @@
+import { loadFixture } from "@nomicfoundation/hardhat-network-helpers";
+import { expect } from "chai";
+import { ethers } from "hardhat";
+import { getCommonSigners } from "../support/utils";
+import { deployServiceConfiguration } from "../support/serviceconfiguration";
+
+describe("VaultFactory", () => {
+  async function deployVaultFactoryFixture() {
+    const { deployer, other } = await getCommonSigners();
+    const { serviceConfiguration } = await deployServiceConfiguration();
+
+    const Factory = await ethers.getContractFactory("VaultFactory");
+    const factory = await Factory.deploy(serviceConfiguration.address);
+
+    // Create Vault implementation
+    const Vault = await ethers.getContractFactory("Vault");
+    const vault = await Vault.deploy();
+    return {
+      vault,
+      factory,
+      deployer,
+      other
+    };
+  }
+
+  describe("createVault()", () => {
+    it("reverts if no implementation is set", async () => {
+      const { factory, other } = await loadFixture(deployVaultFactoryFixture);
+
+      await expect(factory.createVault(other.address)).to.be.revertedWith(
+        "VaultFactory: no implementation set"
+      );
+    });
+  });
+});

test/libraries/PoolLib.test.ts

@@ -25,12 +25,12 @@ describe("PoolLib", () => {
         }
       }
     );
-    const poolLibWrapper = await PoolLibWrapper.deploy();
-    await poolLibWrapper.deployed();
-
     const liquidityAsset = (await deployMockERC20()).mockERC20;
-
     await liquidityAsset.mint(caller.address, FIRST_LOSS_AMOUNT);
+
+    const poolLibWrapper = await PoolLibWrapper.deploy(liquidityAsset.address);
+    await poolLibWrapper.deployed();
+
     await liquidityAsset
       .connect(caller)
       .approve(poolLibWrapper.address, FIRST_LOSS_AMOUNT);