Astaria contest - cccz's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L107-L231

Vulnerability details

Impact

Vault A can call buyoutLien to buy out Vault B's lien tokens, which calls LienToken.buyoutLien

 function buyoutLien(
   ILienToken.Stack[] calldata stack,
   uint8 position,
   IAstariaRouter.Commitment calldata incomingTerms
 )
   external
   whenNotPaused
   returns (ILienToken.Stack[] memory, ILienToken.Stack memory)
 {
...
   return
     lienToken.buyoutLien(
       ILienToken.LienActionBuyout({
         position: position,
         encumber: ILienToken.LienActionEncumber({
           amount: owed,
           receiver: recipient(),
           lien: ROUTER().validateCommitment({
             commitment: incomingTerms,
             timeToSecondEpochEnd: _timeToSecondEndIfPublic()
           }),
           stack: stack

In LienToken.buyoutLien, it will burn Vault B's lien token and mint a new lien token for Vault A

 function _replaceStackAtPositionWithNewLien(
   LienStorage storage s,
   ILienToken.Stack[] calldata stack,
   uint256 position,
   Stack memory newLien,
   uint256 oldLienId
 ) internal returns (ILienToken.Stack[] memory newStack) {
   newStack = stack;
   newStack[position] = newLien;
   _burn(oldLienId);                        // @ audit: burn Vault B's lien token
   delete s.lienMeta[oldLienId];
 }
...
   newLienId = uint256(keccak256(abi.encode(params.lien)));
   Point memory point = Point({
     lienId: newLienId,
     amount: params.amount.safeCastTo88(),
     last: block.timestamp.safeCastTo40(),
     end: (block.timestamp + params.lien.details.duration).safeCastTo40()
   });
   _mint(params.receiver, newLienId); // @ audit: mint a new lien token for Vault A
   return (newLienId, Stack({lien: params.lien, point: point}));
 }

And, when Vault B is a public vault, the handleBuyoutLien function of Vault B will be called to update the slope(and yIntercept/last) However, when Vault A is a public vault, it does not update Vault A's slope(and yIntercept/last)

   if (_isPublicVault(s, payee)) {
     IPublicVault(payee).handleBuyoutLien(
       IPublicVault.BuyoutLienParams({
         lienSlope: calculateSlope(params.encumber.stack[params.position]),
         lienEnd: params.encumber.stack[params.position].point.end,
         increaseYIntercept: buyout -
           params.encumber.stack[params.position].point.amount
       })
     );
   }
...
 function handleBuyoutLien(BuyoutLienParams calldata params)
   public
   onlyLienToken
 {
   VaultData storage s = _loadStorageSlot();

   unchecked {
     uint48 newSlope = s.slope - params.lienSlope.safeCastTo48();    // @audit: update the slope 
     _setSlope(s, newSlope);
     s.yIntercept += params.increaseYIntercept.safeCastTo88();
     s.last = block.timestamp.safeCastTo40();
   }

   _decreaseEpochLienCount(s, getLienEpoch(params.lienEnd.safeCastTo64())); 
   emit YInterceptChanged(s.yIntercept);
 }

Unlike buyoutLien, when the public vault minted lien tokens via commitToLien, the slope(and yIntercept/last) is updated in _afterCommitToLien

  function _afterCommitToLien(
    uint40 lienEnd,
    uint256 lienId,
    uint256 lienSlope
  ) internal virtual override {
    VaultData storage s = _loadStorageSlot();

    // increment slope for the new lien
    _accrue(s);
    unchecked {
      uint48 newSlope = s.slope + lienSlope.safeCastTo48();
      _setSlope(s, newSlope);
    }

    uint64 epoch = getLienEpoch(lienEnd);

    _increaseOpenLiens(s, epoch);
    emit LienOpen(lienId, epoch);
  }

The slope(and yIntercept/last) of a public vault is determined by the lien tokens held by the vault, and when the slope(and yIntercept/last) is set incorrectly, it can cause the totalAssets of the vault to be undervalued, thus making the vault's shares incorrectly calculated

  function _totalAssets(VaultData storage s) internal view returns (uint256) {
    uint256 delta_t = block.timestamp - s.last;
    return uint256(s.slope).mulDivDown(delta_t, 1) + uint256(s.yIntercept);
  }

Consider the following case. Public Vault B holds a lien token Public Vault A buys out B's lien token to refinance it At this point, the valuation of Public Vault A should include the interest of borrower contained in the lien token. However, since the slope(and yIntercept/last) of Public Vault A is not updated, the valuation of Public Vault A (totalAssets) is undervalued, so that the user receives more shares when depositing

  function convertToShares(
    uint256 assets
  ) public view virtual returns (uint256) {
    uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

    return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
  }

Proof of Concept

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L107-L231 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L439-L457 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L489-L493

Tools Used

None

Recommended Mitigation Steps

In LienToken.buyoutLien, when the caller is a public vault, update the slope(and yIntercept/last) of the public vault

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L360-L375

Vulnerability details

Impact

In LienToken.transferFrom, transferring lien tokens to the public vault is prohibited because variables such as liensOpenForEpoch are not updated when the public vault receives a lien token, which would prevent the borrower from repaying the loan in that lien token.

  function transferFrom(
    address from,
    address to,
    uint256 id
  ) public override(ERC721, IERC721) {
    LienStorage storage s = _loadLienStorageSlot();
    if (_isPublicVault(s, to)) {
      revert InvalidState(InvalidStates.PUBLIC_VAULT_RECIPIENT);
    }
    if (s.lienMeta[id].atLiquidation) {
      revert InvalidState(InvalidStates.COLLATERAL_AUCTION);
    }
    delete s.lienMeta[id].payee;
    emit PayeeChanged(id, address(0));
    super.transferFrom(from, to, id);
  }

However, public vaults are created using the ClonesWithImmutableArgs.clone function, which uses the create opcode, which allows the address of the public vault to be predicted before it is created.

https://ethereum.stackexchange.com/questions/760/how-is-the-address-of-an-ethereum-contract-computed

            assembly {
                instance := create(0, ptr, creationSize)
            }

This allows a malicious private vault to transfer lien tokens to the predicted public vault address in advance, and then call AstariaRouter.newPublicVault to create the public vault, which has a liensOpenForEpoch of 0. When the borrower repays the loan via LienToken.makePayment, decreaseEpochLienCount fails due to overflow in _payment, resulting in the liquidation of the borrower's collateral

    } else {
      amount = stack.point.amount;
      if (isPublicVault) {
        // since the openLiens count is only positive when there are liens that haven't been paid off
        // that should be liquidated, this lien should not be counted anymore
        IPublicVault(lienOwner).decreaseEpochLienCount(
          IPublicVault(lienOwner).getLienEpoch(end)
        );
      }

Consider the following scenario where private vault A provides a loan of 1 ETH to the borrower, who deposits NFT worth 2 ETH and borrows 1 ETH. Private Vault A creates Public Vault B using the account alice and predicts the address of Public Vault B before it is created and transfers the lien tokens to it. The borrower calls LienToken.makePayment to repay the loan, but fails due to overflow. The borrower is unable to repay the loan, and when the loan expires, the NFTs used as collateral are auctioned

Proof of Concept

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L360-L375 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L835-L847 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L731-L742 https://ethereum.stackexchange.com/questions/760/how-is-the-address-of-an-ethereum-contract-computed

Tools Used

None

Recommended Mitigation Steps

In LienToken.transferFrom, require to.code.length >0, thus preventing the transfer of lien tokens to uncreated public vaults

  function transferFrom(
    address from,
    address to,
    uint256 id
  ) public override(ERC721, IERC721) {
    LienStorage storage s = _loadLienStorageSlot();
    if (_isPublicVault(s, to)) {
      revert InvalidState(InvalidStates.PUBLIC_VAULT_RECIPIENT);
    }
+  require(to.code.length > 0);
    if (s.lienMeta[id].atLiquidation) {
      revert InvalidState(InvalidStates.COLLATERAL_AUCTION);
    }
    delete s.lienMeta[id].payee;
    emit PayeeChanged(id, address(0));
    super.transferFrom(from, to, id);
  }

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L313-L351

Vulnerability details

Impact

Vault A can call buyoutLien to buy out Vault B's lien tokens, which calls LienToken.buyoutLien

  function buyoutLien(
    ILienToken.Stack[] calldata stack,
    uint8 position,
    IAstariaRouter.Commitment calldata incomingTerms
  )
    external
    whenNotPaused
    returns (ILienToken.Stack[] memory, ILienToken.Stack memory)
  {
...
    return
      lienToken.buyoutLien(
        ILienToken.LienActionBuyout({
          position: position,
          encumber: ILienToken.LienActionEncumber({
            amount: owed,
            receiver: recipient(),
            lien: ROUTER().validateCommitment({
              commitment: incomingTerms,
              timeToSecondEpochEnd: _timeToSecondEndIfPublic()
            }),
            stack: stack

In LienToken.buyoutLien, it will burn Vault B's lien token and mint a new lien token for Vault A

  function _replaceStackAtPositionWithNewLien(
    LienStorage storage s,
    ILienToken.Stack[] calldata stack,
    uint256 position,
    Stack memory newLien,
    uint256 oldLienId
  ) internal returns (ILienToken.Stack[] memory newStack) {
    newStack = stack;
    newStack[position] = newLien;
    _burn(oldLienId);                        // @ audit: burn Vault B's lien token
    delete s.lienMeta[oldLienId];
  }
...
    newLienId = uint256(keccak256(abi.encode(params.lien)));
    Point memory point = Point({
      lienId: newLienId,
      amount: params.amount.safeCastTo88(),
      last: block.timestamp.safeCastTo40(),
      end: (block.timestamp + params.lien.details.duration).safeCastTo40()
    });
    _mint(params.receiver, newLienId); // @ audit: mint a new lien token for Vault A
    return (newLienId, Stack({lien: params.lien, point: point}));
  }

And, when Vault B is a public vault, the handleBuyoutLien function of Vault B will be called to decrease liensOpenForEpoch However, when Vault A is a public vault, it does not increase the liensOpenForEpoch of Vault A

    if (_isPublicVault(s, payee)) {
      IPublicVault(payee).handleBuyoutLien(
        IPublicVault.BuyoutLienParams({
          lienSlope: calculateSlope(params.encumber.stack[params.position]),
          lienEnd: params.encumber.stack[params.position].point.end,
          increaseYIntercept: buyout -
            params.encumber.stack[params.position].point.amount
        })
      );
    }
...
  function handleBuyoutLien(BuyoutLienParams calldata params)
    public
    onlyLienToken
  {
    VaultData storage s = _loadStorageSlot();

    unchecked {
      uint48 newSlope = s.slope - params.lienSlope.safeCastTo48();
      _setSlope(s, newSlope);
      s.yIntercept += params.increaseYIntercept.safeCastTo88();
      s.last = block.timestamp.safeCastTo40();
    }

    _decreaseEpochLienCount(s, getLienEpoch(params.lienEnd.safeCastTo64()));  // @audit: decrease liensOpenForEpoch 
    emit YInterceptChanged(s.yIntercept);
  }

Since the liensOpenForEpoch of the public vault decreases when the lien token is repaid, and since the liensOpenForEpoch of public vault A is not increased, then when that lien token is repaid, _payment will fail due to overflow when decreasing the liensOpenForEpoch.

    } else {
      amount = stack.point.amount;
      if (isPublicVault) {
        // since the openLiens count is only positive when there are liens that haven't been paid off
        // that should be liquidated, this lien should not be counted anymore
        IPublicVault(lienOwner).decreaseEpochLienCount(  //  @audit: overflow here
          IPublicVault(lienOwner).getLienEpoch(end)
        );
      }

Consider the following case. Public Vault B holds a lien token and B.liensOpenForEpoch == 1 Public Vault A buys out B's lien token for refinancing, B.liensOpenForEpoch == 0, A.liensOpenForEpoch == 0 borrower wants to repay the loan, in the _payment function, the decreaseEpochLienCount function of Vault A will be called, A.liensOpenForEpoch-- will trigger an overflow, resulting in borrower not being able to repay the loan, and borrower's collateral will be auctioned off, but in the call to updateVaultAfterLiquidation function will also fail in decreaseEpochLienCount due to the overflow

  function updateVaultAfterLiquidation(
    uint256 maxAuctionWindow,
    AfterLiquidationParams calldata params
  ) public onlyLienToken returns (address withdrawProxyIfNearBoundary) {
    VaultData storage s = _loadStorageSlot();

    _accrue(s);
    unchecked {
      _setSlope(s, s.slope - params.lienSlope.safeCastTo48());
    }

    if (s.currentEpoch != 0) {
      transferWithdrawReserve();
    }
    uint64 lienEpoch = getLienEpoch(params.lienEnd);
    _decreaseEpochLienCount(s, lienEpoch); //  @audit: overflow here

As a result, the borrower cannot repay the loan and the borrower's collateral cannot be auctioned off, thus causing the depositor of the public vault to suffer a loss

Proof of Concept

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L313-L351 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L835-L843 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L640-L655

Tools Used

None

Recommended Mitigation Steps

In LienToken.buyoutLien, when the caller is a public vault, increase the decreaseEpochLienCount of the public vault

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L15-L25

Vulnerability details

Impact

ERC4626RouterBase.mint approves shares of asset tokens to the vault, but shares represents the number of vault tokens minted by vault.mint, not the number of asset tokens required, since normally it takes more than 1 asset token to mint 1 vault token, this will result in a failure in vault.mint due to insufficient number of approved asset tokens.

  function mint(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 maxAmountIn
  ) public payable virtual override returns (uint256 amountIn) {
    ERC20(vault.asset()).safeApprove(address(vault), shares);
    if ((amountIn = vault.mint(shares, to)) > maxAmountIn) {
      revert MaxAmountError();
    }
  }

Proof of Concept

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L15-L25

Tools Used

None

Recommended Mitigation Steps

Change to

  function mint(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 maxAmountIn
  ) public payable virtual override returns (uint256 amountIn) {
+    ERC20(vault.asset()).safeApprove(address(vault), maxAmountIn);
-   ERC20(vault.asset()).safeApprove(address(vault), shares);
    if ((amountIn = vault.mint(shares, to)) > maxAmountIn) {
      revert MaxAmountError();
    }
+  ERC20(vault.asset()).safeApprove(address(vault), 0);
  }

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L849-L850

Vulnerability details

Impact

When the borrower calls LienToken.makePayment to repay the loan, safeTransferFrom is used to send tokens to the recipient of the vault, which in the case of a private vault is the owner of the private vault.

    s.TRANSFER_PROXY.tokenTransferFrom(stack.lien.token, payer, payee, amount);
...
  function tokenTransferFrom(
    address token,
    address from,
    address to,
    uint256 amount
  ) external requiresAuth {
    ERC20(token).safeTransferFrom(from, to, amount);
  }
...
  function recipient() public view returns (address) {
    if (IMPL_TYPE() == uint8(IAstariaRouter.ImplementationType.PublicVault)) {
      return address(this);
    } else {
      return owner();
    }
  }

If the token for the loan is an ERC777 token, a malicious private vault owner can refuse to receive repayment in the callback, which results in the borrower not being able to repay the loan and the borrower's collateral being auctioned off when the loan expires.

Proof of Concept

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L849-L850 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/TransferProxy.sol#L28-L35 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L900-L909 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L366-L372

Tools Used

None

Recommended Mitigation Steps

For private vaults, when the borrower repays, sending tokens to the vault, and the private vault owner claims it later

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L181-L186

Vulnerability details

Impact

When LienToken.buyoutLien is called to buy out the lien token, it sends the token to the recipient of the vault using safeTransferFrom, where recipient is the owner of the private vault in the case of a private vault.

    s.TRANSFER_PROXY.tokenTransferFrom(
      params.encumber.stack[params.position].lien.token,
      msg.sender,
      payee,
      buyout
    );
...
function tokenTransferFrom(
  address token,
  address from,
  address to,
  uint256 amount
) external requiresAuth {
  ERC20(token).safeTransferFrom(from, to, amount);
}
...
function recipient() public view returns (address) {
  if (IMPL_TYPE() == uint8(IAstariaRouter.ImplementationType.PublicVault)) {
    return address(this);
  } else {
    return owner();
  }
}

If the loan token is an ERC777 token, a malicious private vault owner can refuse to receive the token in the callback, which results in the lien token not being bought out and prevents the borrower from receiving a higher quality loan

Proof of Concept

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L181-L186

Tools Used

None

Recommended Mitigation Steps

For private vaults, sending the tokens to the vault at the time of buyout, after which the private vault owner claims it

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L41-L52

Vulnerability details

Impact

ERC4626RouterBase.withdraw will approve an amount of vault tokens to the vault, but the amount represents the number of asset tokens taken out by vault.withdraw, not the required number of vault tokens, and since it normally requires less than 1 vault token to take out 1 asset token, it will prevent ERC4626RouterBase.withdraw from using all approved vault tokens.

  function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 maxSharesOut
  ) public payable virtual override returns (uint256 sharesOut) {

    ERC20(address(vault)).safeApprove(address(vault), amount);
    if ((sharesOut = vault.withdraw(amount, to, msg.sender)) > maxSharesOut) {
      revert MaxSharesError();
    }
  }

and since safeApprove cannot approve a non-zero value to a non-zero value, the second call to ERC4626RouterBase.withdraw will fails in safeApprove.

    function safeApprove(
        IERC20 token,
        address spender,
        uint256 value
    ) internal {
        // safeApprove should only be called when setting an initial allowance,
        // or when resetting it to zero. To increase and decrease it, use
        // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
        require(
            (value == 0) || (token.allowance(address(this), spender) == 0),
            "SafeERC20: approve from non-zero to non-zero allowance"
        );
        _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
    }

Proof of Concept

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L41-L52

Tools Used

None

Recommended Mitigation Steps

Change to

  function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
-   uint256 maxSharesOut
+  uint256 maxSharesIn
  ) public payable virtual override returns (uint256 sharesOut) {
+   ERC20(address(vault)).safeApprove(address(vault), maxSharesIn);
+   if ((sharesIn = vault.withdraw(amount, to, msg.sender)) > maxSharesIn) {
-   ERC20(address(vault)).safeApprove(address(vault), amount);
-   if ((sharesOut = vault.withdraw(amount, to, msg.sender)) > maxSharesOut) {
      revert MaxSharesError();
    }
+   ERC20(address(vault)).safeApprove(address(vault), 0);

  }