-
Notifications
You must be signed in to change notification settings - Fork 2
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
a5031b8
commit 85cb09d
Showing
172 changed files
with
12,764 additions
and
10 deletions.
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
Scrawny Iron Tiger | ||
|
||
High | ||
|
||
# ETH Locked in Vault Due to Rounding Errors in Fee Distribution | ||
|
||
## Summary | ||
The LidoVault contract manages deposits, staking through Lido, and distributes rewards and early exit fees between variable and fixed users. When fixed users withdraw before the vault ends, they are charged early exit fees, which are then distributed to variable users and the protocol fee receiver. However, due to the lack of floating-point arithmetic in Solidity, rounding errors in fee calculations cause small amounts of ETH to be left behind in the contract, permanently locking those funds. | ||
|
||
## Vulnerability Detail | ||
The issue arises in the calculateVariableWithdrawState and calculateVariableWithdrawStateWithUser functions, where the following calculation is used to distribute early exit fees: | ||
uint256 totalOwed = bearerBalance.mulDiv(totalEarnings, variableSideCapacity); | ||
|
||
Here, Solidity’s integer division causes the remainder from the division (bearerBalance * totalEarnings % variableSideCapacity) to be discarded. This means small fractions of ETH are not distributed to any user but remain in the contract. | ||
|
||
Over time, as more transactions occur, these undistributed remainders accumulate, leading to permanently locked ETH in the vault. Because the fees are already moved from Lido to the vault, these remainders remain in the vault with no mechanism to recover them. | ||
|
||
## Impact | ||
This vulnerability leads to small but gradually increasing amounts of ETH being locked in the contract, potentially reducing the overall ETH distributed to users. The longer the contract operates, the larger this locked ETH becomes, representing a loss for users and potentially affecting the correct distribution of rewards and fees. | ||
|
||
## Code Snippet | ||
https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L890 | ||
https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L867 | ||
|
||
## Tool used | ||
|
||
Manual Review | ||
|
||
## Recommendation | ||
To prevent ETH from being locked in the vault due to rounding errors, consider leaving early exit fees in Lido instead of transferring them to the vault. Since the Lido contract manages most of the vault's assets, keeping the early exit fees in Lido would allow the fees to accrue and be distributed without suffering from rounding errors in the vault's internal calculations. | ||
|
||
Alternatively, consider implementing a mechanism to recover and redistribute the small remainder amounts that are currently left behind due to Solidity's integer division. This could involve a periodic sweep or redistribution of remaining ETH to active users. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
Rural Fuchsia Starfish | ||
|
||
Medium | ||
|
||
# Calls To `vaultEndedWithdraw()` Can Revert Through Underflow Preventing Finalization | ||
|
||
### Summary | ||
|
||
Some outcomes of the `LidoVault` can lead to a `totalEarnings` calculation that results in `revert` during variable side withdrawal. | ||
|
||
This prevents a full term variable side from ever realizing their earnings in accrued fees. | ||
|
||
### Root Cause | ||
|
||
The following [`totalEarnings` calculation](https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L775), executed in calls to [`vaultEndedWithdraw(uint256)`](https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L709C12-L709C44) is liable to integer underflow: | ||
|
||
```solidity | ||
uint256 totalEarnings = vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes, vaultEndingStakesAmount) - totalProtocolFee + vaultEndedStakingEarnings; | ||
if (totalEarnings > 0) { | ||
// ... | ||
``` | ||
|
||
Although we safely handle the case where `totalEarnings` may not evaluate to zero, we do not handle the edge case where `totalProtocolFee`s are in excess of the `vaultEndingETHBalance` (i.e. through slashing). | ||
|
||
Notice then, that for the case where `vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes, vaultEndingStakesAmount)` is less than the `totalProtocalFee`, this calculation will revert through underflow, i.e.: | ||
|
||
```solidity | ||
uint256 a = vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes, vaultEndingStakesAmount); | ||
uint256 b = totalProtocolFee; | ||
uint256 totalEarnings = b - a + vaultEndedStakingEarnings; /// @audit a > b will revert | ||
``` | ||
|
||
For the case where `a` is greater than `b`, this calculation will revert, preventing successful completion of the `vaultEndedWithdraw(uint256)`. | ||
|
||
Although the vault may be finalized by a fixed side participant, no variable side accounts will be able to [progress far enough to successfully redeem their share of the fee shares](https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L787C7-L797C1). | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
1. A `LidoVault` `isEnded()` with a value of `vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes, vaultEndingStakesAmount)` which is less than the `totalProtocalFee`. | ||
|
||
### Impact | ||
|
||
Inability for all variable side withdrawals to finalize the vault and [claim their share of fees](https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L787C7-L797C1) due to DoS. | ||
|
||
### PoC | ||
|
||
_No response_ | ||
|
||
### Mitigation | ||
|
||
Sanity check the calculation before attempting the subtraction: | ||
|
||
```diff | ||
- uint256 totalEarnings = vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes, vaultEndingStakesAmount) - totalProtocolFee + vaultEndedStakingEarnings; | ||
+ uint256 shareOfEndingBalance = vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes, vaultEndingStakesAmount) + vaultEndedStakingEarnings; | ||
+ uint256 totalEarnings = shareOfEndingBalance > totalProtocolFee | ||
+ ? shareOfEndingBalance - totalProtocolFee | ||
+ : 0; | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
Rural Fuchsia Starfish | ||
|
||
Medium | ||
|
||
# Variable Fee Earnings Share Calculation Overvalues Variable Side Withdrawal's Claim To Fees | ||
|
||
### Summary | ||
|
||
When performing a variable side withdraw, the withdrawer's claim to the [`feeShareAmount`](https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L787C7-L797C1) is amplified through mistaking the effect of charging the `protocolFee` as debt owed to the caller. | ||
|
||
### Root Cause | ||
|
||
When a variable side withdrawal is executed, [their staking earnings are computed as follows](https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L536C13-L540C88): | ||
|
||
```solidity | ||
withdrawnStakingEarnings += ethAmountOwed - protocolFee; /// @audit earnings inclusive of fees | ||
withdrawnStakingEarningsInStakes += stakesAmountOwed; /// @audit earnings in shares not inclusive of fees | ||
variableToWithdrawnStakingEarnings[msg.sender] += ethAmountOwed - protocolFee; /// @audit earnings inclusive of fees | ||
variableToWithdrawnStakingEarningsInShares[msg.sender] += stakesAmountOwed; /// @audit earnings in shares not inclusive of fees | ||
``` | ||
|
||
This in itself is not a problem, because the `variableToWithdrawnStakingEarnings` is designed to track the specific accounting for a user's withdrawal (subject to protocol fees), whereas the `variableToWithdrawnStakingEarningsInShares` is designed to represent the user's total claim to shares at the point the withdrawal was made. | ||
|
||
Their total claim to shares (non-inclusive of fees) is used to determine the `msg.sender`'s total share of the accumulated fees at vault finalization. | ||
|
||
To describe this concept, notice that in `getCalculateVariableWithdrawStateWithStakingBalance(uint256)`, [a staker's `previousWithdrawnAmount` is amplified](https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L885C5-L885C118) to undo the effects of the protocol fee and ensure the calculation is performed in fixed terms of their total shares of the vault at withdrawal: | ||
|
||
```solidity | ||
/// @audit Amplify to undo the effects of the protocol fee so we | ||
/// @audit can reliably relate the `variableToWithdrawnStakingEarnings` | ||
/// @audit to as a function of the user's total share of the vault. | ||
uint256 previousWithdrawnAmount = variableToWithdrawnStakingEarnings[user].mulDiv(10000, 10000 - protocolFeeBps); | ||
``` | ||
|
||
Now, notice that to calculate the `msg.sender`'s `feeShareAmount`, we make a subsequent call [`calculateVariableFeeEarningsShare`](https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L787C7-L790C8): | ||
|
||
```solidity | ||
uint256 feeShareAmount = 0; | ||
if (withdrawnFeeEarnings + feeEarnings > 0) { | ||
feeShareAmount = calculateVariableFeeEarningsShare(); | ||
} | ||
``` | ||
|
||
Diving into the function, we see the following: | ||
|
||
```solidity | ||
function calculateVariableFeeEarningsShare() internal returns (uint256) { | ||
(uint256 currentState, uint256 feeEarningsShare) = calculateVariableWithdrawState( | ||
feeEarnings + withdrawnFeeEarnings, /// @audit Calculating the total fees accrued across all shares. | ||
@> variableToWithdrawnFees[msg.sender] /// @audit Using the value inclusive of fees. | ||
); | ||
variableToWithdrawnFees[msg.sender] = currentState; | ||
withdrawnFeeEarnings += feeEarningsShare; | ||
feeEarnings -= feeEarningsShare; | ||
return feeEarningsShare; | ||
} | ||
``` | ||
|
||
Therefore, the protocol uses a value of `variableToWithdrawnFees[msg.sender]` which decreases the `previousWithdrawnAmount` in calls to [`calculateVariableWithdrawState`](https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L859C12-L859C42), since **this is the value inclusive of fees**: | ||
|
||
```solidity | ||
function calculateVariableWithdrawState( | ||
uint256 totalEarnings, | ||
uint256 previousWithdrawnAmount /// @audit Using a lower value since fees were taken from this amount. | ||
) internal view returns (uint256, uint256) { | ||
uint256 bearerBalance = variableBearerToken[msg.sender]; | ||
require(bearerBalance > 0, "NBT"); | ||
uint256 totalOwed = bearerBalance.mulDiv(totalEarnings, variableSideCapacity); | ||
uint256 ethAmountOwed = 0; | ||
if (previousWithdrawnAmount < totalOwed) { | ||
/// @audit Increases perceived `ethAmountOut` since fees make it | ||
/// @audit look like the `msg.sender` has withdrawn less, but these | ||
/// @audit were actually fees: | ||
ethAmountOwed = totalOwed - previousWithdrawnAmount; | ||
} | ||
return (ethAmountOwed + previousWithdrawnAmount, ethAmountOwed); | ||
} | ||
``` | ||
|
||
In doing so, the `LidoVault` believes the claimant to have a greater claim to the vault, when in actuality these additional claims are representative of protocol fees. | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
_No response_ | ||
|
||
### Impact | ||
|
||
Variable size withdrawals receive undue claims to fee shares. | ||
|
||
### PoC | ||
|
||
_No response_ | ||
|
||
### Mitigation | ||
|
||
Amplify the `previousWithdrawAmount` by undoing the affects of fees when we [`calculateVariableFeeEarningsShare`](https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L946C12-L946C45): | ||
|
||
```diff | ||
(uint256 currentState, uint256 feeEarningsShare) = calculateVariableWithdrawState( | ||
feeEarnings + withdrawnFeeEarnings, | ||
- variableToWithdrawnFees[msg.sender] | ||
+ variableToWithdrawnFees[msg.sender].mulDiv(10000, 10000 - protocolFeeBps) | ||
); | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
Scrawny Iron Tiger | ||
|
||
High | ||
|
||
# Locked Ether Accumulation in Vault After Withdrawal Process | ||
|
||
## Summary | ||
In the provided contract, there is an issue with locked Ether accumulating in the vault after users withdraw their funds. This happens because fractional amounts of Ether resulting from the calculation involving fixedBearerToken[msg.sender], vaultEndedFixedDepositsFunds, and fixedLidoSharesTotalSupply() are effectively being locked and cannot be withdrawn by the users. Over time, this will accumulate in the vault, and if multiple vaults are created, the amount of Ether locked in the system could grow substantially. | ||
|
||
## Vulnerability Detail | ||
sendAmount = fixedBearerToken[msg.sender].mulDiv( | ||
vaultEndedFixedDepositsFunds, | ||
fixedLidoSharesTotalSupply() | ||
); | ||
|
||
Each user can withdraw only the integer part of this calculation, and the remainder is effectively lost and accumulated in the vault. Over time, as more users withdraw and as more vaults are created, these small locked amounts can accumulate into significant values of Ether that are permanently locked. | ||
|
||
## Impact | ||
Loss of Funds: Users may lose fractional amounts of Ether on each withdrawal, leading to a cumulative loss of Ether in the vault. | ||
Locked Ether: Over time, the vault will accumulate non-negligible amounts of Ether, which will be locked and inaccessible to users. | ||
System Inefficiency: Ether that is locked in the vault cannot be used or withdrawn, reducing the efficiency of the system. | ||
|
||
## Code Snippet | ||
|
||
https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L748-L751 | ||
|
||
## Tool used | ||
|
||
Manual Review | ||
|
||
## Recommendation | ||
Consider to make function to move rest ethers to lido. | ||
You can use selfdestruct function. | ||
At the end of vault scenario, owner of vault can call selfdestruct(address(lido)) and system can collect rest ethers. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
Chilly Amethyst Griffin | ||
|
||
High | ||
|
||
# wrong equation when calculating the profit | ||
|
||
### Summary | ||
|
||
in LidoVault function withdraw line 528 the function **calculateVariableWithdrawState** takes two argument **totalEarnings** and **previousWithdrawnAmount** and based on that and the user **variableBearerToken** it calculates the amount of earning and the problem is that when calculating **totalEarnings** it uses `(lidoStETHBalance.mulDiv(currentStakes + withdrawnStakingEarningsInStakes, currentStakes)` where lidoStETHBalance is the StETH amount of the contract **currentStakes** is the ldo share of the contract and **withdrawnStakingEarningsInStakes** is the previous requested amount of shares and it adds **currentStakes** and **withdrawnStakingEarningsInStakes** but instead it should subtract in order to not count the previous withdrawal as a profit but in here it is adding which will even add more profit | ||
|
||
### Root Cause | ||
|
||
in LidoVault.sol:528 it adds currentStakes + withdrawnStakingEarningsInStakes instead it should subtract | ||
https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L527 | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
_No response_ | ||
|
||
### Impact | ||
|
||
loss of fund as it give a false profit and based on that it calculates the amount to give for user | ||
|
||
### PoC | ||
|
||
_No response_ | ||
|
||
### Mitigation | ||
|
||
subtract instead of adding |
Oops, something went wrong.