625 feedback (#659)

* initial implementation of increase/decrease allowance

* update tests for increase/decrease

* remove unused function

* update comments

* rename lp transfer event

* additional assertions

* remove msg.sender param from ApproveLPTransfer

* rename events

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>

Author: MikeHathaway

src/PositionManager.sol

@@ -317,7 +317,7 @@ contract PositionManager is ERC721, PermitERC721, IPositionManager, Multicall, R
     /**
      *  @inheritdoc IPositionManagerOwnerActions
      *  @dev External calls to Pool contract:
-     *          - approveLpOwnership(): approve ownership for transfer
+     *          - increaseLPAllowance(): approve ownership for transfer
      *          - transferLPs(): transfer LPs ownership from PositionManager contract
      *  @dev write state:
      *          - positionIndexes: remove from bucket index
@@ -367,7 +367,7 @@ contract PositionManager is ERC721, PermitERC721, IPositionManager, Multicall, R
         address owner = ownerOf(params_.tokenId);
 
         // approve owner to take over the LPs ownership (required for transferLPs pool call)
-        pool.approveLpOwnership(owner, params_.indexes, lpAmounts);
+        pool.increaseLPAllowance(owner, params_.indexes, lpAmounts);
         // update pool lps accounting and transfer ownership of lps from PositionManager contract
         pool.transferLPs(address(this), owner, params_.indexes);
 

src/base/Pool.sol

@@ -164,41 +164,82 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
         _transferQuoteTokenFrom(msg.sender, quoteTokenAmountToAdd_);
     }
 
-    /**
-     *  @inheritdoc IPoolLenderActions
-     *  @dev write state:
-     *          - _lpAllowances mapping
-     */
-    function approveLpOwnership(
-        address newOwner_,
+    /// @inheritdoc IPoolLenderActions
+    function decreaseLPAllowance(
+        address spender_,
         uint256[] calldata indexes_,
         uint256[] calldata amounts_
     ) external override nonReentrant {
-        mapping(uint256 => uint256) storage allowances = _lpAllowances[msg.sender][newOwner_];
+        mapping(uint256 => uint256) storage allowances = _lpAllowances[msg.sender][spender_];
 
         uint256 indexesLength = indexes_.length;
         uint256 index;
 
         for (uint256 i = 0; i < indexesLength; ) {
             index = indexes_[i];
 
-            // revert if allowance at index is already set (not 0) and the new allowance does not reset the old one (not 0)
-            // this prevents possible attack where LPs receiver (newOwner) frontruns owner allowance calls to transfer more than allowed
-            if (allowances[index] != 0 && amounts_[i] != 0) revert AllowanceAlreadySet();
+            allowances[index] -= amounts_[i];
+
+            unchecked { ++i; }
+        }
+
+        emit SetLpAllowance(
+            spender_,
+            indexes_,
+            amounts_
+        );
+    }
+
+    /// @inheritdoc IPoolLenderActions
+    function increaseLPAllowance(
+        address spender_,
+        uint256[] calldata indexes_,
+        uint256[] calldata amounts_
+    ) external override nonReentrant {
+        mapping(uint256 => uint256) storage allowances = _lpAllowances[msg.sender][spender_];
+
+        uint256 indexesLength = indexes_.length;
+        uint256 index;
+
+        for (uint256 i = 0; i < indexesLength; ) {
+            index = indexes_[i];
 
-            allowances[index] = amounts_[i];
+            allowances[index] += amounts_[i];
 
             unchecked { ++i; }
         }
 
-        emit ApproveLpOwnership(
-            msg.sender,
-            newOwner_,
+        emit SetLpAllowance(
+            spender_,
             indexes_,
             amounts_
         );
     }
 
+    /// @inheritdoc IPoolLenderActions
+    function revokeLPAllowance(
+        address spender_,
+        uint256[] calldata indexes_
+    ) external override nonReentrant {
+        mapping(uint256 => uint256) storage allowances = _lpAllowances[msg.sender][spender_];
+
+        uint256 indexesLength = indexes_.length;
+        uint256 index;
+
+        for (uint256 i = 0; i < indexesLength; ) {
+            index = indexes_[i];
+
+            allowances[index] = 0;
+
+            unchecked { ++i; }
+        }
+
+        emit RevokeLpAllowance(
+            spender_,
+            indexes_
+        );
+    }
+
     /**
      *  @inheritdoc IPoolLenderActions
      *  @dev write state:
@@ -822,6 +863,15 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
         if (buckets[index_].bankruptcyTime < depositTime_) lpBalance_ = buckets[index_].lenders[lender_].lps;
     }
 
+    /// @inheritdoc IPoolState
+    function lpAllowance(
+        uint256 index_,
+        address spender_,
+        address owner_
+    ) external view override returns (uint256 allowance_) {
+        allowance_ = _lpAllowances[owner_][spender_][index_];
+    }
+
     /// @inheritdoc IPoolState
     function loanInfo(
         uint256 loanId_

src/interfaces/pool/commons/IPoolEvents.sol

@@ -23,15 +23,13 @@ interface IPoolEvents {
     );
 
     /**
-     *  @notice Emitted when lender approves a new owner of LPs at specified indexes with specified amounts.
-     *  @param  lender    Recipient that approves new owner for LPs.
-     *  @param  newOwner  Recipient approved to transfer LPs.
+     *  @notice Emitted when lender approves a spender owner of LPs at specified indexes with specified amounts.
+     *  @param  spender   Address approved to transfer LPs.
      *  @param  indexes   Bucket indexes of LPs approved.
      *  @param  amounts   LP amounts approved (ordered by approved indexes).
      */
-    event ApproveLpOwnership(
-        address indexed lender,
-        address indexed newOwner,
+    event SetLpAllowance(
+        address indexed spender,
         uint256[] indexes,
         uint256[] amounts
     );
@@ -234,6 +232,16 @@ interface IPoolEvents {
         uint256 currentBurnEpoch
     );
 
+    /**
+     *  @notice Emitted when lender removes the allowance of a spender for their LPB.
+     *  @param  spender Address that is having it's allowance revoked.
+     *  @param  indexes List of bucket index to remove the allowance from.
+     */
+    event RevokeLpAllowance(
+        address indexed spender,
+        uint256[] indexes
+    );
+
     /**
      *  @notice Emitted when lender removes addresses from the LPs transferors whitelist.
      *  @param  lender      Recipient that approves new owner for LPs.

src/interfaces/pool/commons/IPoolLenderActions.sol

@@ -20,14 +20,27 @@ interface IPoolLenderActions {
     ) external returns (uint256 lpbChange);
 
     /**
-     *  @notice Called by lenders to approve transfer of LPs to a new owner.
+     *  @notice Called by lenders to approve transfer of an amount of LPs to a new owner.
      *  @dev    Intended for use by the PositionManager contract.
-     *  @param  allowedNewOwner The new owner of the LPs.
+     *  @param  spender The new owner of the LPs.
      *  @param  indexes         Bucket indexes from where LPs are transferred.
      *  @param  amounts         The amounts of LPs approved to transfer.
      */
-    function approveLpOwnership(
-        address allowedNewOwner,
+    function increaseLPAllowance(
+        address spender,
+        uint256[] calldata indexes,
+        uint256[] calldata amounts
+    ) external;
+
+    /**
+     *  @notice Called by lenders to decrease the amount of LPs that can be spend by a new owner.
+     *  @dev    Intended for use by the PositionManager contract.
+     *  @param  spender The new owner of the LPs.
+     *  @param  indexes         Bucket indexes from where LPs are transferred.
+     *  @param  amounts         The amounts of LPs approved to transfer.
+     */
+    function decreaseLPAllowance(
+        address spender,
         uint256[] calldata indexes,
         uint256[] calldata amounts
     ) external;
@@ -91,6 +104,16 @@ interface IPoolLenderActions {
         uint256 index
     ) external returns (uint256 quoteTokenAmount, uint256 lpAmount);
 
+    /**
+     *  @notice Called by lenders to decrease the amount of LPs that can be spend by a new owner.
+     *  @param  spender Address that is having it's allowance revoked.
+     *  @param  indexes List of bucket index to remove the allowance from.
+     */
+    function revokeLPAllowance(
+        address spender,
+        uint256[] calldata indexes
+    ) external;
+
     /**
      *  @notice Called by lenders to transfers their LPs to a different address. approveLpOwnership needs to be run first
      *  @dev    Used by PositionManager.memorializePositions().

src/interfaces/pool/commons/IPoolState.sol

@@ -176,6 +176,19 @@ interface IPoolState {
             uint256 lastQuoteDeposit
     );
 
+    /**
+     *  @notice Return the LPB allowance a LP owner provided to a spender.
+     *  @param  index   Bucket index.
+     *  @param  spender Address of the LPB spender.
+     *  @param  owner   The initial owner of the LPs.
+     *  @return allowance_ Amount of LPs spender can utilize.
+     */
+    function lpAllowance(
+        uint256 index,
+        address spender,
+        address owner
+    ) external view returns (uint256 allowance_);
+
     /**
      *  @notice Returns information about a loan in the pool.
      *  @param  loanId Loan's id within loan heap. Max loan is position 1.