iglyx - Deposit fee can always be avoided

[sherlock-admin]

iglyx

high

Deposit fee can always be avoided

Summary

Any depositor can avoid paying deposit fee by substituting deposit(id, ...) call with an atomically coupled deposit(0, ...) -> mintDepositInQueue(id, 1) call.

Core reason is that deposit fee is fully substituted with relayer fee sent back to a caller, while depositQueue is processed in LIFO order.

Vulnerability Detail

Bob the depositor wants to deposit just before epoch start to minimize the level of risk by maximizing the information as of the deposit time.

To avoid paying nearly full deposit fee instead of deposit(id, _assets, Bob) he runs deposit(0, _assets, Bob) -> mintDepositInQueue(id, 1) atomically.

As relayer fee is sent back to msg.sender, Bob is effectively avoided paying deposit fee, having only somewhat increased the overall gas costs (even not so substantially, as deposit(0, ...) performs only one queue SSTORE instead of fee calculation and token transfer of direct deposit), which is negligible whenever Bob's deposit is big enough.

Impact

As such a replacement can be done for any deposit, protocol can lose up to all deposit fees.

Code Snippet

_deposit() do not apply the fee if the call is for zero epoch and depositQueue is being used:

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L470-L501

    function _deposit(
        uint256 _id,
        uint256 _assets,
        address _receiver
    ) internal {
        // mint logic, either in queue or direct deposit
        if (_id != 0) {
            uint256 assetsToDeposit = _assets;

            if (depositFee > 0) {
                (uint256 maxX, , uint256 minX) = getEpochConfig(_id);
                // deposit fee is calcualted linearly between time of epoch creation and epoch starting (deposit window)
                // this is because late depositors have an informational advantage
                uint256 fee = _calculateFeePercent(int256(minX), int256(maxX));
                // min minRequiredDeposit modifier ensures that _assets has high enough value to not devide by 0
                // 0.5% = multiply by 10000 then divide by 50
                uint256 feeAmount = _assets.mulDivDown(fee, 10000);
                assetsToDeposit = _assets - feeAmount;
                _asset().safeTransfer(treasury, feeAmount);
            }

            _mintShares(_receiver, _id, assetsToDeposit);

            emit Deposit(msg.sender, _receiver, _id, _assets);
        } else {
@>          depositQueue.push(
                QueueItem({assets: _assets, receiver: _receiver, epochId: _id})
            );

            emit DepositInQueue(msg.sender, _receiver, _id, _assets);
        }
    }

Then mintDepositInQueue() proceeds with the queue elements backwards, in LIFO order:

    function mintDepositInQueue(uint256 _epochId, uint256 _operations)
        external
        epochIdExists(_epochId)
        epochHasNotStarted(_epochId)
        nonReentrant
    {
        // make sure there is already a new epoch set
        // epoch has not started
        QueueItem[] memory queue = depositQueue;
        uint256 length = depositQueue.length;

        // dont allow minting if epochId is 0
        if (_epochId == 0) revert InvalidEpochId();

        if (length == 0) revert OverflowQueue();
        // relayers can always input a very big number to mint all deposit queues, without the need to read depostQueue length first
        if (_operations > length) _operations = length;

        // queue is executed from the tail to the head
        // get last index of queue
@>      uint256 i = length - 1;
        while ((length - _operations) <= i) {
            // this loop impelements FILO (first in last out) stack to reduce gas cost and improve code readability
            // changing it to FIFO (first in first out) would require more code changes and would be more expensive
            _mintShares(
                queue[i].receiver,
                _epochId,
                queue[i].assets - relayerFee
            );
            emit Deposit(
                msg.sender,
                queue[i].receiver,
                _epochId,
                queue[i].assets - relayerFee
            );
            depositQueue.pop();
            if (i == 0) break;
            unchecked {
                i--;
            }
        }

        emit RelayerMinted(_epochId, _operations);

        asset.safeTransfer(msg.sender, _operations * relayerFee);
    }

Tool used

Manual Review

Recommendation

As an example, consider applying deposit fee on placing deposit queue request as well, i.e. do this part for all epoch ids:

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L495

            if (depositFee > 0) {
                (uint256 maxX, , uint256 minX) = getEpochConfig(_id);
                // deposit fee is calcualted linearly between time of epoch creation and epoch starting (deposit window)
                // this is because late depositors have an informational advantage
                uint256 fee = _calculateFeePercent(int256(minX), int256(maxX));
                // min minRequiredDeposit modifier ensures that _assets has high enough value to not devide by 0
                // 0.5% = multiply by 10000 then divide by 50
                uint256 feeAmount = _assets.mulDivDown(fee, 10000);
                assetsToDeposit = _assets - feeAmount;
                _asset().safeTransfer(treasury, feeAmount);
            }

It looks it needs to be the full fee as otherwise the demonstrated approach can be able to lower it.

As there is no way to remove deposit from the queue and user will be placing deposits there in advance, the fee for them will not be high anyway.

Duplicate of #75