FIX: properly handle donated assets

contracts/IdleCDO.sol

@@ -531,7 +531,7 @@ contract IdleCDO is PausableUpgradeable, GuardedLaunchUpgradable, IdleCDOStorage
 
     // send underlying to msg.sender. Keep this at the end of the function to avoid 
     // potential read only reentrancy on cdo variants that have hooks (eg with nfts)
-    IERC20Detailed(_token).safeTransfer(msg.sender, toRedeem);
+    _transferUnderlyings(msg.sender, toRedeem);
   }
 
   /// @notice updates trancheAPRSplitRatio based on the current tranches TVL ratio between AA and BB
@@ -713,7 +713,7 @@ contract IdleCDO is PausableUpgradeable, GuardedLaunchUpgradable, IdleCDOStorage
   /// @param _AATrancheSplitRatio AA split ratio used for calculations
   /// @return apr for the specific tranche
   function _getApr(address _tranche, uint256 _AATrancheSplitRatio) internal view returns (uint256) {
-    uint256 stratApr = IIdleCDOStrategy(strategy).getApr();
+    uint256 stratApr = _getStrategyApr();
     uint256 _trancheAPRSplitRatio = trancheAPRSplitRatio;
     bool isAATranche = _tranche == AATranche;
     if (_AATrancheSplitRatio == 0) {
@@ -837,8 +837,7 @@ contract IdleCDO is PausableUpgradeable, GuardedLaunchUpgradable, IdleCDOStorage
   /// @param _maxDecreaseDefault max value, in % where `100000` = 100%, of accettable price decrease for the strategy
   function setMaxDecreaseDefault(uint256 _maxDecreaseDefault) external virtual {
     _checkOnlyOwner();
-    require(_maxDecreaseDefault < FULL_ALLOC, '7');
-    maxDecreaseDefault = _maxDecreaseDefault;
+    require((maxDecreaseDefault = _maxDecreaseDefault) < FULL_ALLOC, '7');
   }
 
   /// @param _active flag to allow Adaptive Yield Split
@@ -872,7 +871,7 @@ contract IdleCDO is PausableUpgradeable, GuardedLaunchUpgradable, IdleCDOStorage
   }
 
   /// @param _rebalancer new rebalancer address
-  function setRebalancer(address _rebalancer) external {
+  function setRebalancer(address _rebalancer) external virtual {
     _checkOnlyOwner();
     require((rebalancer = _rebalancer) != address(0), '0');
   }
@@ -905,7 +904,6 @@ contract IdleCDO is PausableUpgradeable, GuardedLaunchUpgradable, IdleCDOStorage
   function setMinAprSplitAYS(uint256 _aprSplit) external {
     _checkOnlyOwner();
     require((minAprSplitAYS = _aprSplit) <= FULL_ALLOC, '7');
-    minAprSplitAYS = _aprSplit;
   }
 
   /// @param _fee new fee
@@ -1057,6 +1055,18 @@ contract IdleCDO is PausableUpgradeable, GuardedLaunchUpgradable, IdleCDOStorage
     require(keccak256(abi.encodePacked(tx.origin, block.number)) != _lastCallerBlock, "8");
   }
 
+  /// @dev transfer underlyings to a specific address
+  /// @param _to receiver address
+  /// @param _amount amount to transfer
+  function _transferUnderlyings(address _to, uint256 _amount) internal {
+    IERC20Detailed(token).safeTransfer(_to, _amount);
+  }
+
+  /// @dev Get the current strategy apr
+  function _getStrategyApr() internal view returns (uint256) {
+    return IIdleCDOStrategy(strategy).getApr();
+  }
+
   /// @notice concat 2 strings in a single one
   /// @param a first string
   /// @param b second string

contracts/IdleCDOEpochVariant.sol

@@ -91,7 +91,7 @@ contract IdleCDOEpochVariant is IdleCDO {
     disableInstantWithdraw = true;
 
     // scale the apr to include the buffer period
-    _setScaledApr(IdleCreditVault(strategy).getApr());
+    _setScaledApr(_getStrategyApr());
   }
 
   /// @notice Check if msg sender is owner or manager
@@ -217,7 +217,7 @@ contract IdleCDOEpochVariant is IdleCDO {
      if (msg.sender != address(this)) {
       revert NotAllowed();
     }
-    IERC20Detailed(token).safeTransfer(IdleCreditVault(strategy).borrower(), _amount);
+    _transferUnderlyings(IdleCreditVault(strategy).borrower(), _amount);
   }
 
   /// @notice Stop epoch, accrue interest to the vault and get funds to fullfill normal
@@ -271,22 +271,22 @@ contract IdleCDOEpochVariant is IdleCDO {
       // Transfer pending withdraw fees to feeReceiver before update accounting
       // NOTE: Fees are sent with 2 different transfer calls, here and after updateAccounting, to avoid complicated calculations
       if (_pendingWithdrawFees > 0) {
-        IERC20Detailed(token).safeTransfer(feeReceiver, _pendingWithdrawFees);
+        _transferUnderlyings(feeReceiver, _pendingWithdrawFees);
       }
 
       if (_isRequestingAllFunds) {
         // we already have strategyTokens equal to _totBorrowed in this contract
         // so we simply transfer _totBorrowed to the strategy to avoid double counting
         // for getContractValue
-        IERC20Detailed(token).safeTransfer(address(_strategy), _totBorrowed);
+        _transferUnderlyings(address(_strategy), _totBorrowed);
       }
 
       // update tranche prices and unclaimed fees
       _updateAccounting();
 
       // transfer fees
       uint256 _fees = unclaimedFees;
-      IERC20Detailed(token).safeTransfer(feeReceiver, _fees);
+      _transferUnderlyings(feeReceiver, _fees);
       unclaimedFees = 0;
 
       // save net gain (this does not include interest gained for pending withdrawals)
@@ -467,6 +467,10 @@ contract IdleCDOEpochVariant is IdleCDO {
     if (!isWalletAllowed(msg.sender)) {
       revert NotAllowed();
     }
+
+    // we check if there are donated assets to the pool and transfer them to the feeReceiver if any
+    _skimDonatedAssets();
+    // do the inherited deposit flow
     return super._deposit(_amount, _tranche, _referral);
   }
 
@@ -486,6 +490,9 @@ contract IdleCDOEpochVariant is IdleCDO {
       revert NotAllowed();
     }
 
+    // we check if there are donated assets to the pool and transfer them to the feeReceiver if any
+    _skimDonatedAssets();
+
     // we trigger an update accounting to check for eventual losses
     _updateAccounting();
 
@@ -533,6 +540,15 @@ contract IdleCDOEpochVariant is IdleCDO {
     return _underlyings;
   }
 
+  /// @notice Transfer donated assets to the feeReceiver
+  function _skimDonatedAssets() internal {
+    address _token = token;
+    uint256 _underlyings = _contractTokenBalance(_token);
+    if (_underlyings > 0) {
+      IERC20Detailed(_token).transfer(feeReceiver, _underlyings);
+    }
+  }
+
   /// @notice Get the tranche apr split ratio
   /// @param _tranche address
   /// @return _aprRatio apr split ratio for the tranche
@@ -543,7 +559,7 @@ contract IdleCDOEpochVariant is IdleCDO {
   /// @notice Calculate the interest of an epoch for the given amount
   /// @param _amount Amount of underlyings
   function _calcInterest(uint256 _amount) internal view returns (uint256) {
-    return _amount * (IdleCreditVault(strategy).getApr() / 100) * epochDuration / (365 days * ONE_TRANCHE_TOKEN);
+    return _amount * (_getStrategyApr() / 100) * epochDuration / (365 days * ONE_TRANCHE_TOKEN);
   }
 
   /// @notice Calculate the interest of an epoch for a withdraw request
@@ -675,4 +691,7 @@ contract IdleCDOEpochVariant is IdleCDO {
 
   /// NOTE: the vault is either a single tranche (ie all interest to senior and set in additionalInit) or AYS is active
   function setTrancheAPRSplitRatio(uint256) external override {}
+
+  /// NOTE: rebalancer is not used
+  function setRebalancer(address _rebalancer) external override {}
 }

test/foundry/IdleCreditVault.t.sol

@@ -2111,4 +2111,63 @@ contract TestIdleCreditVault is TestIdleCDOLossMgmt {
     cdoEpoch.claimWithdrawRequest();
     vm.stopPrank();
   }
+
+  function testPocWithdrawDos() external {
+    // We check if donating assets to the pool causes the last withdrawer to be unable to withdraw
+    // due to not minting strategyTokens on donations. Sherlock audit finding
+    address alice = vm.addr(0x1);
+    address bob = vm.addr(0x2);
+    address _strategyToken = cdoEpoch.strategy();
+
+    uint256 initialAlice = 10000e6;
+    uint256 initialBob = 1000e6;
+    deal(defaultUnderlying, alice, initialAlice);
+    deal(defaultUnderlying, bob, initialBob);
+    // to pay for interest
+    deal(defaultUnderlying, borrower, 200000e6);
+
+    // Alice deposit 9000 USDC.
+    vm.startPrank(alice);
+    IERC20Detailed(defaultUnderlying).approve(address(idleCDO), type(uint256).max);
+    idleCDO.depositAA(9000e6);
+    vm.stopPrank();
+
+    // Bob deposit 1000 USDC
+    vm.startPrank(bob);
+    IERC20Detailed(defaultUnderlying).approve(address(idleCDO), type(uint256).max);
+    idleCDO.depositAA(1000e6);
+    vm.stopPrank();
+
+    // start epoch
+    _toggleEpoch(true, 0, 0);
+
+    // close pool
+    vm.warp(cdoEpoch.epochEndDate() + 1);
+    vm.startPrank(manager);
+    cdoEpoch.stopEpoch(0, 1);
+    vm.stopPrank();
+
+    vm.startPrank(alice);
+    // donate 1000 USDC to the contract
+    IERC20Detailed(defaultUnderlying).transfer(address(cdoEpoch), 1000e6);
+    // request withdraw for the max amount before the tx reverts
+    cdoEpoch.requestWithdraw(0, cdoEpoch.AATranche());
+    cdoEpoch.claimWithdrawRequest();
+    uint256 afterBalance = IERC20Detailed(defaultUnderlying).balanceOf(alice);
+    vm.stopPrank();
+
+    // Alice will lose money because she donated assets
+    assertLt(afterBalance, initialAlice, 'Alice balance increased');
+
+    // request withdraw for all and claim right away given that pool is closed
+    vm.startPrank(bob);
+    cdoEpoch.requestWithdraw(0, cdoEpoch.AATranche());
+    cdoEpoch.claimWithdrawRequest();
+    afterBalance = IERC20Detailed(defaultUnderlying).balanceOf(bob);
+    _strategyToken = cdoEpoch.strategy();
+    vm.stopPrank();
+
+    // Bob is unaffected by the donation
+    assertGt(afterBalance, initialBob, 'Bob balance decreased');
+  }
 }