pash0k - AbstractStakingAM.sol::_getRewardBalances() will fail and lead to loss of rewards and DOS

[sherlock-admin]

pash0k

high

AbstractStakingAM.sol::_getRewardBalances() will fail and lead to loss of rewards and DOS

Summary

In the current implementation of AbstractStakingAM.sol it is assumed that in underlying staking contract the rewards are claimed only when _withdraw() or _claimReward() are called, which is not true for StakedStargateAM.sol. This will lead to constant reverts of _getRewardBalances(), as _getCurrentReward() will return zero, while assetState_.lastRewardGlobal will be non-zero, so uint256 deltaReward = currentRewardGlobal - assetState_.lastRewardGlobal will underflow and revert.

Vulnerability Detail

In AbstractStakingAM.sol::mint() we see that assetState[asset] is updated with value returned from _getRewardBalances() and after that _stake() is called. _getRewardBalances() calls StakedStargateAM.sol::_getCurrentReward(), which then calls Stargate's LPStakingTime.sol::pendingEmissionToken(). Let's take a look at it.

    function pendingEmissionToken(uint256 _pid, address _user) external view returns (uint256) {
        PoolInfo storage pool = poolInfo[_pid];
        UserInfo storage user = userInfo[_pid][_user];
        uint256 accEmissionPerShare = pool.accEmissionPerShare;
        uint256 lpSupply = pool.lpToken.balanceOf(address(this));
        if (block.timestamp > pool.lastRewardTime && lpSupply != 0 && totalAllocPoint > 0) {
            uint256 multiplier = getMultiplier(pool.lastRewardTime, block.timestamp);
            uint256 tokenReward = multiplier.mul(eTokenPerSecond).mul(pool.allocPoint).div(totalAllocPoint);
            accEmissionPerShare = accEmissionPerShare.add(tokenReward.mul(1e12).div(lpSupply));
        }
        return user.amount.mul(accEmissionPerShare).div(1e12).sub(user.rewardDebt);
    }

When StakedStargateAM.sol::_stake() is called, it then calls Stargate's LPStakingTime.sol::deposit(). Let's take a look at it.

    function deposit(uint256 _pid, uint256 _amount) external {
        PoolInfo storage pool = poolInfo[_pid];
        UserInfo storage user = userInfo[_pid][msg.sender];
        updatePool(_pid);
        if (user.amount > 0) {
            uint256 pending = user.amount.mul(pool.accEmissionPerShare).div(1e12).sub(user.rewardDebt);
            safeTokenTransfer(msg.sender, pending);
        }
        pool.lpToken.safeTransferFrom(address(msg.sender), address(this), _amount);
        user.amount = user.amount.add(_amount);
        user.rewardDebt = user.amount.mul(pool.accEmissionPerShare).div(1e12);
        lpBalances[_pid] = lpBalances[_pid].add(_amount);
        emit Deposit(msg.sender, _pid, _amount);
    }

We see that in deposit function pending rewards are calculated, transferred to the user and user.rewardDent is updated. So, if we call deposit() and then call pendingEmissionToken(), we will get zero as pending rewards, as all of them were claimed in deposit().

Now let's check the following scenario:

1. Some funds are deposited to the contract via StakedStargateAM.sol::mint(). Now the position in Stargate's contract is non-zero, pending rewards are also non-zero. Now let's assume some time has passed and StakedStargateAM.sol::_getCurrentReward() it returns X, which is more than 0.

2. StakedStargateAM.sol::mint() is called again, then AbstractStakingAM.sol::_getRewardBalances(), currentRewardGlobal is X, assetState_.lastRewardGlobal is zero, so assetState_.lastRewardGlobal is updated to X.

uint256 currentRewardGlobal = _getCurrentReward(positionState_.asset);

assetState_.lastRewardGlobal = SafeCastLib.safeCastTo128(currentRewardGlobal);

Later in mint() function:

assetState[asset] = assetState_;

Now, assetState.lastRewardGlobal is X, which is more than zero.

Then, in mint() function _stake() is called, which, as mentioned previously, claims all pending rewards.

3. Now, if AbstractStakingAM.sol::_getRewardBalances() is called, currentRewardGlobal will be less than X for a certain period of time (while rewards are accumulating), while assetState_.lastRewardGlobal is equal to X > 0. So,

uint256 deltaReward = currentRewardGlobal - assetState_.lastRewardGlobal;

reverts due to underflow.

Steps (2) and (3) of this scenario can be repeated many times, given that current pending rewards are non-zero. More than that, calls to increaseLiquidity(), instead of mint(), will lead to similar results.

Impact

This bug has several severe impacts:

1. As AbstractStakingAM.sol::_getRewardBalances() is used in calculating current value of staking asset module, for some period of time the value of many accounts` collaterals will be impossible to calculate due to reverts. (All accounts that have staked Stargate LPs in their collateral).

2. AbstractStakingAM.sol::_getRewardBalances() is used during withdrawal of funds from the contract, so it will also revert and users will not be able to get their staked assets or claim rewards. This DOS may last for more than a week, depending on the value of X and the value of staked assets.

3. As mentioned earlier, during calls to StakedStargateAM.sol::_stake() rewards are claimed, but they are not counted towards the users in AbstractStakingAM.sol::mint() and AbstractStakingAM.sol::increaseLiquidity(). This funds will be forever locked on the contract's balance. So, users lose their rewards from staking Stargate LPs, and rewards are the whole point of staking. As these rewards are from an outside integration, they can be counted as user's funds.

As mint() and increaseLiquidity() are expected to be often called by users, this problem will occur again and again, even if no malicious user try to specifically cause it.

So, regular DOS and loss of funds are main impacts of this vulnerability.

Code Snippet

AbstractStakingAM.sol::mint()

AbstractStakingAM.sol::_getRewardBalances()

Stargate's LPStakingTime.sol

Tool used

Manual Review

Recommendation

Account for claimed rewards before calls to _stake() as it is done before calls to _withdraw() in, for example, AbstractStakingAM.sol::decreaseLiquidity().

Duplicate of #38

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: high(1)