Y2k Finance contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

Depeg event can still happen when the price of a pegged asset is equal to the strike price of a Vault which is incorrect.

This docs clearly mentions:

"When the price of a pegged asset is below the strike price of a Vault, a Keeper(could be anyone) will trigger the depeg event and both Vaults(hedge and risk) will swap their total assets with the other party." - https://code4rena.com/contests/2022-09-y2k-finance-contest

Proof of Concept

1. Assume strike price of vault is 1 and current price of pegged asset is also 1

2. User calls triggerDepeg function which calls isDisaster modifier to check the depeg eligibility

3. Now lets see isDisaster modifier

modifier isDisaster(uint256 marketIndex, uint256 epochEnd) {
        address[] memory vaultsAddress = vaultFactory.getVaults(marketIndex);
        if(
            vaultsAddress.length != VAULTS_LENGTH
            )
            revert MarketDoesNotExist(marketIndex);

        address vaultAddress = vaultsAddress[0];
        Vault vault = Vault(vaultAddress);

        if(vault.idExists(epochEnd) == false)
            revert EpochNotExist();

        if(
            vault.strikePrice() < getLatestPrice(vault.tokenInsured())
            )
            revert PriceNotAtStrikePrice(getLatestPrice(vault.tokenInsured()));

        if(
            vault.idEpochBegin(epochEnd) > block.timestamp)
            revert EpochNotStarted();

        if(
            block.timestamp > epochEnd
            )
            revert EpochExpired();
        _;
    }

4. Assume block.timestamp is at correct timestamp (between idEpochBegin and epochEnd), so none of revert execute. Lets look into the interesting one at

        if(
            vault.strikePrice() < getLatestPrice(vault.tokenInsured())
            )
            revert PriceNotAtStrikePrice(getLatestPrice(vault.tokenInsured()));

5. Since in our case price of vault=price of pegged asset so if condition does not execute and finally isDisaster completes without any revert meaning go ahead of depeg

6. But this is incorrect since price is still not below strike price and is just equal

Recommended Mitigation Steps

Change the isDisaster modifier to revert when price of a pegged asset is equal to the strike price of a Vault

if(
            vault.strikePrice() <= getLatestPrice(vault.tokenInsured())
            )
            revert PriceNotAtStrikePrice(getLatestPrice(vault.tokenInsured()));

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L96

Vulnerability details

Impact

Fee on transfer not considered on asset token at deposit function. This means users will mint more tokens than the funds which were actually deposited by user.

Proof of Concept

1. Assume asset token charges 10% fees on transfer
2. User calls the deposit function

function deposit(
        uint256 id,
        uint256 assets,
        address receiver
    ) public virtual returns (uint256 shares) {
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(id, assets)) != 0, "ZERO_SHARES");

        // Need to transfer before minting or ERC777s could reenter.
        asset.safeTransferFrom(msg.sender, address(this), assets);

        _mint(receiver, id, shares, EMPTY);

        emit Deposit(msg.sender, receiver, id, assets, shares);

        afterDeposit(id, assets, shares);
    }

3. deposit is called with amount 100

4. Below line transfer funds to contract. Since 10% is fees so contract receive only 100-10% = 90

asset.safeTransferFrom(msg.sender, address(this), assets);

5. Even though contract received amount 90, the user gets mint of amount worth 100 only, hence a loss to the contract

require((shares = previewDeposit(id, assets)) != 0, "ZERO_SHARES");
_mint(receiver, id, shares, EMPTY);

Recommended Mitigation Steps

Compute the balance before and post transfer and subtract them to get actual received amount

uint256 before = asset.balanceOf(address(this));
asset.safeTransferFrom(msg.sender, address(this), assets);
uint256 after = asset.balanceOf(address(this));
assets=after-before;
require((shares = previewDeposit(id, assets)) != 0, "ZERO_SHARES");

Lines of code

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

Vulnerability details

Impact

