Introduction & Scope
This audit looks into Pull Request 103 as seen here. This includes the following contracts:
As SSLV can affect the overall OHM supply, its design itself may have unexpected consequences for the economics of OHM ecosystem. As the authors of this report lack the expertise and proper information to audit the economic design of the system, this audit should not be treated as a design audit.
1. [MED] Wrong decimal value for stETH/USD price feed in
StethLiquidityVault.sol, line 215, a comment affirms that the oracle price feed for stETH/USD reports a price in 18 decimals, after double checking it, we noticed this was not accurate, and that the price was in fact being reported in 8 decimals. Incorrectly assuming the decimal count of a feed would complicate the calculations to come after contained in the same function, more specifically the math for the return value in line 223.
- Recommendation: Change the calculations to take into account that stETH/USD price feed reports in 8 decimals.
safeTransfer not present
rewardTokentokens might not be standard ERC20 tokens that revert on failure (they may return false and silently pass as opposed to more common way of reverting) or they may not return a boolean like USDT, consider utilizing a
SafeERC20library for token transfers throughout the contract to account for non ERC20 compliant tokens.
- Recommendation: Use
transferFromwhile handling tokens other than
OHM, especially in the abstract contract as the tokens are not known beforehand. See these related C4 issues.
3. [HIGH] Faulty math in
_canDeposit leads to Denial of Service
- Due to faulty math, the sanity check
ohmMinted - ohmBurned + amount_ > LIMITin both branches as the variable
ohmMinted - ohmBurnedin each of the branches. As Solidity >0.8 reverts when a
uintis evaluated as negative, the states in which
ohmBurned > ohmMintedresults in
_canDepositreverting. As the function
depositis the only path that can increase the variable
_canDepositsanity check, once the contract gets into a state in which
ohmBurned > ohmMintedwhich can occur naturally due to price fluctations in the Balancer pool, no further deposits are possible after that point, resulting in denial of service. As the only way to resume operations in such scenario is upgrading/replacing the contract, see issue 8 [INFORMATIONAL] No emergency protections present.
- Recommendation: Fix the faulty math.
deposit does not conform to checks-effects-interactions pattern
- As can be seen here, state changes happen after
deposit. This is against the safe pattern of checks-effects-interactions, and it can lead to reentrancy attacks utilizing a
pairTokenwith callback capabilities (e.g an ERC777 or a modified ERC20). Even though the
nonReentrantmodifier is used in both external functions of the contract, this modifier can only prevent reentrancy attacks caused by calling these functions mid-function, yet the attacker can manipulate the state by either tampering with the liquidity pool or dumping
OHMdirectly into the contract in the callback.
- A) Tampering with the liquidity pool
The attacker can bypass the check
_isPoolSafeby having an initially safe LP pool state to pass
_isPoolSafe, and then proceeding to manipulate the pool price in the callback from
_deposit, resulting in a deposit to an unsafe pool. The current Balancer implementation is not likely to be susceptible to this as all external state-changing functions of Balancer has the
nonReentrantmodifier, meaning that any price manipulation during the transaction would lock the Balancer vault, resulting in revert in the
_depositcall. Yet, further implementations utilizing different liquidity pool designs might be susceptible to bypassing of
_isPoolSafe, if the underlying liquidity pools don’t implement a mutex in the necessary places.
- B) Dumping
OHMdirectly into contract during execution As the variable
ohmMintedis increased by the difference in the
OHMbalance before and after the
transferFromcalls, the attacker can utilize the callback to directly send
OHMinto the contract to inflate
ohmMintedto increase less than what is supposed to. This path can be utilized to force issue 3 by inflating the difference between
ohmMinted. Other than causing denial of service by issue 3 and tampering with the variables
pairTokenDeposits, no direct exploitation possibility for this path was found in the audit.
- A) Tampering with the liquidity pool The attacker can bypass the check
- Recommendation: Refactor
depositso that it conforms to CEI pattern.
5. [MED] Truncations and unsafe casts in the rewards logic can lead to weird states such as infinite rewards
- PoC Foundry test can be found here.
- As a
MasterChefvariation, the rewards logic of SSLV includes potential truncations (division results in 0 in Solidity if numerator is smaller than denominator as Solidity does not support floats) and unsafe casts (casting a
uintis dangerous as underflow/overflow in casting is not patched after Solidity >0.8). While our research didn’t yield a path that is directly exploitable, we found a interesting enough weird state that is accessible through a valid path in which a first depositor deposits a large amount of
totalLPstarts large, resulting in a small
accumulatedRewardsPerShare. Then a second depositor makes a deposit of usual
1e18, and proceeds to withdraw only
1. As rewards are calculated as in the snippet below in
userRewardDebts[user_][rewardToken.token]is larger than
int256((lpPositions[user_] * accumulatedRewardsPerShare) / 1e18)the outermost
intvalue becomes negative, underflowing
uintand resulting in infinite rewards value. This behavior happens as the withdrawn amount truncates when its subtracted from the
_withdrawUpdateRewardStatedue to both withdrawn LP amount and rewards per share being small and their product is smaller than
1e18, which leads to
userRewardDebtsstaying the same while
int256((lpPositions[user_] * accumulatedRewardsPerShare) / 1e18)is decreased.
- Even though the above scenario highlights the problems associated with truncations and unsafe casts, it is not practically exploitable as the contract wouldn’t have an infinite amount of rewards and the fee calculation would overflow and revert. We extensively studied the contract to see if an arbitrary negative number can be reached through the means explained above, yet we couldn’t find such a path. Yet, this does not mean such a path does not exist, and thus usage of potential truncations and unsafe casts should be minimized in order to prevent potential rewards drainage paths.
- Recommendation: Minimize truncations and unsafe casts.
6. [LOW] Add sanity check when setting variables THRESHOLD and FEE, specially THRESHOLD since it breaks
- The function
setThresholdshould be bound within the parameters to stay compliant with the comment above, specially since it cannot be bigger than
PRECISION, which is hardcoded to the value of 1000, and accidentally adding another zero accidentally when using this function would break
_isPoolSafeand revert and all the functions that depend on this check to function.
- Recommendation: Implement sanity checks in potentially contract breaking setters.
7. [INFO] No emergency protections present
- This module does not have emergency protections present, and in case of a critical event there should be a way to prevent withdraws and other sensitive functions.
- There is also no way to initialize the contract with a pre-set mapping of LP positions, thus upgrading the contract with a new one as a response to a critical event is tricky. An emergency migration function can be designed as the following:
- Withdraw LP tokens from Aura.
- Transfer LP tokens to the new implementation.
- Initialize the new contract with the old state mappings.
- Deposit the LP tokens into Aura again.
- Resume operation as usual.
- Recommendation: Consider implementing permissioned emergency migration and pause/unpause functions.
8. [LOW] Event not emitted due to dead code in
- In line 228 of
SingleSidedLiquidityVault.solthere is a return statement that prevents the posterior event in line 230 to not be emitted.
- Recommendation: Swap the return line with the line firing the event.
9. [INFO] Insufficient test coverage
Upon inspection, the contract’s test unit doesn’t simulate the correct functionality of production, and has the following issues:
- The withdraw testing only applies a condition of warping time, but has no test of a condition without passing time.
- No fuzzing of deposit and withdraw amount inputs, leading to problems.
Mock contracts do not provide the correct functionality compared to mainnet deployment environment and can’t be verified:
BalancerMockVaultmock by naive token minting. This can not provide sufficient verification of Vault’s external function calls.
Recommendation: Extend the test coverage and improve the accuracy of mocks.
10. [INFO] Claim reward functions does not return value
- The functions
_claimExternalRewardsare declared to return uint256, but does not return anything.
- Recommendation: Function should return claimed tokens amount or be declared without return.
_accumulateInternalRewards does not make state changes and can be set to view
- Recommendation: Set function to be
12. [MED] Possible reentrancy in
- The claim function makes several external calls, presented for external token accumulation in _accumulateExternalRewards:
withdrawfunction is protected against reentrancy that allows claiming rewards,
claimRewardscan be claimed.
- Recommendation: Function
nonReentrantto ensure reentrancy safety.
13. [INFO] Tokens rewarding is not accounted for tokens less or more than 18 decimals
- The vault will miscalculate reward token amount if the token’s decimal is not 18. The calculation occurs in following functions: _updateInternalRewardState, _updateExternalRewardState, _depositUpdateRewardDebts, _withdrawUpdateRewardState,
- Recommendation: Make sure all reward tokens have 18 decimals, or extend the logic to support decimals other than 18.
withdraw insufficient amount input checking
- The function doesn’t fully ensure the correctness of passed inputs:
- A user is able to call withdraw with zero amount in
- A user is able to call withdraw with zero amount in
withdrawmakes external calls to Balancer and Aura, checks current token balance and makes update states to reward tokens. Which shouldn’t occur.
- Recommendation: Add sanity checks for
15. [HIGH] Vault receiving reward tokens outside
_accumulateExternalRewards from AURA pool can’t be accounted and claimed
- The accumulation is done by checking current’s balance before and after pool call rewarded upon updating reward state when depositing and withdrawing.
- According to the Aura’s contract
getRewardcan be called by anyone, passing the vault’s address:
- Assuming this is the correct pool,
getReward()call can be triggered by anyone which will result in rewards will be transferred to the vault without getting recorded as it won’t be accounted as a balance change in the function
- The vault also does not have a general token sweep function, and the only way to transfer the rewards out are through reading the recorded
accumulatedExternalRewardsvalues, thus the rewards will be stuck in the contract.
- This path can be used as a griefing attack, as it results in monetary loss for both the users of SSLV and the protocol in the form of potential fees.
- Recommendation: Change accumulation logic so that it covers rewards accumulation happening outside of