Y2k Finance contest - cccz's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L87-L99

Vulnerability details

Impact

In the deposit function of the Vault contract, the user is allowed to deposit assets when block.timestamp < idEpochBegin[id] - timewindow, but the user can only withdraw assets after triggerDepeg or triggerEndEpoch has been called. Since the isDisaster modifier of the Controller contract only allows users to call the triggerDepeg function between epochBegin and epochEnd, which means that the user's assets are locked in the contract before the insurance takes effect. If users accidentally deposit assets into the incorrect vault, they cannot withdraw the assets even if the insurance is not yet in effect.

Proof of Concept

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L87-L99 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L152-L174 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L203-L214 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Controller.sol#L101-L109

Tools Used

None

Recommended Mitigation Steps

Consider allowing users to withdraw assets before the insurance takes effect

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L87-L91

Vulnerability details

Impact

In the Vault contract, when the changeTimewindow function is called to change the timewindow variable, the epochHasNotStarted modifier is affected, thereby affecting the deposit opening window. Consider the following scenario timewindow = 100 EpochBegin = 1000 At time 900, user A observes that the ratio of assets in insrVault to riskVault is 1:5. User A likes this ratio and thinks that the ratio will not change in the future, so user A chooses to deposit assets into insrVault. However, the changeTimewindow function changes the timewindow to 50, users have a longer time window to deposit, and the ratio of assets in insrVault to riskVault reaches 1:1, which breaks user A's expectations.

Proof of Concept

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L87-L91 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L287-L289

Tools Used

None

Recommended Mitigation Steps

Consider saving the current timewindow in the createAssets function and use the saved timewindow in the epochHasNotStarted modifier for calculations

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/rewards/StakingRewards.sol#L213-L223

Vulnerability details

Impact

Similar to https://github.com/code-423n4/2022-02-concur-findings/issues/210 StakingRewards.recoverERC20 rightfully checks against the stakingToken being sweeped away. However, there’s no check against the rewardsToken. This is the case of an admin privilege, which allows the owner to sweep the rewards tokens, perhaps as a way to rug depositors.

    function recoverERC20(address tokenAddress, uint256 tokenAmount)
        external
        onlyOwner
    {
        require(
            tokenAddress != address(stakingToken),
            "Cannot withdraw the staking token"
        );
        ERC20(tokenAddress).safeTransfer(owner, tokenAmount);
        emit Recovered(tokenAddress, tokenAmount);
    }

Proof of Concept

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/rewards/StakingRewards.sol#L213-L223

Tools Used

None

Recommended Mitigation Steps

Add an additional check

        require(
            tokenAddress != address(rewardsToken),
            "Cannot withdraw the rewards token"
        );

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/rewards/StakingRewards.sol#L201-L205

Vulnerability details

Impact

Similar to https://github.com/code-423n4/2022-02-concur-findings/issues/209

        uint256 balance = rewardsToken.balanceOf(address(this));
        require(
            rewardRate <= balance.div(rewardsDuration),
            "Provided reward too high"
        );

In the current implementation, the contract only checks if balanceOf rewardsToken is greater than or equal to the future rewards.

However, under normal circumstances, since users can not withdraw all their rewards in time, the balance in the contract contains rewards that belong to the users but have not been withdrawn yet. This means the current checks can not be sufficient enough to make sure the contract has enough amount of rewardsToken.

As a result, if the rewardsDistribution mistakenly notifyRewardAmount with a larger amount, the contract may end up in a wrong state that makes some users unable to claim their rewards.

Given:

• rewardsDuration = 7 days;

1. Alice stakes 1,000 stakingToken;
2. rewardsDistribution sends 100 rewardsToken to the contract;
3. rewardsDistribution calls notifyRewardAmount() with amount = 100;
4. 7 days later, Alice calls earned() and it returns 100 rewardsToken, but Alice choose not to getReward() for now;
5. rewardsDistribution calls notifyRewardAmount() with amount = 100 without send any fund to contract, the tx will succees;
6. 7 days later, Alice calls earned() 200 rewardsToken, when Alice tries to call getReward(), the transaction will fail due to insufficient balance of rewardsToken.

Expected Results:

The tx in step 5 should revert.

Proof of Concept

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/rewards/StakingRewards.sol#L201-L205

Tools Used

None

Recommended Mitigation Steps

Consider changing the function notifyRewardAmount to addRward and use transferFrom to transfer rewardsToken into the contract:

