codehawks-bot commented 1 year ago

Design flaw in the _distribute() can unexpectedly drain the rescue requestor's stuck token

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L139

https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L146-L147

https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L154

https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L164

Summary

The Distributor::_distribute() does not account for the stuck amount mistakenly transferred to the Proxy contract. If the stuck token's address is identical to the prize token's address, the stuck amount will be distributed to contest winners.

Subsequently, a rescue requestor will lose their token permanently.

Vulnerability Details

The Sparkn protocol was designed as an escrow for distributing prizes (tokens) to contest winners. One of the protocol features is recovering stuck tokens (being sent by mistake) from a contest's Proxy contract after the contest expires.

In other words, all prizes must be distributed to all winners before an owner (protocol admin) can execute the stuck tokens' recovery process. However, if the stuck token's address is identical to the prize token's address, the stuck amount will not be accounted for by the _distribute().

The _distribute() will count the total balance locked in the Proxy as prizes for winners. Therefore, the stuck amount will also be distributed to the contest winners. The remaining amount will then become the protocol's commission fee and be transferred to the protocol's fee collector.

Consequently, a rescue requestor could not retrieve their token as expected.

    function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
        internal
    {
        ...

        IERC20 erc20 = IERC20(token);
@>      uint256 totalAmount = erc20.balanceOf(address(this));

        ...

        uint256 winnersLength = winners.length; // cache length
        for (uint256 i; i < winnersLength;) {
@>          uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
@>          erc20.safeTransfer(winners[i], amount);
            unchecked {
                ++i;
            }
        }

        ...

@>      _commissionTransfer(erc20);
        emit Distributed(token, winners, percentages, data);
    }

    ...

    function _commissionTransfer(IERC20 token) internal {
@>      token.safeTransfer(STADIUM_ADDRESS, token.balanceOf(address(this)));
    }

The total token balance is viewed as winners' prizes: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L139
All balance (minus the commission fee) is distributed to winners: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L146-L147
Transferring the commission fee: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L154
The commission fee is transferred to the protocol's fee collector: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L164

Impact

Suppose the stuck token's address is identical to the prize token's address. In that case, the stuck amount will be distributed to contest winners unexpectedly since the stuck amount will be counted as prizes for the winners.

Subsequently, a rescue requestor will lose their token permanently.

Tools Used

Manual Review

Recommendations

To fix the vulnerability, I recommend improving the following functions: distribute(), _distribute(), and _commissionTransfer(), as shown below.

The uint256 maxAmountToTransfer param is introduced to the distribute(), which must also be passed to the _distribute().
The _distribute() must count the maxAmountToTransfer param as the maximum prize amount to distribute to winners instead.
The _commissionTransfer() must transfer only a commission fee calculated from the maxAmountToTransfer param instead of transferring all remaining amount.

This way, an organizer or owner (protocol admin) can define the maxAmountToTransfer param when executing the distribute(). The stuck amount will not be mistakenly transferred from the Proxy and can be retrieved to a rescue requestor as expected.

    // distribute()
-   function distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
+   function distribute(address token, uint256 maxAmountToTransfer, address[] memory winners, uint256[] memory percentages, bytes memory data) //@audit -- the maxAmountToTransfer param is introduced
        external
    {
        if (msg.sender != FACTORY_ADDRESS) {
            revert Distributor__OnlyFactoryAddressIsAllowed();
        }
-       _distribute(token, winners, percentages, data);
+       _distribute(token, maxAmountToTransfer, winners, percentages, data); //@audit -- the maxAmountToTransfer param must be passed to the _distribute()
    }

    ...

    // _distribute()
-   function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
+   function _distribute(address token, uint256 maxAmountToTransfer, address[] memory winners, uint256[] memory percentages, bytes memory data)
        internal
    {
        ...

        IERC20 erc20 = IERC20(token);
        uint256 totalAmount = erc20.balanceOf(address(this));

        //@audit -- do some input checks on the maxAmountToTransfer param
+       if (maxAmountToTransfer == 0) revert Distributor__MaxAmountToTransferIsZero();
+       if (maxAmountToTransfer > totalAmount) revert Distributor__InsufficientAmountToDistribute();

        ...

        uint256 winnersLength = winners.length; // cache length
        for (uint256 i; i < winnersLength;) {
-           uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
+           uint256 amount = maxAmountToTransfer * percentages[i] / BASIS_POINTS; //@audit -- count the maxAmountToTransfer param as the maximum prize amount to distribute to winners
            erc20.safeTransfer(winners[i], amount);
            unchecked {
                ++i;
            }
        }

        ...

-       _commissionTransfer(erc20);
+       _commissionTransfer(erc20, maxAmountToTransfer); //@audit -- the maxAmountToTransfer param must be passed to the _commissionTransfer()
        emit Distributed(token, winners, percentages, data);
    }

    ...

    // _commissionTransfer()
-   function _commissionTransfer(IERC20 token) internal {
+   function _commissionTransfer(IERC20 token, uint256 maxAmountToTransfer) internal {

        //@audit -- the _commissionTransfer() must transfer only a commission fee calculated from the maxAmountToTransfer param instead of transferring all remaining amount
+       uint256 amount = maxAmountToTransfer * COMMISSION_FEE / BASIS_POINTS;

-       token.safeTransfer(STADIUM_ADDRESS, token.balanceOf(address(this)));
+       token.safeTransfer(STADIUM_ADDRESS, amount);
    }

PatrickAlphaC commented 1 year ago

rescuing tokens is not a valid finding.

aslanbekaibimov commented 1 year ago

The README does describe that rescuing funds is one of the features of Sparkn. The only limitation is that the rescued token should be one of the whitelisted ones.

This issue (and its duplicate) reveal that rescue does not always work well: if the rescued token is the same ERC20 that's used for distribution, it will be lost.

serial-coder commented 1 year ago

The README does describe that rescuing funds is one of the features of Sparkn. The only limitation is that the rescued token should be one of the whitelisted ones.

This issue (and its duplicate) reveal that rescue does not always work well: if the rescued token is the same ERC20 that's used for distribution, it will be lost.

I agreed with @aslanbekaibimov.

Rescuing stuck tokens is one of the primary features of Sparkn (see the protocol's README). This issue indicates that the rescuing feature was misdesigned.

If the stuck token's address is identical to the prize token's, the stuck amount will be distributed to contest winners unexpectedly since the stuck amount will be counted as prizes for the winners.

Subsequently, a rescue requestor will lose their token permanently. This also means that the rescuing feature could not function as promised (design flaw).

For this reason, I escalate this issue.

Note: Please consider the statement from the protocol's README:

There is a way to rescue the token stuck in the proxy contract after the deployment and distribution of prizes only when the token is whitelisted.".

This means that a rescue requestor must be able to recover their token if the token is whitelisted (no matter that the token is the same ERC-20 token as the prize token).

PatrickAlphaC commented 1 year ago

checking with sponsor

PatrickAlphaC commented 1 year ago

Checked with sponsor. The docs were a bit misleading. The protocol is not supposed to be able to work with multiple tokens. I will mark this as informational and the docs will be updated.

Cyfrin / 2023-08-sparkn