user fund loss or lock in claim() of the WithdrawProxy because code uses asset's decimal instead of 1e18 for calculating transferAmount

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L268-L279

Vulnerability details

Impact

Function Claim() in WithdrawProxy return any excess funds to the PublicVault, according to the withdrawRatio between withdrawing and remaining LPs. but when code tries to calculating return amount according to the withdrawRatio it uses asset's decimals instead of 1e18 and if asset's decimals where different then the calculation would go wrong and multiple things can happen:

1. underflow would happens and calls would revert (when transferring assets) and users can't withdraw their funds and protocol would be in broken state.
2. rewards would get distributed wrongly
3. withdrawal funds would be transferred to PublicVault and users would lose their funds.

Proof of Concept

This is part of claim() function in WithdrawProxy which handles transferring excess funds to PublicVault:

  function claim() public {
....
....
    uint256 transferAmount = 0;
    uint256 balance = ERC20(asset()).balanceOf(address(this)) -
      s.withdrawReserveReceived; // will never underflow because withdrawReserveReceived is always increased by the transfer amount from the PublicVault

    if (balance < s.expected) {
      PublicVault(VAULT()).decreaseYIntercept(
        (s.expected - balance).mulWadDown(1e18 - s.withdrawRatio)
      );
    } else {
      PublicVault(VAULT()).increaseYIntercept(
        (balance - s.expected).mulWadDown(1e18 - s.withdrawRatio)
      );
    }

    if (s.withdrawRatio == uint256(0)) {
      ERC20(asset()).safeTransfer(VAULT(), balance);
    } else {
      transferAmount = uint256(s.withdrawRatio).mulDivDown(
        balance,
        10**ERC20(asset()).decimals()
      );

      unchecked {
        balance -= transferAmount;
      }

      if (balance > 0) {
        ERC20(asset()).safeTransfer(VAULT(), balance);
      }
    }
    s.finalAuctionEnd = 0;

    emit Claimed(address(this), transferAmount, VAULT(), balance);
  }

As you can see when code tries to calculate transferAmount it uses 10**ERC20(asset()).decimals() as dominator but withdrawRatio is calculated with 1e18 factor so the value of transferAmount would be wrong.
This is where withdraw ratio is calculated in the PublicVault and 1e18 has been used:

      s.liquidationWithdrawRatio = proxySupply
        .mulDivDown(1e18, totalSupply())
        .safeCastTo88();

This issue can have multiple impact:

1. if asset's decimals were lower than 18 then the value of the transferAmount would be very higher than balance and unchecked { balance -= transferAmount; } would underflow and the value of balance would be very high and asset transfer would fail always and code would revert. so calls to claim() would fail and code would never set finalAuctionEnd = 0 and this would cause calls to PublicVault.processEpoch() to fail and PublicVault would be in broken state forever users would lose access to their funds.
2. if asset's decimals were higher than 18 then the value of the transferAmount would be very low and code would consider those funds as excess amount and withdrawal users funds would get transferred to PublicVault and withdrawal users would lose their funds.

(the bug and impact is obvious so the detailed POC is not included)

Tools Used

VIM

Recommended Mitigation Steps

use 1e18 to calculate transferAmount

[c4-judge]

Picodes marked the issue as duplicate of #482

[c4-judge]

Picodes marked the issue as satisfactory