Y2k Finance contest - eierina's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L198 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L246 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L261 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L277-L286 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L203

Vulnerability details

Impact

At the end of an epoch, the triggerEndEpoch(...) is called to trigger 'epoch end without depeg event', making risk users the winners and entitling them to withdraw (risk + hedge) from the vault. In the case of the Arbitrum sequencer going down or restarting, there is a grace period of one hour before the getLatestPrice() returns to execute without reverting. This means that the triggerEndEpoch(...) cannot complete during this time, because it calls the getLatestPrice().

Making this high-priority because unless the triggerEndEpoch(...) completes:

• winners cannot withdraw althought the epoch is over;
• during this time the strike price might be reached causing a depeg event at all effects turning the table for the winners;
• the getLatestPrice() is not functional to the completion of the triggerEndEpoch(...), nor to the withdraw, but only informative used to initialize the event object emitted at the very end of the triggerEndEpoch function.

First two points each constitute independent jsutification, thrid point reinforces the first 2 points.

Proof of Concept

triggerEndEpoch reverts if arbiter down or restarted less than eq GRACE_PERIOD_TIME ago (1hr)

File: Controller.sol:L246

Revert if getLatestPrice reverts.

function triggerEndEpoch(uint256 marketIndex, uint256 epochEnd) public {
    
    < ... omitted ... >

    emit DepegInsurance(
        keccak256(
            abi.encodePacked(
                marketIndex,
                insrVault.idEpochBegin(epochEnd),
                epochEnd
            )
        ),
        tvl,
        false,
        epochEnd,
        block.timestamp,
        getLatestPrice(insrVault.tokenInsured()) // @audit getLatestPrice reverts while sequencer unavailable or during grace period
    );
}

File: Controller.sol:L277-L286

Revert if sequencer down or grace period after restart not over.

function getLatestPrice(address _token)
    public
    view
    returns (int256 nowPrice)
{
    < ... omitted ... >

    bool isSequencerUp = answer == 0;
    if (!isSequencerUp) {
        revert SequencerDown();
    }

    // Make sure the grace period has passed after the sequencer is back up.
    uint256 timeSinceUp = block.timestamp - startedAt;
    if (timeSinceUp <= GRACE_PERIOD_TIME) { // @audit 1 hour
        revert GracePeriodNotOver();
    }

    < ... omitted ... >
}

withdraw fails if triggerEndEpoch did not execute successfully

File: Vault.sol:L203

Can execute if block.timestamp > epochEnd, but fails if trigger did not execute. Winners cannot withdraw.

function withdraw(
    uint256 id,
    uint256 assets,
    address receiver,
    address owner
)
    external
    override
    epochHasEnded(id) // @audit same as require((block.timestamp > id) || idDepegged[id]), hence independent from triggers.
    marketExists(id)
    returns (uint256 shares)
{
    < ... omitted ... >

    uint256 entitledShares = beforeWithdraw(id, shares); // @audit ratio is idClaimTVL[id]/ifFinalTVL[id], hence zero unless triggers executed
    
    < ... omitted ... >

    emit Withdraw(msg.sender, receiver, owner, id, assets, entitledShares);
    asset.transfer(receiver, entitledShares);

    return entitledShares;
}

Tools Used

n/a

Recommended Mitigation Steps

The latest price is retrieved at the very end of the triggerEndEpoch(...) for the only purpose of initializing the DepegInsurance event. Since it is used for informational purpose (logging / offchain logging) and not for functional purpose to the triggerEndEpoch(...) execution, it can be relaxed.

Depending on how the event is used, when getLatestPrice() is called for informative/logging purpose only, there could be few alternatives:

• log a 0 when SequencerDown or GRACE_PERIOD_TIME not passed
• log a 0 when SequencerDown and ignore GRACE_PERIOD_TIME Once events are logged off-chain, some post processing may be used to correct/update the values with accurate data.

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

Low Priority / Non Critical issues

[NC-01] Market existance should be verified by factory

