fix: Bailsec Core audit remediation (breaking: redemption ABI)

contracts/helpers/BaseHarvester.sol → contracts/helpers/BaseRebalancer.sol

@@ -6,7 +6,7 @@ import { AccessManaged } from "../utils/AccessManaged.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import { IParallelizer } from "contracts/interfaces/IParallelizer.sol";
 import { ITokenP } from "contracts/interfaces/ITokenP.sol";
-import "contracts/interfaces/IHarvester.sol";
+import "contracts/interfaces/IRebalancer.sol";
 
 import "../utils/Errors.sol";
 
@@ -22,17 +22,18 @@ struct YieldBearingParams {
   // Whether limit exposures should be overriden or read onchain through the Parallelizer
   // This value should be 1 to override exposures or 2 if these shouldn't be overriden
   uint64 overrideExposures;
-  // Maximum slippage when dealing with the Parallelizer
-  uint96 maxSlippage;
+  // Maximum slippage when dealing with the Parallelizer, bounded below 1e9 so uint64 leaves the
+  // struct packed in two slots
+  uint64 maxSlippage;
 }
 
-/// @title BaseHarvester
+/// @title BaseRebalancer
 /// @author Cooper Labs
 /// @custom:contact security@cooperlabs.xyz
-/// @dev Abstract contract for a harvester that aims at rebalancing a Parallelizer
-/// @dev This contract is an authorized fork of Angle's BaseHarvester contract:
+/// @dev Abstract contract for a rebalancer that aims at rebalancing a Parallelizer
+/// @dev This contract is an authorized fork of Angle's BaseHarvester contract, substantially modified:
 /// https://github.com/AngleProtocol/angle-transmuter/blob/main/contracts/helpers/BaseHarvester.sol
