Y2k Finance contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

PegOracle.sol always returns 0 for assets with 18 decimals due to precision loss, which is a majority of all chainlink oracles

Proof of Concept

nowPrice = (price2 * 10000) / price1;

nowPrice is set to the ratio between prices scaled to 4 decimals. If the oracle return 18 decimals (as a vast majority of chainlink oracles do) then the price will not be scaled at all. When the ratio is returned, it is divided by 1,000,000. Since the ratio is only scaled to 4 decimals and is being divided 7 decimals the division will always return 0 due to precision loss. Additionally in the contest description it states that it should return as 18 decimals but in no scenario does it return the ratio to 18 decimals.

Tools Used

Recommended Mitigation Steps

PegOracle.sol#latestRoundData price calculations should be rewritten as follows:

//price is multiplied by 10**decimals to preserve the underlying decimals in the ratio
if (price1 > price2) {
    nowPrice = (price2 * (10**decimals)) / price1;
} else {
    nowPrice = (price1 * (10**decimals)) / price2;
}

//ratio is scaled to 18 decimals if not already
int256 decimals10 = int256(10**(18 - decimals));
nowPrice = nowPrice * decimals10;

return (
    roundID1,
    nowPrice,
    startedAt1,
    timeStamp1,
    answeredInRound1
);

Lines of code

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

Vulnerability details

Impact

Permanent loss of access to funds

Proof of Concept

At the end of the epoch or during a depeg event, funds are assigned from one vault to the other. In the event that this occurs in a situation where the receiving vault is empty, then funds could be sent to a vault in which there is no one to claim it. This would result in the funds being completely irretrievable. When creating vaults the admin is not required to add funds to both vaults meaning that a situation like this could arise.

Tools Used

Recommended Mitigation Steps

Vault pairs should have a override functionality that marks both vaults as ended if either of the two vaults is empty

Lines of code

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

Vulnerability details

Impact

Insurance is to protect the user in case the pegged asset drops significantly below the underlying but risk users are required to payout if the pegged asset is worth more than the underlying

Proof of Concept

    if (price1 > price2) {
        nowPrice = (price2 * 10000) / price1;
    } else {
        nowPrice = (price1 * 10000) / price2;
    }

The above lines calculates the ratio using the lower of the two prices, which means that in the scenario that the pegged asset is worth more than the underlying, a depeg event will be triggered. This is problematic for two reasons. The first is that many pegged assets are designed to maintain at least the value of the underlying. They put very strong incentives to keep the asset from going below the peg but usually use much looser policies to bring the asset down to the peg, since an upward break from the peg is usually considered benign. The second is that when a pegged asset move above the underlying, the users who are holding the asset are benefiting from the appreciation of the asset; therefor the insurance is not needed.

Because of these two reasons, it is my opinion that sellers would demand a higher premium from buyers as a result of the extra risk introduced by the possibility of having to pay out during an upward depeg. It is also my opinion that these higher premiums would push users seeking insurance to other cheaper products that don't include this risk

Tools Used

Recommended Mitigation Steps

The ratio returned should always the ratio of the pegged asset to the underlying (i.e. pegged/underlying)

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L244-L252 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/SemiFungibleVault.sol#L205-L213 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/SemiFungibleVault.sol#L237-L239 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/SemiFungibleVault.sol#L244-L246 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/SemiFungibleVault.sol#L251-L258 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/SemiFungibleVault.sol#L263-L270

Vulnerability details

Impact

Other protocols that integrate with Y2K may wrongly assume that the functions are EIP-4626 compliant. Thus, it might cause integration problems in the future that can lead to wide range of issues for both parties.

Proof of Concept

All official EIP-4626 requirements can be found on it's official page. Non-compliant functions are listed below along with the reason they are not compliant:

The following functions are missing but should be present:

1. mint(uint256, address) returns (uint256)
2. redeem(uint256, address, address) returns (uint256)

The following functions are non-compliant because they don't account for withdraw and deposit locking:

1. maxDeposit
2. maxMint
3. maxWithdraw
4. maxRedeem

All of the above functions should return 0 when their respective functions are disabled (i.e. maxDeposit should return 0 when deposits are disabled)

previewDeposit is not compliant because it must account for fees which it does not

totalAssets is not compliant because it does not always return the underlying managed by the vault because it fails to include the assets paid out during a depeg or the end of the epoch.

Tools Used

Recommended Mitigation Steps

All functions listed above should be modified to meet the specifications of EIP-4626

Lines of code

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

Vulnerability details

Impact

Staking is unable to be paused as intended

Proof of Concept

StakingRewards.sol inherits pausable and implements the whenNotPaused modifier on stake, but doesn't implement any method to actually pause or unpause the contract. Pausable.sol only implements internal functions, which requires external or public functions to be implemented to wrap them. Since nothing like this has been implemented, the entire pausing system is rendered useless and staking cannot be paused as is intended.

Tools Used

Recommended Mitigation Steps

Create simple external pause and unpause functions that can be called by owner

Lines of code

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

Vulnerability details

Impact

Fees are taken on funds deposited as collateral

Proof of Concept

uint256 feeValue = calculateWithdrawalFeeValue(entitledShares, id);

In L226 of Vault.sol#withdraw the fee is taken on the entire collateral deposited by the risk users. This is problematic for two reasons. The first is that the collateral provided by the risk users will likely be many many times higher than the premium being paid by the hedge users. This will create a strong disincentive to use the protocol because it is likely a large portion of the profits will be taking by fees and the risk user may unexpectedly lose funds overall if premiums are too low.

The second issue is that this method of fees directly contradicts project documents which clearly indicate that fees are only taken on the premium and insurance payouts, not when risk users are receiving their collateral back.

Tools Used

Recommended Mitigation Steps

Fee calculations should be restructured to only take fees on premiums and insurance payouts