This audit looks into Pull Request 103 as seen here. This includes the following contracts:
This audit was conducted by kebabsec members sai, FlameHorizon and okkothejawa.
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.
StethLiquidityVault.sol
safeTransferFrom
and safeTransfer
not present_canDeposit
leads to Denial of Servicedeposit
does not conform to checks-effects-interactions pattern_isPoolSafe
withdraw
_accumulateInternalRewards
does not make state changes and can be set to viewclaimRewards
_accumulateExternalRewards
from AURA pool can’t be accounted and claimedStethLiquidityVault.sol
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.safeTransferFrom
and safeTransfer
not presentpairToken
and rewardToken
tokens 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 SafeERC20
library for token transfers throughout the contract to account for non ERC20 compliant tokens.safeTransfer
and safeTransferFrom
instead of transfer
and transferFrom
while handling tokens other than OHM
, especially in the abstract contract as the tokens are not known beforehand. See these related C4 issues._canDeposit
leads to Denial of Service_canDeposit
checks if ohmMinted - ohmBurned + amount_ > LIMIT
in both branches as the variable activeOhm
evaluates to ohmMinted - ohmBurned
in each of the branches. As Solidity >0.8 reverts when a uint
is evaluated as negative, the states in which ohmBurned > ohmMinted
results in _canDeposit
reverting. As the function deposit
is the only path that can increase the variable ohmMinted
and as deposit
utilizes _canDeposit
sanity check, once the contract gets into a state in which ohmBurned > ohmMinted
which 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.deposit
does not conform to checks-effects-interactions patterntransfer
and transferFrom
calls to pairToken
in deposit
. This is against the safe pattern of checks-effects-interactions, and it can lead to reentrancy attacks utilizing a pairToken
with callback capabilities (e.g an ERC777 or a modified ERC20). Even though the nonReentrant
modifier 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 OHM
directly into the contract in the callback.
_isPoolSafe
by having an initially safe LP pool state to pass _isPoolSafe
, and then proceeding to manipulate the pool price in the callback from pairToken.transferFrom
call before _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 nonReentrant
modifier, meaning that any price manipulation during the transaction would lock the Balancer vault, resulting in revert in the _deposit
call. 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.OHM
directly into contract during executionohmMinted
is increased by the difference in the OHM
balance before and after the transferFrom
calls, the attacker can utilize the callback to directly send OHM
into the contract to inflate unusedOhm
, causing ohmMinted
to increase less than what is supposed to. This path can be utilized to force issue 3 by inflating the difference between ohmBurned
and ohmMinted
. Other than causing denial of service by issue 3 and tampering with the variables ohmMinted
or pairTokenDeposits
, no direct exploitation possibility for this path was found in the audit.deposit
so that it conforms to CEI pattern.MasterChef
variation, 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 int
to an uint
is dangerous as underflow/overflow in casting is not patched after Solidity >0.8).pairToken
so that totalLP
starts 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 internalRewardsForToken
, once userRewardDebts[user_][rewardToken.token]
is larger than int256((lpPositions[user_] * accumulatedRewardsPerShare) / 1e18)
the outermost int
value becomes negative, underflowing uint
and resulting in infinite rewards value. This behavior happens as the withdrawn amount truncates when its subtracted from the userRewardDebts
in _withdrawUpdateRewardState
due to both withdrawn LP amount and rewards per share being small and their product is smaller than 1e18
, which leads to userRewardDebts
staying the same while int256((lpPositions[user_] * accumulatedRewardsPerShare) / 1e18)
is decreased.uint256(int256((lpPositions[user_] * accumulatedRewardsPerShare) / 1e18) - userRewardDebts[user_][rewardToken.token]);
_isPoolSafe
setThreshold
should 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 _isPoolSafe
and revert and all the functions that depend on this check to function.withdraw
SingleSidedLiquidityVault.sol
there is a return statement that prevents the posterior event in line 230 to not be emitted.Upon inspection, the contract's test unit doesn't simulate the correct functionality of production, and has the following issues:
Mock contracts do not provide the correct functionality compared to mainnet deployment environment and can't be verified:
MockAuraRewardPool
and BalancerMockVault
mock 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.
_claimInternalRewards
and _claimExternalRewards
are declared to return uint256, but does not return anything._accumulateInternalRewards
does not make state changes and can be set to viewview
.claimRewards
auraPool.rewardsPool.getReward(address(this), true);
withdraw
function is protected against reentrancy that allows claiming rewards,claimRewards
can be claimed.claimRewards
should have nonReentrant
to ensure reentrancy safety.internalRewardsForToken
and externalRewardsForToken
withdraw
insufficient amount input checkinglpAmount_
and minTokenAmounts_
.withdraw
makes external calls to Balancer and Aura, checks current token balance and makes update states to reward tokens. Which shouldn't occur.lpAmount_
and minTokenAmounts_
._accumulateExternalRewards
from AURA pool can't be accounted and claimedBaseRewardPool
implementation, getReward
can be called by anyone, passing the vault's address: function getReward(address _account, bool _claimExtras) public updateReward(_account) returns(bool){
uint256 reward = earned(_account);
if (reward > 0) {
rewards[_account] = 0;
rewardToken.safeTransfer(_account, reward);
IDeposit(operator).rewardClaimed(pid, _account, reward);
emit RewardPaid(_account, reward);
}
//also get rewards from linked rewards
if(_claimExtras){
for(uint i=0; i < extraRewards.length; i++){
IRewards(extraRewards[i]).getReward(_account);
}
}
return true;
}
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 _accumulateExternalRewards
.accumulatedExternalRewards
values, thus the rewards will be stuck in the contract._accumulateExternalRewards
.