The recoverERC20 function only checks tokenAddress != address(stakingToken) which means Admin at any time is allowed to extract the Reward token.

Proof of Concept

1. Observe the recoverERC20 function

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);
    }

2. Observe the only restriction placed on owner is that token!=stakingToken and Owner is free to steal the reward token from this contract

Recommended Mitigation Steps

Revise the function as below:

function recoverERC20(address tokenAddress, uint256 tokenAmount)
        external
        onlyOwner
    {
        require(
            tokenAddress != address(stakingToken) && tokenAddress != address(rewardsToken),
            "Cannot withdraw the staking token"
        );
}

Lines of code

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

Vulnerability details

Impact

The contract only checks if balanceOf rewardsToken is greater than or equal to the future rewards. The current checks are not sufficient to make sure the contract has enough amount of rewardsToken.

Proof of Concept

1. User A deposits 1,000 stakingToken
2. rewardsDistribution sends 100 rewardsToken to the contract;
3. rewardsDistribution calls notifyRewardAmount() with amount = 100;
4. User has earned some reward say 100 rewardToken after X days which he has not withdrawn
5. rewardsDistribution calls notifyRewardAmount() with amount = 100 without send any fund to contract, the tx will succees;
6. X days later, User again earns 100 reward token making a total of 200 rewardToken
7. User calls earned() to get his 200 rewardsToken, but the transaction will fail due to insufficient balance of rewardsToken.

Recommended Mitigation Steps

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

Reference

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

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/oracles/PegOracle.sol#L63

Vulnerability details

Impact

In latestRoundData function, Validations are not performed over priceFeed1.latestRoundData()

Proof of Concept

1. In latestRoundData function, priceFeed1.latestRoundData() is used to retrieve the price
2. This is missing necessary checks like below

require(price1 > 0, "Chainlink price <= 0");
        require(
            answeredInRound1 >= roundID1,
            "RoundID from Oracle is outdated!"
        );
        require(timeStamp1 != 0, "Timestamp == 0 !");

Recommended Mitigation Steps

A function getOracle1_Price function is already implemented having required checks. Seems like this was meant to call getOracle1_Price function only which was mistakenly missed

int256 price1 = getOracle1_Price();

Lines of code

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

Vulnerability details

Impact

If there is no deposit for sometime in start then reward for those period is never used

Proof of Concept

1. Admin has added reward which made reward rate as 10 reward per second using notifyRewardAmount function

rewardRate = reward.div(rewardsDuration);

2. For initial 10 seconds there were no deposits which means total supply was 0
3. So no reward were distributed for initial 10 seconds and reward for this duration which is 10*10=100 will remain in contract
4. Since on notifying contract of new rewards, these stuck rewards are not considered so these 100 rewards will remain in contract with no usage

Recommended Mitigation Steps

On very first deposit better to have (block.timestamp-startTime) * rewardRate amount of reward being marked unused which can be used in next notifyrewardamount

setRewardsDuration allows 0 duration

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

Issue: The contract currently allows Owner to set duration as 0 which means notifyRewardAmount will always fail (division by zero). Also there seems to be no min/max cap on reward duration which highly impacts how much rewards are distributed

rewardRate = reward.div(rewardsDuration);

Recommendation: Do not allow 0 reward duration and also set a min/max cap for reward duration

No check to see if stakingToken==rewardsToken

Contract: https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/StakingRewards.sol#L82

Issue: The constructor has currently no check to see if stakingToken==rewardsToken. This could cause blunder where user staked token will be sent out as rewards

Recommendation: Add below check in constructor

require(_stakingToken!=_rewardsToken);

No market exist on index 0

Contract: https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L195

Issue: In createNewMarket function, The first market index starts with 1 as seen at VaultFactory.sol#L195. However checks in function like deployMoreAssets fails to consider this and only checks index > marketIndex which means 0 will be considered as valid market index when it is not

Recommendation: Consider adding one more check index!=0 to verify for correct market index