End epoch cannot be triggered preventing winners to withdraw

[code423n4]

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.

[3xHarry]

grate catch!

[MiguelBits]

fixed this by changing triggerEndEpoch,

AggregatorV3Interface priceFeed = AggregatorV3Interface(
            vaultFactory.tokenToOracle(insrVault.tokenInsured())
        );
        (
            ,  
            int256 price,
            ,
            ,
            
        ) = priceFeed.latestRoundData();

        emit DepegInsurance(
            keccak256(
                abi.encodePacked(
                    marketIndex,
                    insrVault.idEpochBegin(epochEnd),
                    epochEnd
                )
            ),
            tvl,
            true,
            epochEnd,
            block.timestamp,
            price
        );

[HickupHH3]

Agree with the points raised by the warden, especially on how getLatestPrice() is merely for informational purposes in the event emission.