sherlock-audit / 2023-03-Y2K-judging

7 stars 1 forks source link

minhtrng - Dead queue items not removed from rolloverQueue can disincentivize relayers #475

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

minhtrng

medium

Dead queue items not removed from rolloverQueue can disincentivize relayers

Summary

When minting rollovers the whole queue is loaded into memory. This can get expensive because "dead" queue items are not removed and hence disincentivize relayers.

Vulnerability Detail

The function mintRollovers loads the whole queue into memory before processing it:

QueueItem[] memory queue = rolloverQueue;

Furthermore, items that dont contain enough assets remain in queue and will just be skipped.

// skip the rollover for the user if the assets cannot cover the relayer fee instead of revert.
if (queue[index].assets < relayerFee) {
    index++;
    continue;
}

Over time this can lead to a scenario where there are many "dead" items in the queue, due to their owners not even bothering to delist or claim because the assets would not compensate for the transactions cost.

Whenever a new epoch needs to be rolled over, all those items would need to be loaded into memory and iterated over. This can lead to the transaction cost for the relayer being higher than what would be gained by fees and hence disincentivizing them from performing the rollover.

This scenario could also be caused intentionally by malicious actors.

The following foundry test demonstrates the gas cost only for loading an array of 1000 queue item structs into memory (calculation works the same on Arbitrum, only price per gas unit is lower):

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

contract GasTest is Test {

    struct QueueItem {
        uint256 assets;
        address receiver;
        uint256 epochId;
    }

    QueueItem[] public rolloverQueue;

    function setUp() public {
        uint256 queueSize = 1000;
        for (uint i=0; i < queueSize; i++) {
            rolloverQueue.push(
                QueueItem({
                    assets: 1,
                    receiver: address(1),
                    epochId: 1
                }));
        }
    }

    function testY2KGasConsumptionOfArrayLoad() public {
        uint256 gasBefore = gasleft();
        QueueItem[] memory queue = rolloverQueue;
        uint256 gasAfter = gasleft();
        console2.log(gasBefore-gasAfter);
    }
}

The cost amounts to 6594564 gas, not yet accounting for all the additional operations while iterating over, such as calculating previewWithdraw.

Impact

Disincentivizing relayers and hence harming users.

Code Snippet

https://github.com/sherlock-audit/2023-03-Y2K/blob/ae7f210d8fbf21b9abf09ef30edfa548f7ae1aef/Earthquake/src/v2/Carousel/Carousel.sol#L402-L406

Tool used

Manual Review

Recommendation

Remove "dead" items from queue instead of skipping when encountering them

Duplicate of #172