ERC4626Cloned deposit and mint logic differ on first deposit

[code423n4]

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.

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

androolloyd marked the issue as sponsor confirmed

[androolloyd]

@SantiagoGregory

[SantiagoGregory]

@androolloyd how should we handle this along with #367?

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as selected for report