0x52 - Adversary can break deposit queue and cause loss of funds

[sherlock-admin]

0x52

high

Adversary can break deposit queue and cause loss of funds

Summary

Vulnerability Detail

Carousel.sol#L531-L538

function _mintShares(
    address to,
    uint256 id,
    uint256 amount
) internal {
    _mint(to, id, amount, EMPTY);
    _mintEmissions(to, id, amount);
}

When processing deposits for the deposit queue, it _mintShares to the specified receiver which makes a _mint subcall.

ERC1155.sol#L263-L278

function _mint(address to, uint256 id, uint256 amount, bytes memory data) internal virtual {
    require(to != address(0), "ERC1155: mint to the zero address");

    address operator = _msgSender();
    uint256[] memory ids = _asSingletonArray(id);
    uint256[] memory amounts = _asSingletonArray(amount);

    _beforeTokenTransfer(operator, address(0), to, ids, amounts, data);

    _balances[id][to] += amount;
    emit TransferSingle(operator, address(0), to, id, amount);

    _afterTokenTransfer(operator, address(0), to, ids, amounts, data);

    _doSafeTransferAcceptanceCheck(operator, address(0), to, id, amount, data);
}

The base ERC1155 _mint is used which always behaves the same way that ERC721 safeMint does, that is, it always calls _doSafeTrasnferAcceptanceCheck which makes a call to the receiver. A malicious user can make the receiver always revert. This breaks the deposit queue completely. Since deposits can't be canceled this WILL result in loss of funds to all users whose deposits are blocked. To make matters worse it uses first in last out so the attacker can trap all deposits before them

Impact

Users who deposited before the adversary will lose their entire deposit

Code Snippet

Carousel.sol#L310-L355

Tool used

Manual Review

Recommendation

Override _mint to remove the safeMint behavior so that users can't DOS the deposit queue

[3xHarry]

agree with this issue, there is no easy solution to this, as by definition when depositing into queue, the user gives up the atomicity of his intended mint. Looking at Openzeppelins 1155 implementation guide it is recommended to ensure the receiver of the asset is able to call safeTransferFrom. By removing the acceptance check in the _mint function, funds could be stuck in a smart contract.

Another alternative would be to do the 1155 acceptance check in the mint function and confiscate the funds if the receiver is not able to hold 1155s. The funds could be retrieved via a manual process from the treasury afterward.

[3xHarry]

going with Recommendation is prob the easiest way

[3xHarry]

fix PR: Y2K-Finance/Earthquake#124

[IAm0x52]

Fix looks good. _mint no longer calls acceptance check so rollover can longer be DOS'd by it