`deltaCollateralInETH` needs to be rounded up inside `calcDeltaPosition()`

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/core/hooks/UseLeverage.sol#L58

Vulnerability details

Description

The calcDeltaPosition() function rounds-down deltaDebtInETH as well as deltaCollateralInETH. These values are later deducted from the total debt & collateral respectively. This effectively means that both the remaining debt & the remaining collateral backing this debt get rounded-up. This is incorrect. While it's alright to round-up the debt as it is in the favour of the protocol, the remaining collateral backing the debt should always be rounded-down. Hence, deltaCollateralInETH should have been rounded-up.

    function calcDeltaPosition(
        uint256 percentageToBurn,
        uint256 totalCollateralBaseInEth,
        uint256 totalDebtBaseInEth
    ) public pure returns (uint256 deltaCollateralInETH, uint256 deltaDebtInETH) {
        if (percentageToBurn == 0 || percentageToBurn > PERCENTAGE_PRECISION) {
            revert InvalidPercentageValue();
        }
        // Reduce Collateral based on the percentage to Burn
        deltaDebtInETH = (totalDebtBaseInEth * percentageToBurn) / PERCENTAGE_PRECISION; 
        // Reduce Debt based on the percentage to Burn
@--->   deltaCollateralInETH = (totalCollateralBaseInEth * percentageToBurn) / PERCENTAGE_PRECISION;  // @audit : Should have been rounded-up.
    }

Impact

The collateral ratio can be skewed and can dip below acceptable ratio.

Tools Used

Manual review

Recommended Mitigation Steps

Round up deltaCollateralInETH. A library like solmate can be used which has mulDivUp:

-   deltaCollateralInETH = (totalCollateralBaseInEth * percentageToBurn) / PERCENTAGE_PRECISION;
+   deltaCollateralInETH = totalCollateralBaseInEth.mulDivUp(percentageToBurn, PERCENTAGE_PRECISION);

Assessed type

Math

[stalinMacias]

I'd like to add my take on the problem reported on this issue.

Rounding down deltaCollateralInETH is correct, because if deltaCollateralInETH were rounded upthat would cause the withdrawer to end up receiving more funds than it should, and all those extra funds would be the loss of the rest of the depositors.

By rounding up the deltaCollateralInETH would cause that the strategy withdraws more collateral than what it should have withdrawn to process the undeploy request.

• By rounding up deltaCollateralInETH, the withdrawer gets benefitted at the expense of the rest of the withdrawers because deltaCollateralInETH determines the total amount of collateral that is withdrawn from Aave, so, if deltaCollateralInETH is rounded up, the withdrawer will receive more while the loss caused for rounding up is socialized among the rest of the depositors.

If we follow the execution flow, we'll notice that deltaCollateralInETH is the data.originalAmount on the onFlashLoan() that is called after the Flashloan is requested....

• This value is the withdrawAmountInETh parameter received on the _repayAndWithdraw() , which is the exact same value used to withdraw the collateral from Aave, as such, rounding up, as this report suggests, would cause that the rest of depositors gets impacted by the extra funds withdrawn for the current withdrawer.

> StrategyLeverage.sol
function _repayAndWithdraw(
  uint256 withdrawAmountInETh,
  ...
) internal {

  (uint256 collateralBalance,) = _getMMPosition();
  uint256 convertedAmount = _fromWETH(withdrawAmountInETh);
  uint256 withdrawAmount = collateralBalance > convertedAmount ? convertedAmount : collateralBalance;

  _repay(wETHA(), repayAmount);                        

  //@audit => rounding up the amount of collateral that will be withdrawn affects the rest of the depositors.

  _withdraw(ierc20A(), withdrawAmount, address(this));

 ...
}

All in all, this report is trying to point out an issue where there is none. I think it would be beneficial for all parties to hear from the reporter how his suggestion of rounding up the deltaCollateralInETH would benefit the protocol instead of causing harm to the depositors?

[c4-judge]

0xleastwood marked the issue as primary issue

[0xleastwood]

I'd like to add my take on the problem reported on this issue.

Rounding down deltaCollateralInETH is correct, because if deltaCollateralInETH were rounded upthat would cause the withdrawer to end up receiving more funds than it should, and all those extra funds would be the loss of the rest of the depositors.

By rounding up the deltaCollateralInETH would cause that the strategy withdraws more collateral than what it should have withdrawn to process the undeploy request.

• By rounding up deltaCollateralInETH, the withdrawer gets benefitted at the expense of the rest of the withdrawers because deltaCollateralInETH determines the total amount of collateral that is withdrawn from Aave, so, if deltaCollateralInETH is rounded up, the withdrawer will receive more while the loss caused for rounding up is socialized among the rest of the depositors.

If we follow the execution flow, we'll notice that deltaCollateralInETH is the data.originalAmount on the onFlashLoan() that is called after the Flashloan is requested....

• This value is the withdrawAmountInETh parameter received on the _repayAndWithdraw() , which is the exact same value used to withdraw the collateral from Aave, as such, rounding up, as this report suggests, would cause that the rest of depositors gets impacted by the extra funds withdrawn for the current withdrawer.

> StrategyLeverage.sol
function _repayAndWithdraw(
  uint256 withdrawAmountInETh,
  ...
) internal {

  (uint256 collateralBalance,) = _getMMPosition();
  uint256 convertedAmount = _fromWETH(withdrawAmountInETh);
  uint256 withdrawAmount = collateralBalance > convertedAmount ? convertedAmount : collateralBalance;

  _repay(wETHA(), repayAmount);                        

  //@audit => rounding up the amount of collateral that will be withdrawn affects the rest of the depositors.

  _withdraw(ierc20A(), withdrawAmount, address(this));

 ...
}

All in all, this report is trying to point out an issue where there is none. I think it would be beneficial for all parties to hear from the reporter how his suggestion of rounding up the deltaCollateralInETH would benefit the protocol instead of causing harm to the depositors?

I tend to agree with this logic and will close this unless the warden can elaborate otherwise.

[c4-judge]

0xleastwood marked the issue as unsatisfactory: Insufficient proof