Astaria contest - adriro's results

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L123-L127 https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L129-L133

Vulnerability details

The ERC4626Cloned contract is an implementation of the ERC4626 used for vaults. The standard contains a deposit function to deposit a specific amount of the underlying asset, and a mint function that will calculate the amount needed of the underlying token to mint a specific number of shares.

This calculation is done in previewDeposit and previewMint:

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

function previewDeposit(
  uint256 assets
) public view virtual returns (uint256) {
  return convertToShares(assets);
}

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

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/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);
}

In the case of the first deposit (i.e. when supply == 0), previewDeposit will return the same assets amount for the shares (this is the standard implementation), while previewMint will simply return 10e18.

Impact

It seems the intention was to mint a high initial number of shares on first deposit, so an attacker couldn't mint a low number of shares and manipulate the pool to frontrun an initial depositor.

However, the protocol has failed to replicate this logic in the deposit function, as both deposit and mint logic differ (see PoC).

An attacker can still use the deposit function to mint any number of shares.

PoC

contract MockERC20 is ERC20("Mock ERC20", "MERC20", 18) {
    function mint(address account, uint256 amount) external {
        _mint(account, amount);
    }
}

contract TestERC4626 is ERC4626Cloned {
    ERC20 _asset;

    constructor() {
        _asset = new MockERC20();
    }


    function asset() public override view returns (address assetTokenAddress) {
        return address(_asset);
    }

    function minDepositAmount() public override view returns (uint256) {
        return 0;
    }

    function totalAssets() public override view returns (uint256) {
        return _asset.balanceOf(address(this));
    }

    function symbol() external override view returns (string memory) {
        return "TEST4626";
    }
    function name() external override view returns (string memory) {
        return "TestERC4626";
    }

    function decimals() external override view returns (uint8) {
        return 18;
    }
}

contract AuditTest is Test {
    function test_ERC4626Cloned_DepositMintDiscrepancy() public {
        TestERC4626 vault = new TestERC4626();
        MockERC20 token = MockERC20(vault.asset());

        // Amount we deposit
        uint256 amount = 25e18;
        // Shares we get if we deposit amount
        uint256 shares = vault.previewDeposit(amount);
        // Amount needed to mint shares
        uint256 amountNeeded = vault.previewMint(shares);

        // The following values should be equal but they not
        assertFalse(amount == amountNeeded);

        // An attacker can still mint a single share by using deposit to manipulate the pool
        token.mint(address(this), 1);
        token.approve(address(vault), type(uint256).max);
        uint256 mintedShares = vault.deposit(1, address(this));

        assertEq(mintedShares, 1);
    }
}

Recommendation

The deposit function should also implement the same logic as the mint function for the case of the first depositor.

Lines of code

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

Vulnerability details

The ERC4626RouterBase contract contains a set of functions that act as wrappers for a ERC4626 contract, providing a base periphery functionality around a ERC4626 vault.

There are a number of different flaws in the wrapped implementations of mint, deposit, withdraw and redeem.

Impact

All four functions present in the contract are marked as payable (because of the IERC4626RouterBase interface definitions) but don't need to handle ETH payments, as these operate on the assumption that the underlying asset is an ERC20 token. This will cause loss of funds if a user sends ETH to any of the functions.

Note that these functions are exposed as payable functions in the AstariaRouter contract, as this contract inherits from ERC4626Router.

mint

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/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();
  }
}

The mint function uses shares as the amount to ERC20.approve but this value is wrong, as shares is the output of the vault. It should first call previewMint to get the amount of the underlying token needed to mint that number of shares. Compare that amount against the maxAmountIn and then call ERC20.approve with that value.

withdraw and redeem

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/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/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L55-L66

function redeem(
  IERC4626 vault,
  address to,
  uint256 shares,
  uint256 minAmountOut
) public payable virtual override returns (uint256 amountOut) {

  ERC20(address(vault)).safeApprove(address(vault), shares);
  if ((amountOut = vault.redeem(shares, to, msg.sender)) < minAmountOut) {
    revert MinAmountError();
  }
}

Both function calls to ERC20.approve are not valid, since ERC4626.withdraw will burn the shares and no transfers of shares are needed.

Recommendations

• In all cases, since functions are marked as payable, verify the value sent is 0 require(msg.value == 0) to prevent accidental loss of funds.

• For mint, use previewMint to know how much assets are needed to mint the given shares:

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

• For withdraw and redeem, remove the call to ERC20.approve

function withdraw(
  IERC4626 vault,
  address to,
  uint256 amount,
  uint256 maxSharesOut
) public payable virtual override returns (uint256 sharesOut) {
  require(msg.value == 0);
  if ((sharesOut = vault.withdraw(amount, to, msg.sender)) > maxSharesOut) {
    revert MaxSharesError();
  }
}

function redeem(
  IERC4626 vault,
  address to,
  uint256 shares,
  uint256 minAmountOut
) public payable virtual override returns (uint256 amountOut) {
  require(msg.value == 0);
  if ((amountOut = vault.redeem(shares, to, msg.sender)) < minAmountOut) {
    revert MinAmountError();
  }
}

Judge has assessed an item in Issue #596 as 2 risk. The relevant finding follows:

withdraw and redeem https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/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/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L55-L66

function redeem( IERC4626 vault, address to, uint256 shares, uint256 minAmountOut ) public payable virtual override returns (uint256 amountOut) {

ERC20(address(vault)).safeApprove(address(vault), shares); if ((amountOut = vault.redeem(shares, to, msg.sender)) < minAmountOut) { revert MinAmountError(); } } Both function calls to ERC20.approve are not valid, since ERC4626.withdraw will burn the shares and no transfers of shares are needed.