Y2k Finance contest - leosathya's results

Lines of code

There are 1 instance of this issue detected: > https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/oracles/PegOracle.sol#L57-L63

Vulnerability details

Impact

Price can be stale and can lead to wrong answer return value. The latestRoundData function in the contract PegOracle.sol fetches the asset price from a Chainlink aggregator using the latestRoundData function. However, there are no checks on roundID1.

Stale prices could put funds at risk. According to Chainlink's documentation, This function does not error if no answer has been reached but returns 0, causing an incorrect price fed. The external Chainlink oracle, which provides index price information to the system, introduces risk inherent to any dependency on third-party data sources. For example, the oracle could fall behind or otherwise fail to be maintained, resulting in outdated data being fed to the index price calculations of the liquidity.

Proof of Concept

Oracle data feed is insufficiently validated. There is no check for stale price and round completeness.

   (
        uint80 roundID1,
        int256 price1,    
        uint256 startedAt1,
        uint256 timeStamp1,
        uint80 answeredInRound1
    ) = priceFeed1.latestRoundData();

    int256 price2 = getOracle2_Price();

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

Tools Used

Manual Review

Recommended Mitigation Steps

   (
        uint80 roundID1,
        int256 price1,
        ,
        uint256 timeStamp1,
        uint80 answeredInRound1
    ) = priceFeed1.latestRoundData();

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

   int256 price2 = getOracle2_Price();

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

OR

  int256 price1 = getOracle1_Price();
  int256 price2 = getOracle2_Price();

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

[L-01] Absence Of Zero Address Checks

Zero address checks absent, before assigning function parameters to stateVariables

There is 10 instance of this issue:

File : rewards/RewardsDistributionRecipient.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/RewardsDistributionRecipient.sol#L1

File : rewards/RewardFactory => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/RewardsFactory.sol#L68-L70

File : rewards/StakingRewards => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/StakingRewards.sol#L60-L68

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

File : VoultFactory.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L180

File : VoultFactory.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L184

File : VoultFactory.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L312

Recommended Mitigation

Write code for Zero address check before assigning them to state variable .

[L-02] Missing Of License (SPDX-License-Identifier Missing)

License Identifier missing in some contract files

There is 2 instance of this issue:

File : rewards/Owned.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/Owned.sol#L1 File : rewards/RewardsDistributionRecipient.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/RewardsDistributionRecipient.sol#L1

Recommended Mitigation

Should mention License Identifier

[L-03] Modifiers Changing State Variables

Its not recommended that modifiers able to change state variables, So either all those functionality should be enclosed in a private function and further use those in other functions instead of modifiers .

There is 1 instance of this issue:

File : rewards/StakingRewards => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/StakingRewards.sol#L60-L68

[L-04] Return value of TransferFrom() not checked

There is 4 instance of this issue:

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L167

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L228

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L231

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L365

Recommended Mitigation

Return value of transferFrom() should checked before further proceeding in code base. Or consider using safeTransfer() / safeTransferFrom() instead of transfer() / transferFrom()

[L-05] Use Require() / Revert() instead of Assert()

When a function call fails then in case of assert() all remaining gas is consumed But in case of require() and revert() remaining gas were send back to Caller,

So its recommended to use require/revert instead of assert in perspective of gas optimization

There is 1 instance of this issue:

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L190

[N-01] Misleading Comment

There is 1 instance of this issue:

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L265

It saying // 0.5% = multiply by 1000 then divide by 5 where actually it will multiply by 5 divide by 1000 for 0.5%

[G-01] BOOLEAN COMPARISONS

There is 5 instance of this issue:

File : rewards/RewardFactory => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/RewardsFactory.sol#L96

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

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

File : Vault.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L88

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L217

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L314

Mitigation

For example :

if(Vault(_insrToken).idExists(_epochEnd) == false || Vault(_riskToken).idExists(_epochEnd) ** TO ** if(Vault(_insrToken).idExists(_epochEnd) || Vault(_riskToken).idExists(_epochEnd)

The results of Vault(_insrToken).idExists(_epochEnd) already in true or false, so need to compair those with booleans

[G-02] NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

Here Uints initiaziled with 0, which is not necessary cause its by default value is zero

There is 3 instance of this issue:

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

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L443

File : VoultFactory.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L159

Mitigation

No need to initialize with 0

[G-03] EMPTY BLOCKS SHOULD BE REMOVED OR EMIT SOMETHING

The function is with empty block

There is 3 instance of this issue:

File : rewards/RewardsDistributionRecipient.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/RewardsDistributionRecipient.sol#L18

File : SemiFungibleVault.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L277-L281

File : SemiFungibleVault.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L283-L287

Mitigation

It should do something. At least emits some events.

[G-4] ABI.ENCODE() IS LESS EFFICIENT THAN ABI.ENCODEPACKED()

There is 2 instance of this issue:

File : rewards/RewardFactory => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/RewardsFactory.sol#L118

File : rewards/RewardFactory => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/RewardsFactory.sol#L150

Mitigation

Use abi.encodePacked()

[G-05] UNNECESSARY USE OF SAFEMATH

There is 1 instance of this issue:

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

Mitigation

There is no need to use SafeMath above solidity 0.8.0 as it by default checks for over and underflow condition

[G-06] >= COSTS LESS GAS THAN >

There is 2 instance of this issue:

File : Vault.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L88

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

[G-07] FUNCTION FOR LENGTH SHOULD GET CACHED IN FOR LOOP

here the return value for epochsLength() should cached first and then used in loop

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L443

[G-08] FUNCTION THAT COULD BE EXTERNAL

To save gas consumption function should declare external, which does not call inside that smartcontract by other functions

There is 3 instance of this issue:

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L438

File : VoultFactory.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L295

File : VoultFactory.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L366

[G-09] SHOULD OPTIMIZED FOR LOOP

There is 1 instance of this issue:

File : Voult.sol => https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L443

. Should not initialize uint with default value i.e uint i=0 TO uint i; . Should cached the length function to memory stack then used that memory variable for loop condition check . Should use ++i instead i++ . Should uncheck i++