Controller calls VaultFactory's getVaults() which returns the Vaults array, and then check wether the array length is 2. This is repeated/duplicated in both triggerDepeg and triggerEndEpoch wich violates DRY. It also violates responsibility principle, as it is VaultFactory concern to return a correct response if the preconditions exist.

Suggested changes follow.

File-Line: VaultFactory.sol:L385-L391

     function getVaults(uint256 index)
         public
         view
-        returns (address[] memory vaults)
+        returns (address[2] memory vaults)
     {
-        return indexVaults[index];
+        vaults = indexVaults[index];
+        if(vaults[0] == address(0) || vaults[1] == address(0))
+            revert MarketDoesNotExist(index);
+
     }
 }

NOTE: the code above also includes a gas optimization described here.

File-Line: Controller.sol:L83-L88

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

File-Line: Controller.sol:L198-L201

     function triggerEndEpoch(uint256 marketIndex, uint256 epochEnd) public {
-        if(
-            vaultFactory.getVaults(marketIndex).length != VAULTS_LENGTH)
-                revert MarketDoesNotExist(marketIndex);

[NC-02] Controller address should be verified before using it

In VaultFactory::createNewMarket the controller address is tested for zero address after using it to test vault factory address. Move test for zero address on top.

File-Line: VaultFactory.sol:L188-L192

Actual

if(
    IController(controller).getVaultFactory() != address(this)
    )
    revert AddressFactoryNotInController();

if(controller == address(0))
    revert ControllerNotSet();

Expected

if(controller == address(0))
    revert ControllerNotSet();

if(
    IController(controller).getVaultFactory() != address(this)
    )
    revert AddressFactoryNotInController();

[NC-03] timewindow has no bounds

File-Line: Vault.sol:L287-L289

By default timewindow is set to 1 day, but it can be set to any unsigned value. The changeTimewindow should validate the new timewindow for valid bounds.

    function changeTimewindow(uint256 _timewindow) public onlyFactory {
+       if(_timeWindow < 1 hours || _timewindow > 2 days) // Define valid bounds
+           revert InvalidTimewindow(_timewindow);
        timewindow = _timewindow;
    }

[NC-04] Vault's withdraw can be called after epoch expires, before end is triggered

File-Line: Vault.sol:L211

modifier epochHasEnded should eventually be called epochHasExpired, because it does not catch wether Controller's triggerEndEpoch was called or not. Since it is not possible to withdraw without first executing either triggerEndEpoch or triggerDepegged, an extra validation needs to be added to verify trigger execution, which in this case can be done by checking idFinalTVL[id] state. This prevents the withdraw from reverting for dividing by zero and instead return a meaningful error.

    modifier epochHasEnded(uint256 id) {
-       if((block.timestamp < id) && idDepegged[id] == false)
+       if(((block.timestamp < id) && idDepegged[id] == false) || idFinalTVL[id] == 0)
            revert EpochNotFinished();
        _;
    }

[NC-05] Errors should help to quickly identify the root cause or security issues

File: Controller.sol

Line 75

-revert AddressNotAdmin();
+revert AddressNotAdmin(msg.sender);

Line 94, Line 212

-revert EpochNotExist();
+revert EpochNotExist(epochEnd);

Line 103

-revert EpochNotStarted();
+revert EpochNotStarted(block.timestamp, epochEnd);

Line 108

-revert EpochExpired();
+revert EpochExpired(block.timestamp, epochEnd);

Line 158, Line 160, Line 216, Line 218

-revert NotZeroTVL();
+revert NotZeroTVL(epochEnd);

Line 204

-revert EpochNotExpired();
+revert EpochNotExpired(block.timestamp, epochEnd);

Line 285

-revert GracePeriodNotOver();
+revert GracePeriodNotOver(block.timestamp, timeSinceUp, GRACE_PERIOD_TIME);

Line 306

-revert RoundIDOutdated();
+revert RoundIDOutdated(block.timestamp, answeredInRound, roundID);

Line 309

-revert TimestampZero();
+revert TimestampZero(block.timestamp);

File: VaultFactory.sol

Line 190

-revert AddressFactoryNotInController();
+revert AddressFactoryNotInController(controller, vaultFactory /*cached getVaultFactory()*/, address(this));

File: Vault.sol

Line 81

-revert MarketEpochDoesNotExist();
+revert MarketEpochDoesNotExist(id);

Line 89

-revert EpochAlreadyStarted();
+revert EpochAlreadyStarted(block.timestamp, idEpochBegin[id], timewindow);

Line 97

-revert EpochNotFinished();
+revert EpochNotFinished(block.timestamp, id);

Line 315

-revert MarketEpochExists();
+revert MarketEpochExists(epochEnd);

Line 318

-revert EpochEndMustBeAfterBegin();
+revert EpochEndMustBeAfterBegin(epochBegin, epochEnd);

[NC-06] nonReentrant modifier should be on top of other modifiers

File-Line: Vault.sol:L161

As a best-practice always set the nonReentrant modifier on top to avoid reentracy on other modifiers.

    function deposit(
        uint256 id,
        uint256 assets,
        address receiver
    )
        public
        override
+       nonReentrant        
        marketExists(id)
        epochHasNotStarted(id)
-       nonReentrant
        returns (uint256 shares)
    {

Gas Optimizations

Prefer fixed size arrays in place of dynamic sized arrays where fits

dynamic arrays are more expensive than fixed arrays in terms of gas, prefer fixed array size when it fits the case.

VaultFactory.sol can use fixed size array for market vaults

Vaults always are in a pair, and are mapped to a dynamic array. Changing to a fixed array can save a lot of gas (e.g. ~22100 gas saved for an insert).

Before: [PASS] testMarketDoesNotExist() (gas: 6066351)</br> After: [PASS] testMarketDoesNotExist() (gas: 6044191)

File-Line: VaultFactory.sol:L119

-    mapping(uint256 => address[]) public indexVaults; //[0] hedge and [1] risk vault
+    mapping(uint256 => address[2]) public indexVaults; //[0] hedge and [1] risk vault

File-Line: VaultFactory.sol:L313, VaultFactory.sol:L331, VaultFactory.sol:L352

-        address[] memory vaults = indexVaults[_marketIndex];
+        address[2] memory vaults = indexVaults[_marketIndex];

File-Line: VaultFactory.sol:L388

-        returns (address[] memory vaults)
+        returns (address[2] memory vaults)

RewardsFactory.sol can use fixed size array for staking rewards

File-Line: RewardsFactory.sol:L27

-    mapping(bytes32 => address[]) public hashedIndex_StakingRewards; //Hashed Index, Staking Rewards [0] = insrance, [1] = risk
+    mapping(bytes32 => address[2]) public hashedIndex_StakingRewards; //Hashed Index, Staking Rewards [0] = insrance, [1] = risk

Avoid unnecessary calls to external contracts

Making external calls to other contracts costs gas due to the call itself and the code executed by the external call. Unnecessarily repeating external calls duplicate costs. Prefer caching external call response where possible.

modifier isDisaster is large, forces duplicate external calls and is used once only

Part of the code used in isDisaster modifier is repeated in the only function that uses the modifier triggerDepeg, and the duplicated code includes an external call. Suggestion is to:

• remove the modifier, and do the validation inline same as it is done;
• moreover cache getLatestPrice calls as they are repeated within the function.

This change not only reduces the total lines of code but also reduces gas costs.

-    function triggerDepeg(uint256 marketIndex, uint256 epochEnd)
-        public
-        isDisaster(marketIndex, epochEnd)
+    function triggerDepeg(uint256 marketIndex, uint256 epochEnd) public
     {
-        address[] memory vaultsAddress = vaultFactory.getVaults(marketIndex);
+        address[2] memory vaultsAddress = vaultFactory.getVaults(marketIndex);
         Vault insrVault = Vault(vaultsAddress[0]);
         Vault riskVault = Vault(vaultsAddress[1]);

+        if(insrVault.idExists(epochEnd) == false)
+            revert EpochNotExist();
+
+        int256 latestPrice = getLatestPrice(insrVault.tokenInsured()); // @audit cache getLatestPrice call
+        if(insrVault.strikePrice() < latestPrice) // @audit use cached price
+            revert PriceNotAtStrikePrice(latestPrice); // @audit use cached price
+
+        if(insrVault.idEpochBegin(epochEnd) > block.timestamp)
+            revert EpochNotStarted();
+
+        if(block.timestamp > epochEnd)
+            revert EpochExpired();
+
         //require this function cannot be called twice in the same epoch for the same vault
         if(insrVault.idFinalTVL(epochEnd) != 0)
             revert NotZeroTVL();
         if(riskVault.idFinalTVL(epochEnd) != 0)
             revert NotZeroTVL();

         insrVault.endEpoch(epochEnd, true);
         riskVault.endEpoch(epochEnd, true);

         insrVault.setClaimTVL(epochEnd, riskVault.idFinalTVL(epochEnd));
         riskVault.setClaimTVL(epochEnd, insrVault.idFinalTVL(epochEnd));

         insrVault.sendTokens(epochEnd, address(riskVault));
         riskVault.sendTokens(epochEnd, address(insrVault));

         VaultTVL memory tvl = VaultTVL(
             riskVault.idClaimTVL(epochEnd),
             insrVault.idClaimTVL(epochEnd),
             riskVault.idFinalTVL(epochEnd),
             insrVault.idFinalTVL(epochEnd)
         );

         emit DepegInsurance(
             keccak256(
                 abi.encodePacked(
                     marketIndex,
                     insrVault.idEpochBegin(epochEnd),
                     epochEnd
                 )
             ),
             tvl,
             true,
             epochEnd,
             block.timestamp,
-            getLatestPrice(insrVault.tokenInsured()) // @audit remove duplicate call
+            latestPrice // @audit use cached price
         );
     }             

xyz.sol 123

modifier isDisaster forces duplicate calls to VaultFactory::getVaults()

File-Line: Controller.sol:L148-L152

Modifier isDisaster is used only by triggerDepeg and never reused. Move isDisaster verification code inline to triggerDepeg and remove duplicate calls to VaultFactory::getVaults().

getLatestPrice called multiple times within a function

File-Line: .sol:L-L

Use unchecked where no risk for overflow/underflow

File-Line: VaultFactory.sol:L195

+    unchecked {
         marketIndex += 1;
+    }

File-Line: Vault.sol:L88-L89

    modifier epochHasNotStarted(uint256 id) {
+    unchecked {        
        if(block.timestamp > idEpochBegin[id] - timewindow)
            revert EpochAlreadyStarted();
+    }            
        _;
    }

NOTE: requires timewindow to have bounds checked as described here.

File-Line: Vault.sol:L227

+   unchecked {
        entitledShares = entitledShares - feeValue; // @audit cannot underflow, feeValue is a fraction of entitledShares
+   }

File-Line: Vault.sol:L266

+   unchecked {
        return (amount * epochFee[_epoch]) / 1000; // @audit cannot realistically overflow, epochFee is limited to a max of 150
+   }

File-Line: Vault.sol:L438

Cheaper getNextEpoch below, multiple optimizations.

function getNextEpoch(uint256 _epoch)
    public
    view
    returns (uint256 nextEpochEnd)
{
+   unchecked {
+       uint256 scanLength = epochs.length > 0 ? epochs.length - 1 : 0;
+       for(uint256 i = 0; i < scanLength; ++i) {
+           if(epochs[i] == _epoch) {
+               nextEpochEnd = epochs[i+1];
+               break;
+           }
+       }
+   }
-   for (uint256 i = 0; i < epochsLength(); i++) {
-       if (epochs[i] == _epoch) {
-           if (i == epochsLength() - 1) {
-               return 0;
-           }
-           return epochs[i + 1];
-       }
-   }
}

Prefer pre-increment, post-increment operators

Prefer increment operator rather than adding or subtracting 1. Pre-increment if it fits the case, post-increment otherwise.

File-Line: VaultFactory.sol:L195

+    unchecked {
-        marketIndex += 1;
+        ++marketIndex;
+    }