From ea3423702c20380b159b485f45c101c22065644e Mon Sep 17 00:00:00 2001 From: ams9198 <111915188+ams9198@users.noreply.github.com> Date: Tue, 14 Feb 2023 12:46:03 -0500 Subject: [PATCH] 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 | 1 + contracts/Pool.sol | 1 + contracts/factories/PoolFactory.sol | 4 + contracts/factories/VaultFactory.sol | 4 + .../factories/WithdrawControllerFactory.sol | 4 + contracts/libraries/LoanLib.sol | 5 + contracts/mocks/PoolLibTestWrapper.sol | 17 ++-- test/factories/LoanFactory.test.ts | 94 +++++++++++++++++++ test/factories/PoolFactory.test.ts | 12 +++ test/factories/VaultFactory.test.ts | 35 +++++++ test/libraries/PoolLib.test.ts | 8 +- 11 files changed, 175 insertions(+), 10 deletions(-) create mode 100644 test/factories/LoanFactory.test.ts create mode 100644 test/factories/VaultFactory.test.ts diff --git a/contracts/Loan.sol b/contracts/Loan.sol index 10d6bf10..1233b4fb 100644 --- a/contracts/Loan.sol +++ b/contracts/Loan.sol @@ -138,6 +138,7 @@ contract Loan is ILoan, BeaconImplementation { LoanLib.validateLoan( _serviceConfiguration, + IPool(_pool), settings.duration, settings.paymentPeriod, settings.principal, diff --git a/contracts/Pool.sol b/contracts/Pool.sol index 451aadaa..b8c64cb7 100644 --- a/contracts/Pool.sol +++ b/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); diff --git a/contracts/factories/PoolFactory.sol b/contracts/factories/PoolFactory.sol index e883874b..5648ecbd 100644 --- a/contracts/factories/PoolFactory.sol +++ b/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" diff --git a/contracts/factories/VaultFactory.sol b/contracts/factories/VaultFactory.sol index 3991303c..3582de0c 100644 --- a/contracts/factories/VaultFactory.sol +++ b/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( diff --git a/contracts/factories/WithdrawControllerFactory.sol b/contracts/factories/WithdrawControllerFactory.sol index 5f20e125..1cbc633f 100644 --- a/contracts/factories/WithdrawControllerFactory.sol +++ b/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), diff --git a/contracts/libraries/LoanLib.sol b/contracts/libraries/LoanLib.sol index fc8cc7bd..f3a4cce4 100644 --- a/contracts/libraries/LoanLib.sol +++ b/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" + ); } /** diff --git a/contracts/mocks/PoolLibTestWrapper.sol b/contracts/mocks/PoolLibTestWrapper.sol index 819140db..2a28c2d4 100644 --- a/contracts/mocks/PoolLibTestWrapper.sol +++ b/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, diff --git a/test/factories/LoanFactory.test.ts b/test/factories/LoanFactory.test.ts new file mode 100644 index 00000000..88a38878 --- /dev/null +++ b/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"); + }); + }); +}); diff --git a/test/factories/PoolFactory.test.ts b/test/factories/PoolFactory.test.ts index 533c7660..3d1ab8b9 100644 --- a/test/factories/PoolFactory.test.ts +++ b/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); diff --git a/test/factories/VaultFactory.test.ts b/test/factories/VaultFactory.test.ts new file mode 100644 index 00000000..464ff9f4 --- /dev/null +++ b/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" + ); + }); + }); +}); diff --git a/test/libraries/PoolLib.test.ts b/test/libraries/PoolLib.test.ts index 38e64bdf..32243e84 100644 --- a/test/libraries/PoolLib.test.ts +++ b/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);