Transaction revert in division by zero error when handling protocol fee if the feeTo address is set but s.protocolFeeDenominator is not set

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L654
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L650
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L303

Vulnerability details

Impact

Transaction revert in division by zero error when handling protocol fee if the feeTo address is set but s.protocolFeeDenominator is not set

Proof of Concept

In the VaultImplementation, function commitToLien is called for lifecycle of new loan origination.

  function commitToLien(
    IAstariaRouter.Commitment calldata params,
    address receiver
  )
    external
    whenNotPaused
    returns (uint256 lienId, ILienToken.Stack[] memory stack, uint256 payout)
  {
    _beforeCommitToLien(params);
    uint256 slopeAddition;
    (lienId, stack, slopeAddition, payout) = _requestLienAndIssuePayout(
      params,
      receiver
    );
    _afterCommitToLien(
      stack[stack.length - 1].point.end,
      lienId,
      slopeAddition
    );
  }

which calls:

(lienId, stack, slopeAddition, payout) = _requestLienAndIssuePayout(
  params,
  receiver
);

which calls:

  function _requestLienAndIssuePayout(
    IAstariaRouter.Commitment calldata c,
    address receiver
  )
    internal
    returns (
      uint256 newLienId,
      ILienToken.Stack[] memory stack,
      uint256 slope,
      uint256 payout
    )
  {
    _validateCommitment(c, receiver);
    (newLienId, stack, slope) = ROUTER().requestLienPosition(c, recipient());
    payout = _handleProtocolFee(c.lienRequest.amount);
    ERC20(asset()).safeTransfer(receiver, payout);
  }

which calls:

_handleProtocolFee(c.lienRequest.amount);

function _handleProtocolFee(uint256 amount) internal returns (uint256) {
address feeTo = ROUTER().feeTo();
bool feeOn = feeTo != address(0);
if (feeOn) {
  uint256 fee = ROUTER().getProtocolFee(amount);

  unchecked {
	amount -= fee;
  }
  ERC20(asset()).safeTransfer(feeTo, fee);
}
return amount;
}

If the feeTo address is set, the code will query the protocol fee and deduct from the amount.

which calls ROUTER().getProtocolFee(amount)

  function getProtocolFee(uint256 amountIn) external view returns (uint256) {
    RouterStorage storage s = _loadRouterSlot();
  
    return
      amountIn.mulDivDown(s.protocolFeeNumerator, s.protocolFeeDenominator);
  }

note that the s.protocolFeeNumerator and s.protocolFeeDenominator is not set in the init function of the AstairRouter.sol, so if the feeTo address is set, transaction of commitToLien revert in division by zero error because s.protocolFeeDenominator is 0.

In fact, if we look into getLiquidatorFee and getBuyoutFee

  function getLiquidatorFee(uint256 amountIn) external view returns (uint256) {
    RouterStorage storage s = _loadRouterSlot();

    return
      amountIn.mulDivDown(
        s.liquidationFeeNumerator,
        s.liquidationFeeDenominator
      );
  }

  function getBuyoutFee(uint256 remainingInterestIn)
    external
    view
    returns (uint256)
  {
    RouterStorage storage s = _loadRouterSlot();
    return
      remainingInterestIn.mulDivDown(
        s.buyoutFeeNumerator,
        s.buyoutFeeDenominator
      );
  }

if the s.buyoutFeeDenominator and s.liquidationFeeDenominator is 0, the divison by zero error can also occur, but these parameter is set in the init function of the AstariaRouter.sol

s.liquidationFeeNumerator = uint32(130);
s.liquidationFeeDenominator = uint32(1000);
s.minInterestBPS = uint32((uint256(1e15) * 5) / (365 days));
s.minEpochLength = uint32(7 days);
s.maxEpochLength = uint32(45 days);
s.maxInterestRate = ((uint256(1e16) * 200) / (365 days)).safeCastTo88();
//63419583966; // 200% apy / second
s.buyoutFeeNumerator = uint32(100);
s.buyoutFeeDenominator = uint32(1000);

but s.protocolFeeNumerator and s.protocolFeeDenominator is not set in the init function of the AstairRouter.sol is not set in the init function of the AstariaRouter.sol

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol set the feeTo address and
s.protocolFeeNumerator and s.protocolFeeDenominator together.

Also validate the buyout fee, liqudatior fee and protocol fee's dominator are not 0 to avoid division by zero error.

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

SantiagoGregory marked the issue as disagree with severity

[SantiagoGregory]

Downgrading to low severity because this would be configured before the fee switch was turned on.

[Picodes]

Downgrading to low as this is a configuration mistake by the admin

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-a