# Audit Recap #2: Olympus DAO *We regularly publish short recaps on a decentralized audit in which we participated. This time, we cover the audit of Olympus DAO's "Single Sided Liquidity Vaults".* ![](https://i.imgur.com/PCYS4PE.png) *brainbot is a web3 service provider, offering consulting and development services as well as [smart contract audits](https://brainbot.com/smart-contract-audits/). To gain more experience in auditing, our security researchers regularly participate in [decentralized audits](https://https://medium.com/@brainbot/decentralised-audits-are-here-to-stay-bcee6d1118a8). In this series, we will publish recaps of audits in which we participated in order to provide some insight into the functioning, the smart contract architecture and our findings for the respective protocols.* ## Olympus DAO The goal of this content is to give insight on the project and structure of the code as audited during the [Sherlock contest of OlympusDAO](https://app.sherlock.xyz/audits/contests/50), as well as a couple of interesting findings. OlympusDAO intends to create a non-pegged stable coin called OHM. In this audit, we were interested by what they called the `SingleSidedLiquidityVault`. The idea behind it is to create a vault bound to an AMM liquidity pool (e.g. Balancer), and let users deposit `pair tokens` (e.g. wstETH) into the vault. The vault will then mine OHM tokens matching the USD value of pair tokens deposited by the user and provide liquidity to the pool in a 50/50 OHM/pair token value split. The vault can deposit the LP tokens it gets from the pool and stake them to farm reward (e.g. in Aura). The idea behind this is to stabilize the price of OHM compared to the pair token, as explained in their short economic brief: "SSLVs should dampen OHM volatility relative to the counter-asset. As OHM price increases relative to the counter-asset, OHM that was minted into the vault is released into circulation outside the control and purview of the protocol. This increases circulating supply and holding all else equal should push the OHM price back down. As OHM price decreases relative to the counter-asset, OHM that was previously circulating has now entered the liquidity pool where the protocol has a claim on the OHM side of the pool. This decreases circulating supply and holding all else equal should push the OHM price back up." The vault uses internal and external reward tokens to reward the users providing liquidity. Internal reward tokens are reward tokens only used by the vault and for which the vault handles accounting An external reward token is a token for which the source of reward comes from outside the vault (e.g. Aura tokens), and the vault is responsible for harvesting the rewards and distributing them. ### Scope The scope of the audit contained contracts responsible for handling the minting / burning of the OHM tokens by the vault (mainly access control) as well as an `OlympusLiquidityRegistry` used to register deployed vaults seemingly for accounting. ### Approach I do not believe these contracts require much attention as they should not have important flaws, so I will not spend time describing them. Instead I will focus on the core contracts for this audit: `SingleSidedLiquidityVault.sol` providing the abstract base for every SSLV, and `WstethLiquidityVault.sol` the specific implementation of an SSLV for wstETH using a Balancer pool. ## SSLV The abstract SingleSidedLiquidityVault provides the usual access controlled functions to set up the contract. The deployer of the contract defines the pair token, the underlying pool, and the address of the `kernel` used to link the contract to the the minter, burner, and registry. The admin of the contract (i.e. a DAO) is responsible for setting the limit, threshold, and fee parameters. The limit represents the maximum amount of OHM the vault is allowed to mint. The threshold is used to restrict the vault actions when the pool price and an oracle price (defined by the implementer contract) differ by more than threshold. The fee parameter is used to cut a fee from the reward tokens sent to the user. The admin can activate / deactivate the vault in case of problems, it can add and remove reward tokens, claim the fees, and rescue the tokens owned by the contract (i.e. withdraw them) in case of an emergency. ### Deposit Users start interacting with the vault through the `deposit(amount, slippageParams)` function. The deposit function does the following: - computes the amount of OHM the vault will mint by matching the USD value of the amount of pair token provided. - caches current pair token and OHM balance - checks that the underlying pool's price is not too far from an oracle's price as a safety measure and that the ohm to be minted respects the limit parameter. - update the reward state for internal and external reward token - take the pair token from the user - mint the corresponding amount of OHM - deposits the pair token and OHM into the underlying pool - computes the amount of actually deposited tokens by comparing with the cached value - reimburse the non-deposited pair tokens to the user - burn the non-deposited OHM - updates accounting value (ohmMinted, totalLP, pairTokenDeposits[msg.sender], lpPositions[msg.sender]) - updates the values tracking the reward debt of a user for each token (userRewardDebts[msg.sender][token]) The slippage parameters are passed to the underlying pool and could represent for example the expected amount of LP token received. ### Withdraw Users can later withdraw using the `withdraw(lpAmount, minTokenAmounts, claim)` function. The lpAmount is the LP token amount to withdraw, the minTokenAmounts is an array of the amount of OHM / pair token expected to receive from the underlying pool during withdrawal, and the claim boolean specifies whether the user wants to claim its pending reward token at the same time. The withdraw function does the following: - checks that the underlying pool's price is not too far from an oracle's price as a safety measure - updates the accounting value for the reward tokens and rewards the user if `claim` is true - updates the accounting value for totalLP and lpPositions[msg.sender] - withdraw the tokens from the liquidity pool - reduces the value for pairTokenDeposits[msg.sender], ohmMinted and ohmRemoved according to amount of tokens received from the underlying pool - burn the OHM received - pay back the pair tokens to the user Note that the user only receives the pair token part of the tokens withdrawn from the pool. ### Reward tokens Users can call `claimRewards()` to withdraw their pending reward tokens from the vault. Internal reward tokens are represented with the following struct: ```solidity struct InternalRewardToken { address token; uint256 decimalsAdjustment; uint256 rewardsPerSecond; uint256 lastRewardTime; uint256 accumulatedRewardsPerShare; } ``` External reward tokens use this struct: ```solidity struct ExternalRewardToken { address token; uint256 decimalsAdjustment; uint256 accumulatedRewardsPerShare; uint256 lastBalance; } ``` For internal reward tokens, the accumulatedRewardsPerShare should be increased by `((now - lastRewardTime) * rewardsPerSecond) / totalLP` whenever the totalLP value changes or this value is required. For external reward tokens, `accumulatedRewardsPerShare` should be computed with help from the external token. Additionally, to keep track of the rewards for each token and user the vault stores `userRewardDebts[user][token]` and `cachedUserRewards[user][token]`. The accounting of reward tokens is hard to follow through and ultimately broken, I will only describe it as part of an issue description, but I believe the goal was to save in `userRewardDebts` the reward prior to a user being entitled to reward so that at any point, the user should be able to receive `token.accumulatedRewardsPerShare * userLP - userRewardDebts`. The reason for `cachedUserRewards` seem to be to store the calculated reward for a user that withdrew from the protocol but did not claim their rewards. ## Findings In this section I'll describe a couple of interesting vulnerabilities found for this audit. The first one that I found myself, the second one that I was surprised got accepted as a valid medium severity, and a third one that I missed and was found by other auditors. ### Vault withdraw vulnerable to flash loans The function from `WstethLiquidityVault` to withdraw from the pool will exit the the Balancer pool and get OHM / wstETH in proportion to the current pool balances. If the withdrawer flash loans from the pool before, they can impact the proportion of OHM / wstETH received. As the `withdraw()` function from SingleSidedLiquidityVault will repay (burn) the OHM token to the protocol and only send the pair token (wstETH) to the user, the user is incentivised to flash loan OHM from the pool so he receives more pair token (wstETH). ```solidity function withdraw( uint256 lpAmount_, uint256[] calldata minTokenAmounts_, bool claim_ ) external onlyWhileActive nonReentrant returns (uint256) { ... if (!_isPoolSafe()) revert LiquidityVault_PoolImbalanced(); _withdrawUpdateRewardState(lpAmount_, claim_); totalLP -= lpAmount_; lpPositions[msg.sender] -= lpAmount_; // Withdraw OHM and pairToken from LP (uint256 ohmReceived, uint256 pairTokenReceived) = _withdraw(lpAmount_, minTokenAmounts_); // Reduce deposit values uint256 userDeposit = pairTokenDeposits[msg.sender]; pairTokenDeposits[msg.sender] -= pairTokenReceived > userDeposit ? userDeposit : pairTokenReceived; ohmMinted -= ohmReceived > ohmMinted ? ohmMinted : ohmReceived; ohmRemoved += ohmReceived > ohmMinted ? ohmReceived - ohmMinted : 0; // Return assets _repay(ohmReceived); pairToken.safeTransfer(msg.sender, pairTokenReceived); ... } ``` This attack vector is limited by the `_isPoolSafe()` function called at the beginning of withdraw() which checks that the on-chain pool price is not more than THRESHOLD away from the oracle price. Depending on the value of THRESHOLD, the impact is more or less severe. If THRESHOLD represents 1%, then the user will be able to get 1% more tokens out of the withdraw than they should. An attacker could repeatedly deposit / flash loan / withdraw / reimburse flash loan to make a profit until the price of the pool becomes naturally too far from the oracle price. The attacker can then sell their gained pair tokens which will impact the oracle price to lower the price of pair token and attack again. This vulnerability is a high severity vulnerability that relies on the combination of price oracle manipulation / flash loans and the understanding of the logic of the contract which repays only the pair token to the user. ### Discrepancy between getMaxDeposit and _canDeposit The public view function `getMaxDeposit()` that informs users about the max deposit they can do does not match the internal function `_canDeposit()` that will actually revert if the deposit is too high. ```solidity function getMaxDeposit() public view returns (uint256) { uint256 currentPoolOhmShare = _getPoolOhmShare(); uint256 emitted; // Calculate max OHM mintable amount if (ohmMinted > currentPoolOhmShare) emitted = ohmMinted - currentPoolOhmShare; uint256 maxOhmAmount = LIMIT + ohmRemoved - ohmMinted - emitted; // Convert max OHM mintable amount to pair token amount uint256 ohmPerPairToken = _valueCollateral(1e18); // OHM per 1 pairToken uint256 pairTokenDecimalAdjustment = 10**pairToken.decimals(); return (maxOhmAmount * pairTokenDecimalAdjustment) / ohmPerPairToken; } function _canDeposit(uint256 amount_) internal view virtual returns (bool) { if (amount_ + ohmMinted > LIMIT + ohmRemoved) revert LiquidityVault_LimitViolation(); return true; } ``` The public function `getMaxDeposit()` uses an `emitted` value representing the amount of OHM minted by the protocol not currently handled by the vault. It reduces the max OHM amount by this emitted value. The `_canDeposit()` check does not use such a value. If `getMaxDeposit()` is used in the UI to inform the user of its deposit limit, the user will be receive erroneous values. This bug is a contract logic bug with a low impact. I was surprised that it was accepted as a valid medium during the audit contest. ### Infinite loop due to continue before increment The function to accumulate external rewards in `WstethLiquidityVault` is as follow: ```solidity function _accumulateExternalRewards() internal override returns (uint256[] memory) { uint256 numExternalRewards = externalRewardTokens.length; auraPool.rewardsPool.getReward(address(this), true); uint256[] memory rewards = new uint256[](numExternalRewards); for (uint256 i; i < numExternalRewards; ) { ExternalRewardToken storage rewardToken = externalRewardTokens[i]; uint256 newBalance = ERC20(rewardToken.token).balanceOf(address(this)); // This shouldn't happen but adding a sanity check in case if (newBalance < rewardToken.lastBalance) { emit LiquidityVault_ExternalAccumulationError(rewardToken.token); continue; } rewards[i] = newBalance - rewardToken.lastBalance; rewardToken.lastBalance = newBalance; unchecked { ++i; } } return rewards; } ``` It uses unchecked arithmetic to increment the value of the loop counter `i` to save gas. However, under the condition that `newBalance < rewardToken.lastBalance`, the loop will emit an event and `continue` without increasing the value of `i`. This results in an infinite loop. This function is called during deposit, withdraw, and claiming rewards. If the condition is met and the loop is infinite, the transaction will run out of gas and revert, locking the protocol in an unusable state. However, as noted in the comments by the protocol, this condition can never be met, which adds to my surprise for considering this issue valid. This bug does not require understanding of the protocol (or very little) to be found, which is why I find it interesting even though I would personally not consider it valid in this case. ## What's next? We'll continue to regularly publish audit recaps for different protocols. Meanwhile, you can also have a look at our other publications on [Medium](https://medium.com/@brainbot). You can also find our previous audit recaps on our [overview page](https://hackmd.io/@brainbot-services). ## Hard Facts **Decentralized Audit Platform:** [Sherlock](https://www.sherlock.xyz) **Audited Protocol:** [Olympus DAO](https://app.sherlock.xyz/audits/contests/50) **Security Researcher:** Côme du Crest (ranked 4/154 in this contest)