-abstract contract BaseHarvester is IHarvester, AccessManaged {
+abstract contract BaseRebalancer is IRebalancer, AccessManaged {
   using SafeERC20 for IERC20;
 
   /*//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -67,8 +68,6 @@ abstract contract BaseHarvester is IHarvester, AccessManaged {
   IParallelizer public immutable parallelizer;
   /// @notice TokenP handled by the `parallelizer` of interest
   ITokenP public immutable tokenP;
-  /// @notice Max slippage when dealing with the Parallelizer
-  mapping(address => uint96) public maxTokenSlippage;
   /// @notice Data associated to a yield bearing asset
   mapping(address => YieldBearingParams) public yieldBearingData;
   /// @notice trusted addresses that can update target exposure and do others non critical operations
@@ -80,6 +79,7 @@ abstract contract BaseHarvester is IHarvester, AccessManaged {
 
   event Recovered(address token, uint256 amount, address to);
   event TrustedToggled(address trusted, bool status);
+  event AllowanceReset(address indexed token, address indexed spender);
 
   /*//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
                                                        CONSTRUCTOR
@@ -116,7 +116,7 @@ abstract contract BaseHarvester is IHarvester, AccessManaged {
     uint64 minExposure,
     uint64 maxExposure,
     uint64 overrideExposures,
-    uint96 maxSlippage
+    uint64 maxSlippage
   )
     external
     restricted
@@ -141,7 +141,7 @@ abstract contract BaseHarvester is IHarvester, AccessManaged {
    * @notice Set the max allowed slippage
    * @param newMaxSlippage new max allowed slippage
    */
-  function setMaxSlippage(address yieldBearingAsset, uint96 newMaxSlippage) external restricted {
+  function setMaxSlippage(address yieldBearingAsset, uint64 newMaxSlippage) external restricted {
     _setMaxSlippage(yieldBearingAsset, newMaxSlippage);
   }
 
@@ -165,6 +165,19 @@ abstract contract BaseHarvester is IHarvester, AccessManaged {
     IERC20(tokenAddress).safeTransfer(to, amountToRecover);
   }
 
+  /**
+   * @notice Set an allowance this contract granted back to zero
+   * @param token address of the token
+   * @param spender address losing the allowance
+   * @dev Rebalancing grants unlimited allowances that are never reduced. Rotating a yield bearing
+   * asset's configuration revokes the approvals it created, but this covers a spender that became
+   * untrusted for any other reason.
+   */
+  function resetAllowance(address token, address spender) external restricted {
+    IERC20(token).forceApprove(spender, 0);
+    emit AllowanceReset(token, spender);
+  }
+
   /*//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
                                                         TRUSTED FUNCTIONS
   //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/
@@ -240,12 +253,20 @@ abstract contract BaseHarvester is IHarvester, AccessManaged {
     uint64 minExposure,
     uint64 maxExposure,
     uint64 overrideExposures,
-    uint96 maxSlippage
+    uint64 maxSlippage
   )
     internal
     virtual
   {
     YieldBearingParams storage yieldBearingInfo = yieldBearingData[yieldBearingAsset];
+    address previousAsset = yieldBearingInfo.asset;
+    // Allowances are granted without expiry, so the outgoing asset would keep the yield bearing
+    // vault authorised over any of it later held here
+    if (previousAsset != address(0) && previousAsset != asset) {
+      IERC20(previousAsset).forceApprove(yieldBearingAsset, 0);
+      emit AllowanceReset(previousAsset, yieldBearingAsset);
+      _revokeAssetAllowances(yieldBearingAsset, previousAsset);
+    }
     yieldBearingInfo.asset = asset;
     if (targetExposure >= 1e9) revert InvalidParam();
     if (maxSlippage >= 1e9) revert InvalidParam();
@@ -262,6 +283,9 @@ abstract contract BaseHarvester is IHarvester, AccessManaged {
     }
   }
 
+  /// @notice Lets a rebalancer revoke the allowances it granted over an asset being rotated out
+  function _revokeAssetAllowances(address yieldBearingAsset, address previousAsset) internal virtual { }
+
   function _updateLimitExposuresYieldAsset(
     address asset,
     YieldBearingParams storage yieldBearingInfo
@@ -282,9 +306,9 @@ abstract contract BaseHarvester is IHarvester, AccessManaged {
     else yieldBearingInfo.minExposure = xFeeBurn[length - 2];
   }
 
-  function _setMaxSlippage(address yieldBearingAsset, uint96 newMaxSlippage) internal virtual {
-    if (newMaxSlippage > 1e9) revert InvalidParam();
-    maxTokenSlippage[yieldBearingAsset] = newMaxSlippage;
+  function _setMaxSlippage(address yieldBearingAsset, uint64 newMaxSlippage) internal virtual {
+    if (newMaxSlippage >= 1e9) revert InvalidParam();
+    yieldBearingData[yieldBearingAsset].maxSlippage = newMaxSlippage;
   }
 
   function _scaleAmountBasedOnDecimals(

contracts/helpers/GenericHarvester.sol → contracts/helpers/GenericRebalancer.sol

@@ -15,20 +15,20 @@ import { IERC4626 } from "contracts/interfaces/external/IERC4626.sol";
 
 import "../utils/Errors.sol";
 
-import { BaseHarvester, YieldBearingParams } from "./BaseHarvester.sol";
+import { BaseRebalancer, YieldBearingParams } from "./BaseRebalancer.sol";
 
 enum SwapType {
   VAULT,
   SWAP
 }
 
-/// @title GenericHarvester
+/// @title GenericRebalancer
 /// @author Cooper Labs
 /// @custom:contact security@cooperlabs.xyz
-/// @dev Generic contract for anyone to permissionlessly adjust the reserves of Angle Parallelizer
-/// @dev This contract is an authorized fork of Angle's GenericHarvester contract:
+/// @dev Generic contract for trusted callers to adjust the reserves of Angle Parallelizer
+/// @dev This contract is an authorized fork of Angle's GenericHarvester contract, substantially modified:
 /// https://github.com/AngleProtocol/angle-transmuter/blob/main/contracts/helpers/GenericHarvester.sol
-contract GenericHarvester is BaseHarvester, IERC3156FlashBorrower, RouterSwapper {
+contract GenericRebalancer is BaseRebalancer, IERC3156FlashBorrower, RouterSwapper {
   using SafeCast for uint256;
   using SafeERC20 for IERC20;
 
@@ -52,7 +52,7 @@ contract GenericHarvester is BaseHarvester, IERC3156FlashBorrower, RouterSwapper
     IERC3156FlashLender definitiveFlashloan
   )
     RouterSwapper(initialSwapRouter, initialTokenTransferAddress)
-    BaseHarvester(initialAuthority, definitivetokenP, definitiveParallelizer)
+    BaseRebalancer(initialAuthority, definitivetokenP, definitiveParallelizer)
   {
     if (address(definitiveFlashloan) == address(0)) revert ZeroAddress();
     flashloan = definitiveFlashloan;
@@ -98,7 +98,9 @@ contract GenericHarvester is BaseHarvester, IERC3156FlashBorrower, RouterSwapper
   /// `yieldBearingAsset` to the target exposure
   /// @dev scale is a number between 0 and 1e9 that represents the proportion of the tokenP to harvest,
   /// it is used to lower the amount of the asset to harvest for example to have a lower slippage
-  function harvest(address yieldBearingAsset, uint256 scale, bytes calldata extraData) public virtual {
+  /// @dev Restricted to trusted callers: the flow forwards caller-supplied router calldata and refreshes the
+  /// collateral oracle, neither of which can be safely exposed to arbitrary callers
+  function harvest(address yieldBearingAsset, uint256 scale, bytes calldata extraData) public virtual onlyTrusted {
     if (scale > 1e9) revert InvalidParam();
     updateLimitExposuresYieldAsset(yieldBearingAsset);
     YieldBearingParams memory yieldBearingInfo = yieldBearingData[yieldBearingAsset];
@@ -119,6 +121,8 @@ contract GenericHarvester is BaseHarvester, IERC3156FlashBorrower, RouterSwapper
   }
 
   /// @inheritdoc IERC3156FlashBorrower
+  /// @dev A non-zero lender fee is charged to the budget of the address that called `harvest`, alongside any
+  /// shortfall between the principal and the tokenP the rebalance minted back
   function onFlashLoan(
     address initiator,
     address,
@@ -130,7 +134,7 @@ contract GenericHarvester is BaseHarvester, IERC3156FlashBorrower, RouterSwapper
     virtual
     returns (bytes32)
   {
-    if (msg.sender != address(flashloan) || initiator != address(this) || fee != 0) revert NotTrusted();
+    if (msg.sender != address(flashloan) || initiator != address(this)) revert NotTrusted();
     address sender;
     uint256 typeAction;
     uint256 minAmountOut;
@@ -153,17 +157,26 @@ contract GenericHarvester is BaseHarvester, IERC3156FlashBorrower, RouterSwapper
         tokenOut = asset;
       }
     }
+    uint256 tokenInBalanceBefore = IERC20(tokenIn).balanceOf(address(this));
+
     uint256 amountOut =
       parallelizer.swapExactInput(amount, 0, address(tokenP), tokenIn, address(this), block.timestamp);
+    // The principal is spent, what the lender will pull back is the principal plus its fee
+    amount += fee;
 
-    // Swap to tokenIn
+    // Swap to tokenOut
     amountOut = _swapToTokenOut(typeAction, tokenIn, tokenOut, amountOut, swapType, callData);
 
     _adjustAllowance(tokenOut, address(parallelizer), amountOut);
     uint256 amountStableOut =
       parallelizer.swapExactInput(amountOut, minAmountOut, tokenOut, address(tokenP), address(this), block.timestamp);
+
+    amountStableOut += _recoverResidualInput(tokenIn, tokenInBalanceBefore);
+
     if (amount > amountStableOut) {
       budget[sender] -= amount - amountStableOut; // Will revert if not enough funds
+    } else if (amountStableOut > amount) {
+      budget[sender] += amountStableOut - amount;
     }
     return CALLBACK_SUCCESS;
   }
@@ -217,6 +230,24 @@ contract GenericHarvester is BaseHarvester, IERC3156FlashBorrower, RouterSwapper
     );
   }
 
+  /**
+   * @dev Returns any input the route did not consume to the Parallelizer
+   * @param tokenIn address of the token offered to the route
+   * @param balanceBefore balance of `tokenIn` held before the operation
+   * @return the tokenP obtained from the remainder, zero when the route consumed everything
+   *
+   * Router calldata is built off chain while the input is sized on chain, so a route can consume
+   * less than was offered. Returning the remainder keeps it inside the Parallelizer's accounting
+   * instead of stranding it here, and it settles with the rest of the operation.
+   */
+  function _recoverResidualInput(address tokenIn, uint256 balanceBefore) internal returns (uint256) {
+    uint256 balanceAfter = IERC20(tokenIn).balanceOf(address(this));
+    if (balanceAfter <= balanceBefore) return 0;
+    uint256 residual = balanceAfter - balanceBefore;
+    _adjustAllowance(tokenIn, address(parallelizer), residual);
+    return parallelizer.swapExactInput(residual, 0, tokenIn, address(tokenP), address(this), block.timestamp);
+  }
+
   function _swapToTokenOut(
     uint256 typeAction,
     address tokenIn,
@@ -260,6 +291,8 @@ contract GenericHarvester is BaseHarvester, IERC3156FlashBorrower, RouterSwapper
     uint256[] memory amounts = new uint256[](1);
     amounts[0] = amount;
     _swap(tokens, callDatas, amounts);
+    // A route that consumed less than offered would otherwise leave the difference approved
+    IERC20(tokenIn).forceApprove(tokenTransferAddress, 0);
 
     return IERC20(tokenOut).balanceOf(address(this)) - balance;
   }

contracts/helpers/MultiBlockHarvester.sol → contracts/helpers/MultiBlockRebalancer.sol

@@ -5,21 +5,21 @@ import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/I
 import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import { Math } from "@openzeppelin/contracts/utils/math/Math.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
-import { BaseHarvester, YieldBearingParams } from "./BaseHarvester.sol";
+import { BaseRebalancer, YieldBearingParams } from "./BaseRebalancer.sol";
 import { IParallelizer } from "contracts/interfaces/IParallelizer.sol";
 import { ITokenP } from "contracts/interfaces/ITokenP.sol";
 import { IPool } from "contracts/interfaces/IPool.sol";
 
 import "../utils/Errors.sol";
 import "../utils/Constants.sol";
 
-/// @title MultiBlockHarvester
+/// @title MultiBlockRebalancer
 /// @author Cooper Labs
 /// @custom:contact security@cooperlabs.xyz
 /// @dev Contract to harvest yield from multiple yield bearing assets in multiple blocks transactions
 /// @dev This contract is an authorized fork of Angle's MultiBlockHarvester contract:
 /// https://github.com/AngleProtocol/angle-transmuter/blob/main/contracts/helpers/MultiBlockHarvester.sol
-contract MultiBlockHarvester is BaseHarvester {
+contract MultiBlockRebalancer is BaseRebalancer {
   using SafeERC20 for IERC20;
   using Math for uint256;
 
@@ -39,7 +39,7 @@ contract MultiBlockHarvester is BaseHarvester {
     ITokenP definitivetokenP,
     IParallelizer definitiveParallelizer
   )
-    BaseHarvester(initialAuthority, definitivetokenP, definitiveParallelizer)
+    BaseRebalancer(initialAuthority, definitivetokenP, definitiveParallelizer)
   { }
 
   /*//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -52,9 +52,25 @@ contract MultiBlockHarvester is BaseHarvester {
    * @param newDepositAddress address to deposit to receive yieldBearingAsset
    */
   function setYieldBearingToDepositAddress(address yieldBearingAsset, address newDepositAddress) external restricted {
+    address previousDepositAddress = yieldBearingToDepositAddress[yieldBearingAsset];
+    address asset = yieldBearingData[yieldBearingAsset].asset;
+    // The outgoing deposit address would otherwise keep its allowance over the underlying asset
+    if (previousDepositAddress != address(0) && previousDepositAddress != newDepositAddress && asset != address(0)) {
+      IERC20(asset).forceApprove(previousDepositAddress, 0);
+      emit AllowanceReset(asset, previousDepositAddress);
+    }
     yieldBearingToDepositAddress[yieldBearingAsset] = newDepositAddress;
   }
 
+  /// @inheritdoc BaseRebalancer
+  function _revokeAssetAllowances(address yieldBearingAsset, address previousAsset) internal override {
+    address depositAddress = yieldBearingToDepositAddress[yieldBearingAsset];
+    if (depositAddress != address(0)) {
+      IERC20(previousAsset).forceApprove(depositAddress, 0);
+      emit AllowanceReset(previousAsset, depositAddress);
+    }
+  }
+
   /*//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
                                                         TRUSTED FUNCTIONS
   //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/
@@ -141,7 +157,7 @@ contract MultiBlockHarvester is BaseHarvester {
     address asset,
     address depositAddress,
     bool assetIn,
-    uint96 maxSlippage
+    uint64 maxSlippage
   )
     internal
     view

contracts/interfaces/IGetters.sol

@@ -144,6 +144,9 @@ interface IGetters {
 
   /// @notice Returns the minimum collateral ratio that must hold after surplus processing
   /// @dev Expressed in base 1e9, and always >= 1e9
+  /// @notice Returns the aggregator or DEX the `RewardHandler` forwards its swap payload to
+  function getSwapRouter() external view returns (address);
+
   function getSurplusBufferRatio() external view returns (uint64);
 
   /// @notice Computes the surplus of a collateral.

contracts/interfaces/IHarvester.sol → contracts/interfaces/IRebalancer.sol

@@ -1,26 +1,26 @@
 // SPDX-License-Identifier: GPL-3.0
 pragma solidity 0.8.28;
 
-/// @title IHarvester
+/// @title IRebalancer
 /// @author Cooper Labs
 /// @custom:contact security@cooperlabs.xyz
 /// @dev This interface is an authorized fork of Angle's `IHarvester` interface
 /// https://github.com/AngleProtocol/angle-transmuter/blob/main/contracts/interfaces/IHarvester.sol
-interface IHarvester {
+interface IRebalancer {
   function setYieldBearingAssetData(
     address yieldBearingAsset,
     address stablecoin,
     uint64 targetExposure,
     uint64 minExposureYieldAsset,
     uint64 maxExposureYieldAsset,
     uint64 overrideExposures,
-    uint96 maxSlippage
+    uint64 maxSlippage
   )
     external;
 
   function updateLimitExposuresYieldAsset(address yieldBearingAsset) external;
 
-  function setMaxSlippage(address yieldBearingAsset, uint96 newMaxSlippage) external;
+  function setMaxSlippage(address yieldBearingAsset, uint64 newMaxSlippage) external;
 
   function harvest(address yieldBearingAsset, uint256 scale, bytes calldata extraData) external;
 }