VAL-134 Add missing permissioning checks around the Loan (#175)

* Add failing test: Borrower Permissioning should be applied when withdrawing collateral

* Add implementation: check borrower permissioning on collateral withdrawal.

* Mediate calls to Loan.reclaimFunds() through the PoolController, allowing protocol permissioning to be applied. Update existing tests + add new tests to cover this new exposed method -  PoolController.reclaimLoanFunds.

* Pipe claimCollateral calls from PA to a loan through the PoolController layer so global permissioning can be applied.

* Pipe cancel funded loan calls from PA to a loan through the PoolController layer so global permissioning can be applied.

* Pipe markCallback() calls through the PoolController layer

* Remove unused modifier

* Rename function for clarity

* Lint, edit natspec for clarity

* Create PermissionedPoolController test file, move relevant tests

* Lint fixes

contracts/Loan.sol

@@ -85,14 +85,6 @@ contract Loan is ILoan, BeaconImplementation {
         _;
     }
 
-    modifier onlyPoolAdmin() {
-        require(
-            msg.sender == IPool(_pool).admin(),
-            "Loan: caller is not pool admin"
-        );
-        _;
-    }
-
     /**
      * @dev Modifier that can be overriden by derived classes to enforce
      * access control.
@@ -209,7 +201,8 @@ contract Loan is ILoan, BeaconImplementation {
         returns (ILoanLifeCycleState)
     {
         require(
-            msg.sender == _borrower || msg.sender == IPool(_pool).admin(),
+            msg.sender == _borrower ||
+                msg.sender == address(IPool(_pool).poolController()),
             "Loan: invalid caller"
         );
         require(
@@ -228,27 +221,56 @@ contract Loan is ILoan, BeaconImplementation {
     }
 
     /**
-     * @dev Claims specific collateral types. Can be called by the borrower
-     * (when Canceled or Matured) or by the PA (when Defaulted)
+     * @inheritdoc ILoan
      */
     function claimCollateral(
         address[] memory assets,
         ILoanNonFungibleCollateral[] memory nonFungibleAssets
     ) external override onlyNotPaused {
+        address recipient;
+        if (msg.sender == _borrower) {
+            _checkBorrowerCanWithdrawCollateral();
+            recipient = _borrower;
+        } else {
+            // Only the PA or borrower can withdraw collateral.
+            _checkAdminCanWithdrawCollateral();
+            recipient = IPool(_pool).admin();
+        }
+
+        LoanLib.withdrawFungibleCollateral(collateralVault, assets, recipient);
+        LoanLib.withdrawNonFungibleCollateral(
+            collateralVault,
+            nonFungibleAssets,
+            recipient
+        );
+    }
+
+    /**
+     * @dev Internal check that a borrower is eligible to withdraw collateral.
+     */
+    function _checkBorrowerCanWithdrawCollateral()
+        internal
+        view
+        onlyPermittedBorrower
+    {
         require(
-            (_state == ILoanLifeCycleState.Canceled &&
-                msg.sender == _borrower) ||
-                (_state == ILoanLifeCycleState.Defaulted &&
-                    msg.sender == IPool(_pool).admin()) ||
-                (_state == ILoanLifeCycleState.Matured &&
-                    msg.sender == _borrower),
+            _state == ILoanLifeCycleState.Canceled ||
+                _state == ILoanLifeCycleState.Matured,
             "Loan: unable to claim collateral"
         );
+    }
 
-        LoanLib.withdrawFungibleCollateral(collateralVault, assets);
-        LoanLib.withdrawNonFungibleCollateral(
-            collateralVault,
-            nonFungibleAssets
+    /**
+     * @dev Internal check that a PA is eligible to withdraw collateral.
+     */
+    function _checkAdminCanWithdrawCollateral()
+        internal
+        view
+        onlyPoolController
+    {
+        require(
+            _state == ILoanLifeCycleState.Defaulted,
+            "Loan: unable to claim collateral"
         );
     }
 
@@ -317,7 +339,7 @@ contract Loan is ILoan, BeaconImplementation {
     /**
      * @inheritdoc ILoan
      */
-    function reclaimFunds(uint256 amount) external onlyNotPaused onlyPoolAdmin {
+    function reclaimFunds(uint256 amount) external override onlyPoolController {
         require(settings.loanType == ILoanType.Open);
 
         fundingVault.withdrawERC20(liquidityAsset, amount, _pool);
@@ -508,7 +530,7 @@ contract Loan is ILoan, BeaconImplementation {
     /**
      * @inheritdoc ILoan
      */
-    function markCallback() external override onlyNotPaused onlyPoolAdmin {
+    function markCallback() external override onlyPoolController {
         callbackTimestamp = block.timestamp;
     }
 

contracts/controllers/PoolController.sol

@@ -512,6 +512,60 @@ contract PoolController is IPoolController, BeaconImplementation {
         );
     }
 
+    /**
+     * @inheritdoc IPoolController
+     */
+    function reclaimLoanFunds(address loan, uint256 amount)
+        external
+        override
+        onlyNotPaused
+        onlyAdmin
+        onlyPermittedAdmin
+        onlySnapshottedPool
+    {
+        ILoan(loan).reclaimFunds(amount);
+    }
+
+    /**
+     * @inheritdoc IPoolController
+     * @dev Note that the Loan enforces the non-paused state, so it's omitted here.
+     */
+    function claimLoanCollateral(
+        address loan,
+        address[] memory assets,
+        ILoanNonFungibleCollateral[] memory nonFungibleAssets
+    ) external override onlyAdmin onlyPermittedAdmin onlySnapshottedPool {
+        ILoan(loan).claimCollateral(assets, nonFungibleAssets);
+    }
+
+    /**
+     * @inheritdoc IPoolController
+     * @dev Note that the Loan enforces the non-paused state, so it's omitted here.
+     */
+    function cancelFundedLoan(address loan)
+        external
+        override
+        onlyAdmin
+        onlyPermittedAdmin
+        onlySnapshottedPool
+    {
+        ILoan(loan).cancelFunded();
+    }
+
+    /**
+     * @inheritdoc IPoolController
+     */
+    function markLoanCallback(address loan)
+        external
+        override
+        onlyNotPaused
+        onlyAdmin
+        onlyPermittedAdmin
+        onlySnapshottedPool
+    {
+        ILoan(loan).markCallback();
+    }
+
     /*//////////////////////////////////////////////////////////////
                                 Fees
     //////////////////////////////////////////////////////////////*/

contracts/controllers/interfaces/IPoolController.sol

@@ -2,6 +2,7 @@
 pragma solidity ^0.8.16;
 
 import "../../interfaces/IPool.sol";
+import "../../interfaces/ILoan.sol";
 
 /**
  * @dev Expresses the various states a pool can be in throughout its lifecycle.
@@ -214,6 +215,33 @@ interface IPoolController {
      */
     function defaultLoan(address) external;
 
+    /**
+     * @dev Called by the pool admin, this allows reclaiming loan principal funds back to the pool
+     * from open-term loans.
+     */
+    function reclaimLoanFunds(address loan, uint256 amount) external;
+
+    /**
+     * @dev Called by the pool admin, this allows claiming loan collateral
+     * back to the PA.
+     */
+    function claimLoanCollateral(
+        address loan,
+        address[] memory assets,
+        ILoanNonFungibleCollateral[] memory nonFungibleAssets
+    ) external;
+
+    /**
+     * @dev Called by the pool admin, this cancels a funded loan.
+     */
+    function cancelFundedLoan(address loan) external;
+
+    /**
+     * @dev Called by the pool admin, this marks an open-term loan as being
+     * called back.
+     */
+    function markLoanCallback(address loan) external;
+
     /*//////////////////////////////////////////////////////////////
                 Fees
     //////////////////////////////////////////////////////////////*/

contracts/libraries/LoanLib.sol

@@ -154,14 +154,15 @@ library LoanLib {
      */
     function withdrawFungibleCollateral(
         IVault collateralVault,
-        address[] memory collateralToWithdraw
+        address[] memory collateralToWithdraw,
+        address recipient
     ) external {
         for (uint256 i = 0; i < collateralToWithdraw.length; i++) {
             address asset = collateralToWithdraw[i];
 
             // Perform transfer
             uint256 amount = IERC20(asset).balanceOf(address(collateralVault));
-            collateralVault.withdrawERC20(asset, amount, msg.sender);
+            collateralVault.withdrawERC20(asset, amount, recipient);
             emit WithdrewCollateral(asset, amount);
         }
     }
@@ -171,14 +172,15 @@ library LoanLib {
      */
     function withdrawNonFungibleCollateral(
         IVault collateralVault,
-        ILoanNonFungibleCollateral[] memory collateralToWithdraw
+        ILoanNonFungibleCollateral[] memory collateralToWithdraw,
+        address recipient
     ) external {
         for (uint256 i = 0; i < collateralToWithdraw.length; i++) {
             ILoanNonFungibleCollateral memory wc = collateralToWithdraw[i];
             address asset = wc.asset;
             uint256 tokenId = wc.tokenId;
 
-            collateralVault.withdrawERC721(asset, tokenId, msg.sender);
+            collateralVault.withdrawERC721(asset, tokenId, recipient);
             emit WithdrewNonFungibleCollateral(asset, tokenId);
         }
     }