Summary

The ProxyFactory::distributeByOwner() can unexpectedly drain all tokens from an incorrect contest's Proxy contract. The affected contest can be under any organizer and does not need to expire before.

Vulnerability Details

The distributeByOwner() receives the proxy (in L206) and other input parameters. The function will compute a salt from the organizer, contestId, and implementation parameters and then validate that the contest corresponding to the computed salt must expire before executing the _distribute().

Apparently, the distributeByOwner() does not execute the _distribute() by passing the Proxy address derived from the previously computed salt. Specifically, the inputted proxy parameter will be passed instead.

If the proxy parameter is mistakenly inputted, the proxy parameter can point to another contest's Proxy contract.

• The proxy param is inputted by an owner: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L206

• The proxy param can point to an incorrect Proxy contract: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L217

Impact

The distributeByOwner() can unexpectedly drain all tokens from an incorrect contest's Proxy contract. The affected contest can be under any organizer and does not need to expire before (but must be closed already).

To clarify the vulnerability, although only an owner can execute the distributeByOwner() and the owner is trusted, the incident can occur by mistake (i.e., this vulnerability is not about any centralization or trust risks; it is about the risks of input mistakes only).

The likelihood is considered LOW (since the owner is expected to do due diligence). The impact is considered HIGH. Therefore, the severity is considered MEDIUM.

Tools Used

Manual Review

Recommendations

Derive the Proxy address from the computed salt by consulting the getProxyAddress(salt, implementation), as shown below.