code-423n4 / 2022-06-yieldy-findings

0 stars 0 forks source link

Unstaking reward tokens in liquidity reserve takes longer than it should #284

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L219

Vulnerability details

Impact

Instantly unstaking reward tokens via the liquidity reserve function LiquidityReserve.instantUnstake allows reward token owners to instantly receive staking tokens. Within this process, reward tokens (e.g. USDCy) are sent to the liquidity reserve contract (via LiquidityReserve.instantUnstake) and staking tokens (USDC) are received in return.

Those USDCy (reward tokens) are then immediately unstaked again to replenish the staking token liquidity reserve and to enable liquidity providers to remove their supplied staking token liquidity.

However, once reward tokens are unstaked (L223), those received reward tokens can only be unstaked after the cooldown period has ended.

There is no reason to only unstake reward tokens if there's no cooldown period. It even hurts the liquidity reserve as it takes 1 additional epoch to unstake and receive staking tokens and liquidity providers have to wait longer to get out their provided liquidity.

Proof of Concept

LiquidityReserve.unstakeAllRewardTokens

function unstakeAllRewardTokens() public {
    require(isReserveEnabled, "Not enabled yet");
    uint256 coolDownAmount = IStaking(stakingContract)
        .coolDownInfo(address(this))
        .amount;
    if (coolDownAmount == 0) { // @audit-info unnecessary check. This artificially extends unstaking cooldown period
        uint256 amount = IERC20Upgradeable(rewardToken).balanceOf(
            address(this)
        );
        if (amount > 0) IStaking(stakingContract).unstake(amount, false);
    }
}

The following table should demonstrate the issue:

Given a cooldown period of 1 epoch, a reward token USDCy, a staking token USDC and a liquidity reserve token lrUSDC.

Epoch Liquidity Reserve token (lrUSDC) USDCy (Reward token) USDC (Staking token) Unstaking cooldown Liquidity Reserve Comment
1 Add liquidity 10.000 0 10.000 0
1 Instant unstake 5.000 USDCy 10.000 0 5.000 5.000
1 Instant unstake 5.000 USDCy 10.000 5.000 0 5.000 Cooldown period active: no unstaking via unstakeAllRewardTokens
2 Remove 10.000 lrUSDC liquidity 10.000 5.000 0 5.000 Reverts: Insufficient staking token
2 Claim staking tokens from previous unstaking 10.000 5.000 5.000 0
2 Unstake reward tokens from liquidity reserve 10.000 0 5.000 5.000
3 Claim staking tokens from previous unstaking 10.000 0 10.000 0
3 Remove 10.000 lrUSDC liquidity 0 0 0 0

As seen, only in epoch 3 all USDC staking tokens are unstaked and available for the liquidity provider to remove.

With the recommended mitigation (mentioned below), reward tokens can be unstaked 1 epoch earlier and staking token liquidity is restored earlier which allows liquidity providers to remove their initially supplied liquidity earlier:

Epoch Liquidity Reserve token (lrUSDC) USDCy (Reward token) USDC (Staking token) Unstaking cooldown Liquidity Reserve
1 Add liquidity 10.000 0 10.000 0
1 Instant unstake 5.000 USDCy 10.000 0 5.000 5.000
1 Instant unstake 5.000 USDCy 10.000 0 0 10.000
2 Claim staking tokens from previous unstaking 10.000 0 10.000 0
2 Remove 10.000 lrUSDC liquidity 0 0 0 0

Tools Used

Manual review

Recommended mitigation steps

Unstake reward tokens immediately, which means that if there's currently a cooldown period, add the newly received reward tokens to the cooldown period:

function unstakeAllRewardTokens() public {
    require(isReserveEnabled, "Not enabled yet");

    uint256 amount = IERC20Upgradeable(rewardToken).balanceOf(
        address(this)
    );
    if (amount > 0) IStaking(stakingContract).unstake(amount, false);
}

(This is also how it was previously implemented in the old Foxy LiquidityReserve contract)

toshiSat commented 2 years ago

sponsor disputed. The reason we changed this is to prevent an infinite cooldowns for the liquidity reserve. If we unstake while the reserve currently has a cooldown it will reset the epoch of the cooldown and theoretically can never instant unstake