Skip to content
This repository has been archived by the owner on Oct 1, 2023. It is now read-only.

ShadowForce - Unbounded loop causes dos and relayers cannot be compensated #322

Closed
sherlock-admin opened this issue Mar 27, 2023 · 0 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Mar 27, 2023

ShadowForce

medium

Unbounded loop causes dos and relayers cannot be compensated

Summary

Unbounded loop causes dos and relayers cannot be compensated

Vulnerability Detail

In the Carousel.sol contract there is unbounded loop in the mintRollovers function. we can see this unbounded loop in the snippet below.

        while ((index - prevIndex) < (_operations)) {
            // only roll over if last epoch is resolved
            if (epochResolved[queue[index].epochId]) {
                uint256 entitledShares = previewWithdraw(
                    queue[index].epochId,
                    queue[index].assets
                );
                // mint only if user won epoch he is rolling over
                if (entitledShares > queue[index].assets) {
                    // skip the rollover for the user if the assets cannot cover the relayer fee instead of revert.
                    if (queue[index].assets < relayerFee) {
                        index++;
                        continue;
                    }
                    // @note we know shares were locked up to this point
                    _burn(
                        queue[index].receiver,
                        queue[index].epochId,
                        queue[index].assets
                    );
                    // transfer emission tokens out of contract otherwise user could not access them as vault shares are burned
                    _burnEmissions(
                        queue[index].receiver,
                        queue[index].epochId,
                        queue[index].assets
                    );
                    // @note emission token is a known token which has no before transfer hooks which makes transfer safer
                    emissionsToken.safeTransfer(
                        queue[index].receiver,
                        previewEmissionsWithdraw(
                            queue[index].epochId,
                            queue[index].assets
                        )
                    );

                    emit Withdraw(
                        msg.sender,
                        queue[index].receiver,
                        queue[index].receiver,
                        _epochId,
                        queue[index].assets,
                        entitledShares
                    );
                    uint256 assetsToMint = queue[index].assets - relayerFee;
                    _mintShares(queue[index].receiver, _epochId, assetsToMint);
                    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++;
        }

because the amount can be infinitely large, the loop may run out of gas and this in turn will cause a dos for the protocol. The relayers will be failed to be compensated because of this denial of service.

only when the roll over request win in the epoch round, the execution is incremented and the relayer is compensated with the relayer fee, but it is possible there is a large number of queued request that does not win the epoch, the relayer still pay the gas fee to process fee and get no compensation. this is seen in the snippet below.

 queue[index].epochId,
                    queue[index].assets
                );
                // mint only if user won epoch he is rolling over
                if (entitledShares > queue[index].assets) {
                    // skip the rollover for the user if the assets cannot cover the relayer fee instead of revert.
                    if (queue[index].assets < relayerFee) {
                        index++;
                        continue;
                    }

Impact

Unbounded loop can cause a denial of service in the protocol. Relayers cannot be paid and functions do not work properly. Direct loss of funds for the relayers.

Code Snippet

https://github.com/Y2K-Finance/Earthquake/blob/736b2e1e51bef6daa6a5ecd1decb7d156316d795/src/v2/Carousel/Carousel.sol#L361-L459

Tool used

Manual Review

Recommendation

still send at least a part of the relayer fee to relayer and also limit the unbounded loop to avoid running out of gas error.

Duplicate of #172

@github-actions github-actions bot closed this as completed Apr 3, 2023
@github-actions github-actions bot added Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Apr 3, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

1 participant