codehawks-bot commented 1 year ago

If a winner is blacklisted on any of the tokens they can't receive their funds

Severity

High Risk

Relevant GitHub Links

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

Summary

Normally this would be a big issue since transfers are done in a loop to all winners i.e all winners wouldn't be able to get their tokens, but winners are chosen off chain and from the Q&A section of SparkN onboarding video we can see that after picking a set of winners they can later on be changed, that's the set of winners. This means that, reasonably, after an attempt to send the tokens to winners has been made and it reverts due to one or a few of the users being in the blacklist/blocklist of USDC/USDT, the set of winners can just be re-chosen without the blacklisted users, now whereas that helps other users from having their funds locked in the contract, this unfairly means that the blacklisted users would lose their earned tokens, since their share must be re-shared to other winners to cause this not to revert

        if (totalPercentage != (10000 - COMMISSION_FEE)) {
            revert Distributor__MismatchedPercentages();
        }

Vulnerability Detail

See summary

Additionally note that, the contest readMe's section has indicated that that general stablecoins would be used... specifically hinting USDC, DAI, USDT & JPYC,

Now important is to also keep in mind that https://github.com/d-xo/weird-erc20#tokens-with-blocklists shows that:

Some tokens (e.g. USDC, USDT) have a contract level admin controlled address blocklist. If an address is blocked, then transfers to and from that address are forbidden.

Impact

Two impacts, depending on how SparkN decides to sort this out, either:

All winners funds ends up stuck in contract if sparkN doesn't want to change the percentages of each winner by setting that of blacklisted users to zero and sharing their percentages back in the pool
Some users would have their funds unfairly given to other users

Tool used

Manual Audit

Recommendation

Consider introducing a functionality that allows winners to specify what address they'd like to be paid, that way even a blocklisted account can specify a different address he/she owns, this case also doesn't really sort this as an attacker could just send any blacklisted address to re-grief the whole thing, so a pull over push method could be done to transfer rewards to winners

Additional Note

With this attack window in mind, if a pull method is going to be used then the _commisionTransfer() function needs to be refactored to only send the commision.

PatrickAlphaC commented 1 year ago

Moving to low. If an address is blacklisted you can just redeploy without the blacklisted user. Not a big deal if it reverts the first time.

If a user gets blacklisted, that's their bad for working with that token. They could also use a clean fresh address when interacting to limit their chance of this happening.

leeftk commented 1 year ago

As per the documentation "If a contest is created and funded, there is no way to refund. All the funds belong to the persons who wants to help solve the problem, we call them "supporters". And there is a certain assets-locking period of time in which no one except the organizer can call and distribute the funds to the winners."

Meaning funds sent to a contract will be stuck in the contract if they're not able to be sent to the winners.

This should be a high.

serial-coder commented 1 year ago

I disagree with @leeftk.

An organizer can re-execute the distribute() and change the blacklisted winner's address to a new address. This issue cannot freeze the funds.

So this is a valid low issue.

leeftk commented 1 year ago

That's not possible the distribute function there is a check that only the FACTORY_ADDRESS can call this function.

    function distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
        external
    {
        if (msg.sender != FACTORY_ADDRESS) {
            revert Distributor__OnlyFactoryAddressIsAllowed();
        }
        _distribute(token, winners, percentages, data);
    }

In proxy factory there is a distributeByOwner() function that can only be called by the owner as well.

    function distributeByOwner(
        address proxy,
        address organizer,
        bytes32 contestId,
        address implementation,
        bytes calldata data
    ) public onlyOwner {
        if (proxy == address(0)) revert ProxyFactory__ProxyAddressCannotBeZero();
        bytes32 salt = _calculateSalt(organizer, contestId, implementation);
        if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
        // distribute only when it exists and expired
        if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
        _distribute(proxy, data);
    }

Essentially this means that yes these funds could potentially be salvaged but it would fall on the owner to WANT to spend the gas costs to call this function on behalf of the organizer. I agree that it may not be a high but it should be a medium, depending on the owner to fix a problem is not "trustless".

serial-coder commented 1 year ago

That's not possible the distribute function there is a check that only the FACTORY_ADDRESS can call this function.
    function distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
        external
    {
        if (msg.sender != FACTORY_ADDRESS) {
            revert Distributor__OnlyFactoryAddressIsAllowed();
        }
        _distribute(token, winners, percentages, data);
    }
In proxy factory there is a distributeByOwner() function that can only be called by the owner as well.
    function distributeByOwner(
        address proxy,
        address organizer,
        bytes32 contestId,
        address implementation,
        bytes calldata data
    ) public onlyOwner {
        if (proxy == address(0)) revert ProxyFactory__ProxyAddressCannotBeZero();
        bytes32 salt = _calculateSalt(organizer, contestId, implementation);
        if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
        // distribute only when it exists and expired
        if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
        _distribute(proxy, data);
    }
Essentially this means that yes these funds could potentially be salvaged but it would fall on the owner to WANT to spend the gas costs to call this function on behalf of the organizer. I agree that it may not be a high but it should be a medium, depending on the owner to fix a problem is not "trustless".

Why do you need the owner to recover the funds?

Let's say one of the winners gets blacklisted. Upon executing the ProxyFactory::deployProxyAndDistribute() by an organizer, the transaction will be reverted in the _distribute() in L251.

This way, the organizer can change the blacklisted winner's address to a new address and execute the deployProxyAndDistribute() to deploy the proxy and distribute prizes to all winners again.

There are no funds permanently stuck. Hence, your argument is invalid.

joedakwa commented 1 year ago

This should not be a low.

magnetto90 commented 1 year ago

The real issue in this case is when the blacklisted address is STADIUM. As said in this issue https://www.codehawks.com/finding/clm9ckp7m000lw9ecanewhihe

PatrickAlphaC commented 1 year ago

I think this warrants fixing, it could be argued that it's invalid since the owner could take steps to rescue funds, but it's a decent deviation from the intended functionality of the protocol, so I'm keeping it as a low.

Cyfrin / 2023-08-sparkn