function addRward(uint256 reward)
    external
    updateReward(address(0))
{
    require(
        msg.sender == rewardsDistribution,
        "Caller is not RewardsDistribution contract"
    );

    if (block.timestamp >= periodFinish) {
        rewardRate = reward / rewardsDuration;
    } else {
        uint256 remaining = periodFinish - block.timestamp;
        uint256 leftover = remaining * rewardRate;
        rewardRate = (reward + leftover) / rewardsDuration;
    }

    rewardsToken.safeTransferFrom(msg.sender, address(this), reward);

    lastUpdateTime = block.timestamp;
    periodFinish = block.timestamp + rewardsDuration;
    emit RewardAdded(reward);
}

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/rewards/StakingRewards.sol#L183-L195

Vulnerability details

Impact

Similar to https://github.com/code-423n4/2022-02-concur-findings/issues/183.

        if (block.timestamp >= periodFinish) {
            rewardRate = reward.div(rewardsDuration);
        } else {
            uint256 remaining = periodFinish.sub(block.timestamp);
            uint256 leftover = remaining.mul(rewardRate);
            rewardRate = reward.add(leftover).div(rewardsDuration);
        }

The StakingRewards.notifyRewardAmount function receives a reward amount and extends the current reward end time to now + rewardsDuration. It rebases the currently remaining rewards + the new rewards (reward + leftover) over this new rewardsDuration period. This can lead to a dilution of the reward rate and rewards being dragged out forever by malicious new reward deposits.

Proof of Concept

Imagine the current rewardRate is 1000 rewards / rewardsDuration. 20% of the rewardsDuration passed, i.e., now = lastUpdateTime + 20% * rewardsDuration. A malicious actor notifies the contract with a reward of 0: notifyRewardAmount(0). Then the new rewardRate = (reward + leftover) / rewardsDuration = (0 + 800) / rewardsDuration = 800 / rewardsDuration. The rewardRate just dropped by 20%. This can be repeated infinitely. After another 20% of reward time passed, they trigger notifyRewardAmount(0) to reduce it by another 20% again: rewardRate = (0 + 640) / rewardsDuration = 640 / rewardsDuration.

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/rewards/StakingRewards.sol#L183-L195

Tools Used

None

Recommended Mitigation Steps

The rewardRate should never decrease by a notifyRewardAmount call. Consider not extending the reward payouts by rewardsDuration on every call. periodFinish probably shouldn't change at all, the rewardRate should just increase by rewardRate += reward / (periodFinish - block.timestamp).

Alternatively, consider keeping the rewardRate constant but extend periodFinish time by += reward / rewardRate.

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Controller.sol#L223

Vulnerability details

Impact

When the triggerEndEpoch function of the Controller contract is called, the assets in the insrVault will be sent to the riskVault, which also means that the tokens in the insrVault will be worthless.

insrVault.setClaimTVL(epochEnd, 0);
...
        else {
            entitledAmount = amount.divWadDown(idFinalTVL[id]).mulDivDown(
                idClaimTVL[id],
                1 ether
            );
        }

However, if the periodFinish > _epochEnd in the StakingRewards contract corresponding to the insrVault, the user can continue to stake his insrToken and receive rewards.

Proof of Concept

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Controller.sol#L223 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L418-L423 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/rewards/StakingRewards.sol#L183-L210

Tools Used

None

Recommended Mitigation Steps

Add the following code at the end of the notifyRewardAmount function of the StakingRewards contract to limit the periodFinish

         lastUpdateTime = block.timestamp;
         periodFinish = block.timestamp.add(rewardsDuration);
+       require(periodFinish <= id);

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/rewards/StakingRewards.sol#L225-L232

Vulnerability details

Impact

Similar to https://github.com/code-423n4/2022-02-concur-findings/issues/223. notifyRewardAmount will be inoperable if rewardsDuration bet set to zero. If will cease to produce meaningful results if rewardsDuration be too small or too big.

Proof of Concept

The setter do not control the value, allowing zero/near zero/enormous duration:

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/rewards/StakingRewards.sol#L225-L232

Division by the duration is used in notifyRewardAmount:

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/rewards/StakingRewards.sol#L189-L195

Tools Used

None

Recommended Mitigation Steps

Check for min and max range in the rewardsDuration setter, as too small or too big rewardsDuration breaks the logic