Astaria contest - 0Kage's results

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L27

Vulnerability details

Impact

When a user calls the deposit function on Public Vault, deposit function in Line 27 of ERC4626-Cloned.sol gets called. Protocol only allows a deposit amount > minimum Deposit. However when making this check, code wrongly compares shares with minimumDeposit instead of comparing assets with minimumDeposit.

shares is a reflection of the ownership in the vault. Shares don't need to have a 1-to-1 conversion into assets. Higher the vault earnings, higher the share price -> lower the amount of shares that can be exchanged for a given unit of asset.

A wrong comparison can deny a legitimate depositor from depositing in the pool. Since the values are relatively small, I've marked this as MEDIUM risk

Proof of Concept

Alice deposits 100 gwei (min deposit) to get 100 gwei of shares Vault earnings double in 1 day (so 100 gwei shares represent 200 gwei in 1 day) Bob deposits 100 gwei -> this is equal to 50 gwei of shares -> this fails the check since 50 gwei (shares) < 100 gwei (minimum deposit).

Although Bob was a legitimate depositor, he got DOS'ed by the vault

Tools Used

Manual

Recommended Mitigation Steps

Replace following line in Line 27

require(shares > minDepositAmount(), "VALUE_TOO_SMALL");

with

require(assets > minDepositAmount(), "VALUE_TOO_SMALL");

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L48 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L168 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L131 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L163

Vulnerability details

Impact

withdraw() function in Line 168 of AstariaRouter.sol allows vault token owners to withdraw underlying asset from vault. Note that implementation calls the withdraw() function of its parent contract ERC4626RouterBase.sol, refer line 48.

Note that approval is taken for amount instead of maxSharesOut - amount here refers to the actual underlying asset that was deposited in the vault (eg. WETH) and not the shares held by owner.

This function then calls withdraw function in Line 131 of PublicVault.sol. This function reduces allowance of shares that are supposed to be burnt - since the approval was given for amount of underlying asset & not maxShares, this could lead to either over allowance/under allowance situation. Under allowance leads to denial-of-service, over allowance leads to open approvals that were never supposed to be open.

Since this can potentially lead to loss of vault tokens of LPs, I've marked it as HIGH risk

Proof of Concept

• Bob is a LP who owns 100 shares in vault whose present vaule is 1000 ETH (let's assume each vault share is worth 10 ETH)
• Bob calls the withdraw function of Astaria Router
• Bob wants to withdraw 100 WETH by burning a maximum of 10 vault shares
• Function takes approval for 100 shares instead of 10
• Even after withdrawal, Bob still unknowingly has given approval to vault to spend 90 more shares representing 900 ETH

Tools Used

Manual

Recommended Mitigation Steps

Change line 48 in ERCRouterBase.sol from

ERC20(address(vault)).safeApprove(address(vault), amount);

to

ERC20(address(vault)).safeApprove(address(vault), maxSharesOut);

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L48 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L168

Vulnerability details

Impact

Withdraw function in Line 48 of AstariaRouter.sol allows LP's to place withdraw requests for vault tokens. This function calls the withdraw function in Line 48 of its parent contract, ERC4626RouterBase.sol.

Note that this function is supposed to give approval for maxSharesOut (I've already submitted that issue) & calls the withdraw on PublicVault. PublicVault can burn shares <= maxSharesOut -> since approval was given to vault for maxSharesOut and actual amount of burned shares is less than maxSharesOut -> earlier approval given for maxSharesOut needs to be reset back to 0.

Not doing so will lead to over approval to vault that can be exploited later. Since this could directly lead to loss of LP tokens, I've marked it as HIGH risk

Proof of Concept

• Bob has given approval to vault to spend max of 100 shares (maxSharesOut) to withdraw 200 WETH (say 1 share = 2 WETH)
• Vault burns 98 shares to withdraw 200 WETH
• Bob's approval for 2 extra shares stays as is & can be exploited by the vault owner at a later date

Tools Used

Manual

Recommended Mitigation Steps

After vault runs withdraw function and returns shares -> and if shares < maxSharesOut, reset approval to 0.

  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();
    }
   // *********Recommend following********
  if(sharesOut < maxSharesOut){
    ERC20(address(vault)).safeApprove(address(vault), 0);
  }
}