bin2chen - enlistInRollover/delistInRollover lack nonReentrant

[sherlock-admin]

bin2chen

medium

enlistInRollover/delistInRollover lack nonReentrant

Summary

enlistInRollover()/delistInRollover() Lack of reentrant protection provides the possibility to maliciously reentrancy modify other people's rolloverQueue, resulting in the queue may be blocked

Vulnerability Detail

mintRollovers() mints for rollovers
The code is as follows:

    function mintRollovers(uint256 _epochId, uint256 _operations)
        external
        epochIdExists(_epochId)
        epochHasNotStarted(_epochId)
        nonReentrant
    {
....

        while ((index - prevIndex) < (_operations)) {
...
                    _mintShares(queue[index].receiver, _epochId, assetsToMint); //@audit<------if receiver is contract ,will call receiver.onERC1155BatchReceived
                    emit Deposit(
                        msg.sender,
                        queue[index].receiver,
                        _epochId,
                        assetsToMint
                    );
                    rolloverQueue[index].assets = assetsToMint;
                    rolloverQueue[index].epochId = _epochId;
                    // only pay relayer for successful mints
                    executions++;
                }
            }
            index++;
        }

From the above code, we can see that _mintShares (receiver) will be called, and then modify rolloverQueue [index].assets
If the receiver is a contract, receiver.onERC1155BatchReceived() will be called, thus providing the possibility of malicious reentrancy

And enlistInRollover() delistInRollover() does not have nonReentrant modifier

Malicious users can reentrancy and adjust the rolloverQueue, resulting in the modified rolloverQueue [index].assets may not belong to the original user
Example:
Suppose current rolloverQueue:
rolloverQueue[.] = ...
rolloverQueue[9] = jack asset =100
rolloverQueue[10] = Alice asset =100 <------malicious contract and rolloverAccounting[_epochId] =10
rolloverQueue[11] = bob asset =10

1. Alice call mintRollovers(_operations=1)
2. _mintShares() -> call Alice.onERC1155BatchReceived()
3. in onERC1155BatchReceived() Alice call delistInRollover(), rolloverQueue become:
   rolloverQueue[.] = ...
   rolloverQueue[9] = jack asset =100
   rolloverQueue[10] = bob asset =100 <------alice remove , set to bob
4. after onERC1155BatchReceived() , back to mintRollovers() , set rolloverQueue[10].assets = 100
   ( index=10, No longer belongs to Alice)

This way modifies bob's rolloverQueue [10].assets, Subsequent other calls mintRollovers will fail.
because if bob's shares are not enough assets, when burn() will fail,

Impact

enlistInRollover/delistInRollover Lack of reentrant protection provides the possibility to maliciously modify other people's rolloverQueue, resulting in the queue may be blocked

Code Snippet

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

Tool used

Manual Review

Recommendation

enlistInRollover/delistInRollover add nonReentrant

Duplicate of #468