This contest was an update following a previous contest on Sherlock in which I also participated. The code was reviewed as part of the Sherlock contest.

I ranked 2nd out of 68 participants in this audit. I found 1 high and 1 medium vulnerabilities, I present my findings here.

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.

Recommendation

Add tracking of deposits and withdraw within user vaults so that they can only withdraw what they deposited.

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

Recommendation

Use similar restrictions as in mintOhmToVault() for setLimit or lower circulatingOhmBurned when minting new OHM.