Blacklisted STADIUM_ADDRESS address cause fund stuck in the contract forever

[codehawks-bot]

Blacklisted STADIUM_ADDRESS address cause fund stuck in the contract forever

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/tree/main/src/Distributor.sol#L164

Summary

The vulnerability relates to the immutability of STADIUM_ADDRESS. If this address is blacklisted by the token used for rewards, the system becomes unable to make transfers, leading to funds being stuck in the contract indefinitely.

Vulnerability Details

1. Owner calls setContest with the correct salt.
2. The Organizer sends USDC as rewards to a pre-determined Proxy address.
3. STADIUM_ADDRESS is blacklisted by the USDC operator.
4. When the contest is closed, the Organizer calls deployProxyAndDistribute with the registered contestId and implementation to deploy a proxy and distribute rewards. However, the call to Distributor._commissionTransfer reverts at Line 164 due to the blacklisting.
5. USDC held at the Proxy contract becomes stuck forever.

// Findings are labeled with '<= FOUND'
// File: src/Distributor.sol
116:    function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
117:        ...
154:        _commissionTransfer(erc20);// <= FOUND
155:        ...
156:    }
                ...
163:    function _commissionTransfer(IERC20 token) internal {
164:        token.safeTransfer(STADIUM_ADDRESS, token.balanceOf(address(this)));// <= FOUND: Blacklisted STADIUM_ADDRESS address cause fund stuck in the contract forever
165:    }

Impact

This vulnerability is marked as High severity because a blacklisted STADIUM_ADDRESS would lead to funds being locked in the Proxy address permanently. Funds are already held in the Proxy, and the Proxy's _implementation cannot be changed once deployed. Even the ProxyFactory.distributeByOwner() function cannot rescue the funds due to the revert.

Tools Used

Manual Review

Recommendations

It is recommended to allow STADIUM_ADDRESS to be updatable by a dedicated admin role to avoid token transfer blacklisting. Moreover, since STADIUM_ADDRESS is no longer immutable, storage collision should be taken into account.

[PatrickAlphaC]

Low chance of this contract getting blacklisted, and low impact as you could just redeploy.

[leeftk]

What do you mean by redepoly? Aren't the funds sent to the Proxy, and isn't it part of the protocol design to not allow refunds of funds meaning that they would be locked forever in the contract?

[kosedogus]

Sponsor mentioned that stadium address is owner:

auditor: so STADIUM_ADDRESS will be set by the protocol. correct me if I'm wrong
sponsor: Nope the STADIUM_ADDRESS is the owner of the protocol.

So chance of getting blacklisted for STADIUM_ADDRESS is not low. Funds are stored at Proxy, and when the time comes, they are distributed between winners and STADIUM_ADDRESS. There is no way to redeploy and save the funds. I recommend checking carefully. So impact is high (loss of all funds for given proxy). Hence this issue should be valid high.

[ShaheenRehman]

Appeal!

I agree with the preceding remarks.

Even if we say that the likelihood of StadiumAddr getting blacklisted is low but the significant and lasting impact it will create on the protocol and users' funds is critical which makes it a minimum Medium severity issue and could potentially warrant a High rating.

Pls Judge Sahab, Consider reading this issue more vigintally.

Thanks!

[serial-coder]

@PatrickAlphaC

Escalate

This issue should be (at least) MEDIUM. Redeploying any contracts cannot solve the issue.

The following is an excerpt from my Vulnerability Details section (#899).

Suppose the fee collector (STADIUM_ADDRESS) gets blacklisted. In that case, the token transferred to the Proxy will be locked up permanently. The root cause of this vulnerability is that the STADIUM_ADDRESS is an immutable variable. Furthermore, the Proxy is also pointing to the immutable Implementation contract.

Even the stuck token's recovery process could not deal with this vulnerability since the recovery process also shares the same token transfer logic in the _distribute(), as described previously.

Moreover, this vulnerability will affect all contests' Proxy contracts since they point to the same Implementation contract.

[0xhahax0]

Not sure what is meant with redeploy. Funds will be stuck forever, re-deploying Distributor.sol will not do anything since the address of Distributor.sol is used to calculate the salt that is used to calculate the pre-determined proxy address. Using a new Distributor.sol will lead to a new salt which leads to a new proxy address which means you will never being able to retrieve the funds that are stuck.

High is warranted here.

[PatrickAlphaC]

Agree with appeals. Sorry on my cryptic comment. Went a bit too fast there.

Likelihood: Low Impact: High