Y2k Finance contest - async's results

Lines of code

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

Vulnerability details

Impact

If a 0 or a stale value is returned, possibly during a time of high chain congestion, the protocol will function as if the tokens are in a depegged or incorrect state when that may not be accurate. This would allow an attacker to collect undue rewards as well as break the core functionality of the protocol.

Additionally there is no requirement that nowPrice != 0 before the return. So if the priceFeed1 oracle were to return price1 == 0, this would cause the nowPrice to be returned as 0 which would wildly inflate payouts.

Proof of Concept

In PegOracle.sol, the function latestRoundData() is used to calculate the ratio between two Oracle feeds to determine if a Token is pegged or depegged.

The function correctly retrieves price2 with a call to getOracle2_Price(), but price1 is set by calling priceFeed1.latestRoundData() which does not validate the return data for freshness and being > 0.

function latestRoundData()
    public
    view
    returns (
        uint80 roundID,
        int256 nowPrice,
        uint256 startedAt,
        uint256 timeStamp,
        uint80 answeredInRound
    )
{
    (
        uint80 roundID1,
        int256 price1,
        uint256 startedAt1,
        uint256 timeStamp1,
        uint80 answeredInRound1
    ) = priceFeed1.latestRoundData(); // @audit there's no validation on this call like there is in getOracle1_Price().

    int256 price2 = getOracle2_Price();

    if (price1 > price2) {
        nowPrice = (price2 * 10000) / price1;
    } else {
        nowPrice = (price1 * 10000) / price2; // @audit if priceFeed1.latestRoundData() causes the nowPrice to be 0 it would be no bueno.
    }

    int256 decimals10 = int256(10**(18 - priceFeed1.decimals()));
    nowPrice = nowPrice * decimals10;

    return (
        roundID1,
        nowPrice / 1000000,
        startedAt1,
        timeStamp1,
        answeredInRound1
    );
}

Here's a Foundry test that asserts that it is possible for nowPrice == 0. This test should always fail, but it currently passes.

    function testLatestRoundDataAuditorEdition() public {
        vm.startPrank(admin);
        // staleOracle is setup to return 0 b/c pegOracle.latestRoundData() does not validate the return data
        FakeOracle staleOracle = new FakeOracle(oracleETH, 0);
        PegOracle pegOracle = new PegOracle(address(staleOracle), oracleETH);
        // returns the price from latestRoundData()
        (   ,
            int256 nowPrice,
            ,
            ,
        ) = pegOracle.latestRoundData();

        // This test currently passes! It should always fail.
        assertEq(0,nowPrice);
        vm.stopPrank();
    }

Tools Used

vim, Foundry

Recommended Mitigation Steps

Replace the priceFee1.latestRoundData() call with a call to getOracle1_Price() which includes proper validation checks on the return data.

function latestRoundData()
    public
    view
    returns (
        uint80 roundID,
        int256 nowPrice,
        uint256 startedAt,
        uint256 timeStamp,
        uint80 answeredInRound
    )
{
-    (
-        uint80 roundID1,
-        int256 price1,
-        uint256 startedAt1,
-        uint256 timeStamp1,
-        uint80 answeredInRound1
-    ) = priceFeed1.latestRoundData(); 

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

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

    int256 decimals10 = int256(10**(18 - priceFeed1.decimals()));
    nowPrice = nowPrice * decimals10;

    return (
        roundID1,
        nowPrice / 1000000,
        startedAt1,
        timeStamp1,
        answeredInRound1
    );
}

Judge has assessed an item in Issue #203 as Medium risk. The relevant finding follows:

General notes. The variables insr and hedge are used interchangably in the codebase. Consider sticking with hedge to reduce confusion.

VaultFactory controller address not validated

Impact

The controller addresses can be different in the VaultFactory and insr/risk vaults.

Judge note: this could be intended functionality, but wanted to include the finding if that was not the case.

Proof of Concept

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L295-L300 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L345-L359

In VaultFactory there are two functions to change the controller address - setController(), which sets VaultFactory.controller, and changeController(), which sets the vaults insr.controller and risk.controller address.

Since setController() does not validate if the controller address is non-zero (and disallow setting the address if so) and changeController() accepts any arbitrary non-zero address this can lead to a scenario where the VaultFactory.controller address is different from the insr.controller and risk.controller address.

