Y2k Finance contest - pashov's results

Lines of code

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

Vulnerability details

Proof of concept

The recoverERC20() method can’t be used to rug the staked tokens because it has the check

require(
            tokenAddress != address(stakingToken),
            "Cannot withdraw the staking token"
        );

but it is missing the same check for the reward tokens.

Impact

Users can lose all of their accrued rewards to the malicious/compromised owner

Recommendation

Change the require statement to

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

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/8f41433c1cd866a138e6edbc32b7570fe94643fc/src/rewards/StakingRewards.sol#L203

Vulnerability details

Proof of concept

The method only checks if the reward tokens balance is enough to distribute the newly added rewards for their duration

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

But since part of this reward tokens balance was from unclaimed rewards, this means the current check is not sufficient to prove there are enough rewards for the new rewards duration. This means that if the admin calls notifyRewardAmount() with a bigger reward amount that it has loaded in, it is possible that there won’t be enough rewards for all users to claim, because the calculations were made based on a balance that is partly from already accrued but unclaimed rewards.

Example:

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 success;
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.

Impact

This vulnerability can result in inability for a staker to claim his rewards because there is not enough reward token balance in the contract.

Recommendation

A possible solution is changing the notifyRewardAmount() function to an addReward() function that does the token transfer of reward tokens itself. Here is an example implementation:

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L297 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L63 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L96 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L119

Vulnerability details

Proof of concept

The code in Controller.sol and PegOracle.sol makes use of Chainlink’s latestRoundData() method to get prices of assets. The problem is that the checks implemented are insufficient to ensure the price returned from the oracle is not stale. Also for the latestRoundData() function in the PegOracle.sol smart contract, the call to priceFeed1.latestRoundData() is not validated at all.

Impact

If for some reason Chainlink data feed stops updating (it has happened before when Chainlink paused its $LUNA price feed for example) then the protocol will start working with a stale price.

Recommendation

Change the latestRoundData logic to the following (you can use custom errors instead require statements as well):

(roundId, rawPrice,, updatedAt, answeredInRound) = feedVariable.latestRoundData()
require(rawPrice > 0, "Chainlink price <= 0");
require(answeredInRound >= roundId, "Stale price");
require(block.timestamp - updatedAt < MAX_STALENESS_THRESHOLD_SECOND, "Stale price"); // You should decide the value of MAX_STALENESS_THRESHOLD_SECOND, for most cases 60 minutes is good enough

The code is currently missing the last check I suggested and for PegOracle.sol's latestRoundData() method it is missing all of the validations for priceFeed1.latestRoundData().

Change the code exactly the same way for all latestRoundData calls.

Wrong NatSpec comment for setClaimTVL() in Vault.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/8f41433c1cd866a138e6edbc32b7570fe94643fc/src/Vault.sol#L342

The NatSpec is saying “Function to be called after endEpoch” but it is called when Controller gets a triggerDepeg() call as well.

No need to use OpenZeppelin’s SafeMath library if using Solidity >0.8

https://github.com/code-423n4/2022-09-y2k-finance/blob/8f41433c1cd866a138e6edbc32b7570fe94643fc/src/rewards/StakingRewards.sol#L4

StakingRewards.sol uses OpenZeppelin’s SafeMath library but it doesn’t need it as after solc 0.8 there is compiler built-in over/underflow checks.