No validation of `implementation` parameter in `setContest()`

[codehawks-bot]

No validation of implementation parameter in setContest()

Severity

Low Risk

Relevant GitHub Links

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

Summary

The setContest() function in ProxyFactory is used to configure a contest's properties. The implementation parameter is used to compute a salt for counterfactual proxy contract deployment. If an incorrect implementation is used to generate a proxy address for a contest, and this mistake is not noticed by the owner or organizer, any contest rewards sent to the proxy address will be lost.

Vulnerability Details

The setContest() function in ProxyFactory is used to configure a contest's properties:

src/ProxyFactory.sol:105
105:     function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)
106:         public
107:         onlyOwner
108:     {
109:         if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress();
110:         if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) {
111:             revert ProxyFactory__CloseTimeNotInRange();
112:         }
113:         bytes32 salt = _calculateSalt(organizer, contestId, implementation);
114:         if (saltToCloseTime[salt] != 0) revert ProxyFactory__ContestIsAlreadyRegistered();
115:         saltToCloseTime[salt] = closeTime;
116:         emit SetContest(organizer, contestId, closeTime, implementation);
117:     }

The organizer, contestId and implementation parameters are used to compute a salt for counterfactual contract deployment. The resultant proxy address is then used by the organizer to post contest rewards to. Since there is no validation of the implementation address, it is possible to provide an incorrect implementation address. If this happens and goes unnoticed, contest rewards posted to the resultant proxy address will be lost forever.

Proof of Concept

1. owner invokes setContest() with valid values for organizer, contestId, and closeTime, but an incorrect value for implementation
2. owner invokes getProxyAddress() with the salt (see footnote [1] below) and the same incorrect value for implementation again, receiving a proxy address in response
3. owner sends the proxy address, which is linked to the incorrect implementation address, to the organizer
4. organizer sends rewards to the proxy address
5. Rewards cannot be distributed or rescued because the proxy address is irreversibly linked to the wrong implementation address

Impact

This a low risk issue because there is an opportunity for the owner to notice their mistake twice before sending the proxy address to the organizer, and the organizer could also review the event log to verify the implementation address.

Tools Used

Manual analysis.

Recommendation

Remove the implementation parameter from setContest, and instead add a currentImplemenation state variable to ProxyFactory with a 2-phase configure + commit protocol to avoid misconfiguring contests. When configuring the currentImplementation parameter, it should be validated to ensure contract code is deployed at the specified address, and a signature function should be invoked at the address to confirm that the contract at the address is conformant with the SPARKN protocol.

Footnotes

[1] QA: There is no easy way for owner or organizer to invoke getProxyAddress as salt is not returned by setContest(), nor emitted in the SetContest event. The contract should either return salt in setContest, or include it in the emitted SetContest event. The unit tests are constructing the proxy address by hand -- this is dangerous.

[PatrickAlphaC]

https://github.com/Cyfrin/2023-08-sparkn/issues/262#issuecomment-1709450774