Additionally, changeController() does not reset the VaultFactory's state variable controller address. So if a new market is created for a token after changeController() has been called then the expected controller address may not be what the market creator expects.

Tools Used

Manual anaylsis

Recommended Mitigation Steps

Refactor changeController() to use the VaultFactory.controller address instead of accepting an arbitrary address. Disallow setting a new controller in setController() when controller != 0, or remove this function entirely and set the controller address in the constructor.

Use of assert() instead of require()

Impact

In Vault.depositETH an assert() is used instead of a require() which causes a Panic error on failure and prevents the use of error strings.

0xRajeev explains it very well in their realitycards finding https://github.com/code-423n4/2021-06-realitycards-findings/issues/83

"Prior to solc 0.8.0, assert() used the invalid opcode which used up all the remaining gas while require() used the revert opcode which refunded the gas and therefore the importance of using require() instead of assert() was greater. However, after 0.8.0, assert() uses revert opcode just like require() but creates a Panic(uint256) error instead of Error(string) created by require(). Nevertheless, Solidity’s documentation says:

"Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.”

whereas

“The require function either creates an error without any data or an error of type Error(string). It should be used to ensure valid conditions that cannot be detected until execution time. This includes conditions on inputs or return values from calls to external contracts.”

Also, you can optionally provide a message string for require, but not for assert."

Proof of Concept

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L182-L193

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use require() with informative error strings instead of assert().

Gas Savings

Short circut expensive operations

In Controller.isDisaster() the if tree can be reordered to fail early before expensive external calls are made.

    modifier isDisaster(uint256 marketIndex, uint256 epochEnd) {
        address[] memory vaultsAddress = vaultFactory.getVaults(marketIndex);
        if(
            block.timestamp > epochEnd // no external call
            )
            revert EpochExpired();
        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()) // save this return value for use in the revert
            )
            revert PriceNotAtStrikePrice(getLatestPrice(vault.tokenInsured())); 

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

Multiple external calls can be avoided with saved variable

In Controller.isDisaster() there are two external calls to vault.tokenInsured(). Save gas by storing the return value of getLatestPrice(vault.tokenInsured()) which can be used in the revert.

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

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

Use external instead of public where possible

Functions with the public visibility modifier are costlier than external. Default to using the external modifier until you need to expose it to other functions within your contract.

Consider changing the visibility of these functions in to external

In Vault

• endEpoch
• setClaimTVL
• sendTokens
• getNextEpoch

In PegOracle

• getOracle1_Price

Cache storage values in memory for loop

In Vault.getNextEpoch() the return of epochsLength() is not cached.

Anytime you are reading from storage more than once, it is cheaper to cache variables in memory. An SLOAD cost 100 GAS while MLOAD and MSTORE only cost 3 GAS.

This is especially true in for loops when using the length of a storage array as the condition being checked after each loop. Caching the array length in memory can yield significant gas savings when the array length is high.

// Before
for (uint256 i = 0; i < epochsLength(); i++) { //@audit cache this length
    if (epochs[i] == _epoch) {
        if (i == epochsLength() - 1) {
            return 0;
        }
        return epochs[i + 1];
}

// After
uint memory length = epochsLength() - 1;
for (uint256 i = 0; i < length; i++) { // length cached
    if (epochs[i] == _epoch) {
        if (i == length) {
            return 0;
        }
        return epochs[i + 1];
}

Struct should be efficently packed

Reading and writing from each storage slot cost gas. Packing variables lets us reduce the number of slots our contract needs.

To allow the EVM to optimize your storage layout, make sure to order your storage variables and struct members such that they can be packed tightly. For example, in VaultFactory.sol if we reduce the size of some uint types the Struct MarketVault can be packed like this:

// Before - 6 slots
struct MarketVault{ 
	uint256 index;
    uint256 epochBegin;
    uint256 epochEnd;
    Vault hedge;
    Vault risk;
    uint256 withdrawalFee;
}

// After - 3 slots; the first 4 values fit in a single 32 bytes slot
struct MarketVault{ 
	uint16 index; // 2 bytes
    uint32 epochBegin; // 4 bytes; max time good until 2106
    uint32 epochEnd; // 4 bytes
    Vault hedge; // address 20 bytes
    Vault risk;
    uint256 withdrawalFee;
}

Use custom errors

Consider replacing revert strings with custom errors. Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met.)

This can be implemented in contracts SemiFungibleVault, Vault and PegOracle.