xiaoming90 - Value of vault shares can be manipulated

[sherlock-admin3]

xiaoming90

Medium

Value of vault shares can be manipulated

Summary

The value of vault shares can be manipulated. Inflating the value of vault shares is often the precursor of more complex attacks. Internal (Notional-side) or external protocols that integrate with the vault shares might be susceptible to potential attacks in the future that exploit this issue.

Vulnerability Detail

It was found that the value of the vault shares can be manipulated.

Instance 1 - Kelp

To increase the value of vault share, malicious can directly transfer a large number of stETH to their KelpCooldownHolder contract. In Line 78, the holder contract will determine the number of stETH to be withdrawn from LIDO via IERC20(stETH).balanceOf(address(this)). This means that all the stETH tokens residing on the holder contract, including the ones that are maliciously transferred in, will be withdrawn from LIDO.

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Kelp.sol#L78

File: Kelp.sol
69:     /// @notice this method need to be called once withdraw on Kelp is finalized
70:     /// to start withdraw process from Lido so we can unwrap stETH to ETH
71:     /// since we are not able to withdraw ETH directly from Kelp
72:     function triggerExtraStep() external {
73:         require(!triggered);
74:         (/* */, /* */, /* */, uint256 userWithdrawalRequestNonce) = WithdrawManager.getUserWithdrawalRequest(stETH, address(this), 0);
75:         require(userWithdrawalRequestNonce < WithdrawManager.nextLockedNonce(stETH));
76: 
77:         WithdrawManager.completeWithdrawal(stETH);
78:         uint256 tokensClaimed = IERC20(stETH).balanceOf(address(this));
79: 
80:         uint256[] memory amounts = new uint256[](1);
81:         amounts[0] = tokensClaimed;
82:         IERC20(stETH).approve(address(LidoWithdraw), amounts[0]);
83:         LidoWithdraw.requestWithdrawals(amounts, address(this));
84: 
85:         triggered = true;
86:     }

When determining the value of vault share of a user, the convertStrategyToUnderlying function will be called, which internally calls _getValueOfWithdrawRequest function.

The withdrawsStatus[0].amountOfStETH at Line 126 will be inflated as the amount will include the stETH attackers maliciously transferred earlier. As a result, the vault share will be inflated.

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Kelp.sol#L126

File: Kelp.sol
114:     function _getValueOfWithdrawRequest(
115:         WithdrawRequest memory w,
116:         address borrowToken,
117:         uint256 borrowPrecision
118:     ) internal view returns (uint256) {
119:         address holder = address(uint160(w.requestId));
120: 
121:         uint256 expectedStETHAmount;
122:         if (KelpCooldownHolder(payable(holder)).triggered()) {
123:             uint256[] memory requestIds = LidoWithdraw.getWithdrawalRequests(holder);
124:             ILidoWithdraw.WithdrawalRequestStatus[] memory withdrawsStatus = LidoWithdraw.getWithdrawalStatus(requestIds);
125: 
126:             expectedStETHAmount = withdrawsStatus[0].amountOfStETH;
127:         } else {
128:             (/* */, expectedStETHAmount, /* */, /* */) = WithdrawManager.getUserWithdrawalRequest(stETH, holder, 0);
129: 
130:         }
131: 
132:         (int256 stETHToBorrowRate, /* */) = Deployments.TRADING_MODULE.getOraclePrice(
133:             address(stETH), borrowToken
134:         );
135: 
136:         return (expectedStETHAmount * stETHToBorrowRate.toUint() * borrowPrecision) /
137:             (Constants.EXCHANGE_RATE_PRECISION * stETH_PRECISION);
138:     }

Instance 2 - Ethena

Etherna vault is vulnerable to similar issue due to the due of .balanceOf at Line 37 below.

Before starting the cooldown, malicious user can directly transfer in a large number of sUSDe to the EthenaCooldownHolder holder contract.

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Ethena.sol#L37

File: Ethena.sol
35:     function _startCooldown() internal override {
36:         uint24 duration = sUSDe.cooldownDuration();
37:         uint256 balance = sUSDe.balanceOf(address(this));
38:         if (duration == 0) {
39:             // If the cooldown duration is set to zero, can redeem immediately
40:             sUSDe.redeem(balance, address(this), address(this));
41:         } else {
42:             // If we execute a second cooldown while one exists, the cooldown end
43:             // will be pushed further out. This holder should only ever have one
44:             // cooldown ever.
45:             require(sUSDe.cooldowns(address(this)).cooldownEnd == 0);
46:             sUSDe.cooldownShares(balance);
47:         }
48:     }

Thus, when code in Lines 87 and 99 are executed, the userCooldown.underlyingAmount returns will be large, which inflates the value of the vault shares.

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Ethena.sol#L77

