From 2d03637d23885977404c0b868e063480f5f419cd Mon Sep 17 00:00:00 2001 From: Matt Venables Date: Thu, 1 Dec 2022 16:39:08 -0500 Subject: [PATCH] [VAL-43] Update PermissionedLoan to use override modifier pattern (#122) * Move verite logic out of PoolAccessControl to inherited contract * Update isValidParticipant to be isAllowed * Don't publicly expose Verite isVerified method * Update PoolAdminAccessControl to rely ton verite * Update PermissionedLoan to use override modifier pattern * Fix formatting --- contracts/Loan.sol | 34 +++++++++++---- contracts/permissioned/PermissionedLoan.sol | 40 +----------------- contracts/permissioned/PermissionedPool.sol | 5 +-- test/permissioned/PermissionedLoan.test.ts | 46 +++++++++++++++++++-- test/permissioned/PermissionedPool.test.ts | 8 ++-- 5 files changed, 76 insertions(+), 57 deletions(-) diff --git a/contracts/Loan.sol b/contracts/Loan.sol index 5c3d2fc6..84659952 100644 --- a/contracts/Loan.sol +++ b/contracts/Loan.sol @@ -84,6 +84,14 @@ contract Loan is ILoan { _; } + /** + * @dev Modifier that can be overriden by derived classes to enforce + * access control. + */ + modifier onlyPermittedBorrower() virtual { + _; + } + /** * @dev Modifier that requires the loan not be in a terminal state. */ @@ -157,6 +165,7 @@ contract Loan is ILoan { */ function cancelCollateralized() external + onlyPermittedBorrower onlyBorrower atState(ILoanLifeCycleState.Collateralized) returns (ILoanLifeCycleState) @@ -198,8 +207,8 @@ contract Loan is ILoan { } /** - * @dev Claims specific collateral types. Can be called by the borrower (when Canceled or Matured) - * or by the PA (when Defaulted) + * @dev Claims specific collateral types. Can be called by the borrower + * (when Canceled or Matured) or by the PA (when Defaulted) */ function claimCollateral( address[] memory assets, @@ -226,8 +235,9 @@ contract Loan is ILoan { * @dev Post ERC20 tokens as collateral */ function postFungibleCollateral(address asset, uint256 amount) - public + external virtual + onlyPermittedBorrower onlyBorrower onlyNonTerminalState returns (ILoanLifeCycleState) @@ -251,8 +261,9 @@ contract Loan is ILoan { * @dev Post ERC721 tokens as collateral */ function postNonFungibleCollateral(address asset, uint256 tokenId) - public + external virtual + onlyPermittedBorrower onlyBorrower onlyNonTerminalState returns (ILoanLifeCycleState) @@ -309,8 +320,9 @@ contract Loan is ILoan { * @dev Drawdown the Loan */ function drawdown(uint256 amount) - public + external virtual + onlyPermittedBorrower onlyBorrower returns (uint256) { @@ -331,7 +343,11 @@ contract Loan is ILoan { * @dev Prepay principal. * @dev Only callable by open term loans */ - function paydownPrincipal(uint256 amount) external onlyBorrower { + function paydownPrincipal(uint256 amount) + external + onlyPermittedBorrower + onlyBorrower + { require(outstandingPrincipal >= amount, "Loan: amount too high"); require(settings.loanType == ILoanType.Open, "Loan: invalid loan type"); LoanLib.paydownPrincipal(liquidityAsset, amount, fundingVault); @@ -342,7 +358,8 @@ contract Loan is ILoan { * @dev Complete the next payment according to loan schedule inclusive of all fees. */ function completeNextPayment() - public + external + onlyPermittedBorrower onlyBorrower atState(ILoanLifeCycleState.Active) returns (uint256) @@ -401,7 +418,8 @@ contract Loan is ILoan { * @dev Complete the final payment of the loan. */ function completeFullPayment() - public + external + onlyPermittedBorrower onlyBorrower atState(ILoanLifeCycleState.Active) returns (uint256) diff --git a/contracts/permissioned/PermissionedLoan.sol b/contracts/permissioned/PermissionedLoan.sol index d383833a..7d2cea41 100644 --- a/contracts/permissioned/PermissionedLoan.sol +++ b/contracts/permissioned/PermissionedLoan.sol @@ -18,10 +18,10 @@ contract PermissionedLoan is Loan { /** * @dev a modifier to only allow valid borrowers to perform an action */ - modifier onlyValidBorrower() { + modifier onlyPermittedBorrower() override { require( poolAccessControl.isAllowed(msg.sender), - "caller is not a valid borrower" + "BORROWER_NOT_ALLOWED" ); _; } @@ -50,40 +50,4 @@ contract PermissionedLoan is Loan { { poolAccessControl = PermissionedPool(pool).poolAccessControl(); } - - /** - * @inheritdoc Loan - */ - function postFungibleCollateral(address asset, uint256 amount) - public - override - onlyValidBorrower - returns (ILoanLifeCycleState) - { - return super.postFungibleCollateral(asset, amount); - } - - /** - * @inheritdoc Loan - */ - function postNonFungibleCollateral(address asset, uint256 tokenId) - public - override - onlyValidBorrower - returns (ILoanLifeCycleState) - { - return super.postNonFungibleCollateral(asset, tokenId); - } - - /** - * @inheritdoc Loan - */ - function drawdown(uint256 amount) - public - override - onlyValidBorrower - returns (uint256) - { - return super.drawdown(amount); - } } diff --git a/contracts/permissioned/PermissionedPool.sol b/contracts/permissioned/PermissionedPool.sol index 91c65b5a..226d75d4 100644 --- a/contracts/permissioned/PermissionedPool.sol +++ b/contracts/permissioned/PermissionedPool.sol @@ -19,10 +19,7 @@ contract PermissionedPool is Pool { * @dev a modifier to only allow valid lenders to perform an action */ modifier onlyPermittedLender() override { - require( - poolAccessControl.isAllowed(msg.sender), - "caller is not a valid lender" - ); + require(poolAccessControl.isAllowed(msg.sender), "LENDER_NOT_ALLOWED"); _; } diff --git a/test/permissioned/PermissionedLoan.test.ts b/test/permissioned/PermissionedLoan.test.ts index fee598be..5c331339 100644 --- a/test/permissioned/PermissionedLoan.test.ts +++ b/test/permissioned/PermissionedLoan.test.ts @@ -186,7 +186,7 @@ describe("PermissionedLoan", () => { loan .connect(borrower) .postFungibleCollateral(liquidityAsset.address, 10) - ).to.be.revertedWith("caller is not a valid borrower"); + ).to.be.revertedWith("BORROWER_NOT_ALLOWED"); }); }); @@ -200,7 +200,7 @@ describe("PermissionedLoan", () => { loan .connect(borrower) .postNonFungibleCollateral(liquidityAsset.address, "123") - ).to.be.revertedWith("caller is not a valid borrower"); + ).to.be.revertedWith("BORROWER_NOT_ALLOWED"); }); }); @@ -240,9 +240,49 @@ describe("PermissionedLoan", () => { .connect(poolAdmin) .removeParticipant(borrower.address); await expect(loan.connect(borrower).drawdown(1)).to.be.revertedWith( - "caller is not a valid borrower" + "BORROWER_NOT_ALLOWED" ); }); }); + + describe("cancelCollateralized()", () => { + it("reverts if borrower not an allowed participant", async () => { + const { loan, borrower } = await loadFixture(loadLoanFixture); + + await expect( + loan.connect(borrower).cancelCollateralized() + ).to.be.revertedWith("BORROWER_NOT_ALLOWED"); + }); + }); + + describe("paydownPrincipal()", () => { + it("reverts if borrower not an allowed participant", async () => { + const { loan, borrower } = await loadFixture(loadLoanFixture); + + await expect( + loan.connect(borrower).paydownPrincipal(100) + ).to.be.revertedWith("BORROWER_NOT_ALLOWED"); + }); + }); + + describe("completeNextPayment()", () => { + it("reverts if borrower not an allowed participant", async () => { + const { loan, borrower } = await loadFixture(loadLoanFixture); + + await expect( + loan.connect(borrower).completeNextPayment() + ).to.be.revertedWith("BORROWER_NOT_ALLOWED"); + }); + }); + + describe("completeFullPayment()", () => { + it("reverts if borrower not an allowed participant", async () => { + const { loan, borrower } = await loadFixture(loadLoanFixture); + + await expect( + loan.connect(borrower).completeFullPayment() + ).to.be.revertedWith("BORROWER_NOT_ALLOWED"); + }); + }); }); }); diff --git a/test/permissioned/PermissionedPool.test.ts b/test/permissioned/PermissionedPool.test.ts index 7e36fc6e..1f8413ca 100644 --- a/test/permissioned/PermissionedPool.test.ts +++ b/test/permissioned/PermissionedPool.test.ts @@ -99,7 +99,7 @@ describe("PermissionedPool", () => { await expect( pool.connect(otherAccount).deposit(10, otherAccount.address) - ).to.be.revertedWith("caller is not a valid lender"); + ).to.be.revertedWith("LENDER_NOT_ALLOWED"); }); it("allows deposits if allowed lender", async () => { @@ -126,7 +126,7 @@ describe("PermissionedPool", () => { await expect( pool.connect(otherAccount).mint(10, otherAccount.address) - ).to.be.revertedWith("caller is not a valid lender"); + ).to.be.revertedWith("LENDER_NOT_ALLOWED"); }); it("allows minting if allowed lender", async () => { @@ -155,7 +155,7 @@ describe("PermissionedPool", () => { pool .connect(otherAccount) .redeem(10, otherAccount.address, otherAccount.address) - ).to.be.revertedWith("caller is not a valid lender"); + ).to.be.revertedWith("LENDER_NOT_ALLOWED"); }); it("allows redeeming if allowed lender", async () => { @@ -187,7 +187,7 @@ describe("PermissionedPool", () => { pool .connect(otherAccount) .withdraw(10, otherAccount.address, otherAccount.address) - ).to.be.revertedWith("caller is not a valid lender"); + ).to.be.revertedWith("LENDER_NOT_ALLOWED"); }); it("allows withdrawing if allowed lender", async () => {