File: Ethena.sol
077:     function _getValueOfWithdrawRequest(
078:         WithdrawRequest memory w,
079:         address borrowToken,
080:         uint256 borrowPrecision
081:     ) internal view returns (uint256) {
082:         address holder = address(uint160(w.requestId));
083:         // This valuation is the amount of USDe the account will receive at cooldown, once
084:         // a cooldown is initiated the account is no longer receiving sUSDe yield. This balance
085:         // of USDe is transferred to a Silo contract and guaranteed to be available once the
086:         // cooldown has passed.
087:         IsUSDe.UserCooldown memory userCooldown = sUSDe.cooldowns(holder);
088: 
089:         int256 usdeToBorrowRate;
090:         if (borrowToken == address(USDe)) {
091:             usdeToBorrowRate = int256(Constants.EXCHANGE_RATE_PRECISION);
092:         } else {
093:             // If not borrowing USDe, convert to the borrowed token
094:             (usdeToBorrowRate, /* */) = Deployments.TRADING_MODULE.getOraclePrice(
095:                 address(USDe), borrowToken
096:             );
097:         }
098: 
099:         return (userCooldown.underlyingAmount * usdeToBorrowRate.toUint() * borrowPrecision) /
100:             (Constants.EXCHANGE_RATE_PRECISION * USDE_PRECISION);
101:     }

Impact

Inflating the value of vault shares is often the precursor of more complex attacks. Internal (Notional-side) or external protocols that integrate with the vault shares might be susceptible to potential attacks in the future that exploit this issue.

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Kelp.sol#L78

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Kelp.sol#L126

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Ethena.sol#L37

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Ethena.sol#L77

Tool used

Manual Review

Recommendation

Instance 1 - Kelp

Consider using the before and after balances to determine the actual number of stETH obtained after the execution of WithdrawManager.completeWithdrawal function to guard against potential donation attacks.

function triggerExtraStep() external {
    require(!triggered);
    (/* */, /* */, /* */, uint256 userWithdrawalRequestNonce) = WithdrawManager.getUserWithdrawalRequest(stETH, address(this), 0);
    require(userWithdrawalRequestNonce < WithdrawManager.nextLockedNonce(stETH));
+       uint256 tokenBefore = IERC20(stETH).balanceOf(address(this));
    WithdrawManager.completeWithdrawal(stETH);
+       uint256 tokenAfter = IERC20(stETH).balanceOf(address(this));

-    uint256 tokensClaimed = IERC20(stETH).balanceOf(address(this));
+    uint256 tokensClaimed = tokenAfter - tokenBefore

    uint256[] memory amounts = new uint256[](1);
    amounts[0] = tokensClaimed;
    IERC20(stETH).approve(address(LidoWithdraw), amounts[0]);
    LidoWithdraw.requestWithdrawals(amounts, address(this));

    triggered = true;
}

Instance 2 - Ethena

Pass in the actual amount of sUSDe that needs to be withdrawn instead of using the balanceOf.

- function _startCooldown() internal override {
+ function _startCooldown(uint256 balance) internal override {
    uint24 duration = sUSDe.cooldownDuration();
-    uint256 balance = sUSDe.balanceOf(address(this));
    if (duration == 0) {
        // If the cooldown duration is set to zero, can redeem immediately
        sUSDe.redeem(balance, address(this), address(this));
    } else {
        // If we execute a second cooldown while one exists, the cooldown end
        // will be pushed further out. This holder should only ever have one
        // cooldown ever.
        require(sUSDe.cooldowns(address(this)).cooldownEnd == 0);
        sUSDe.cooldownShares(balance);
    }
}

[jeffywu]

While this is valid and we will fix it, it's also not clear that this can be used as an attack vector. Vault shares cannot be shorted so manipulating the price up does not create an obvious way for an attacker to profit.

[novaman33]

Escalate, It is an informational finding that does not show an a clear attack. It falls into the future issues category of sherlock docs.

[sherlock-admin3]

Escalate, It is an informational finding that does not show an a clear attack. It falls into the future issues category of sherlock docs.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

The report itself says that it opens up an attack vector for the future. Moreover, I don't see how this issue causes the loss of funds or qualifies for Med severity.

Planning to accept the escalation and invalidate the issue.

[0502lian]

The report itself says that it opens up an attack vector for the future. Moreover, I don't see how this issue causes the loss of funds or qualifies for Med severity.

Planning to accept the escalation and leave the issue as it is.

Maybe my understanding is incorrect. If I am wrong, please correct me. Planning to accept the escalation——means not leaving the issue as it is, means the issue is a low/info issue.

[WangSecurity]

@0502lian thank you, you’re correct

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• novaman33: accepted

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/leveraged-vaults/pull/105

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.