Summary

The ProxyFactory::setContest() lacks validating the case of the implementation param pointing to a target address with no contract. As a result, all tokens sent to the Proxy address will be stuck forever.

Vulnerability Details

The Proxy contract was designed as an escrow for distributing tokens to contest winners. By design, a contest organizer or sponsor must send tokens to an address of the contest's Proxy before the Proxy gets deployed. The tokens can be permanently stuck in the Proxy address if a mistake occurs.

Every contest will be initiated by an owner via the setContest(). The function will verify the implementation param against the address(0). However, the verification cannot guarantee that the Proxy will function properly.

Specifically, the setContest() lacks checking that the implementation param must point to an address with a contract bytecode. If an incorrect address is inputted by mistake, such as address(1), the Proxy will be bricked, locking away all tokens.

This mistake could not be undone since the inputted implementation param will be used to compute a salt for the Proxy. The implementation param will no longer be updated after executing the setContest().

• The implementation param is checked against the address(0) only: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L109

• The implementation param is used to compute a salt for a contest's Proxy: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L113

Further Explanation

To explain further the vulnerability.

The Proxy contract will be deployed and triggered by calling the following functions after sending prize tokens.

1. deployProxyAndDistribute()

2. deployProxyAndDistributeBySignature()

3. deployProxyAndDistributeByOwner()

Below is the Proxy contract's snippet. The inputted implementation param will be assigned to the immutable variable _implementation in the constructor(). When the Proxy is invoked to distribute tokens, the fallback() will be triggered to execute the delegatecall() to the target implementation address.

If an incorrect address was inputted by mistake, the delegatecall() will execute nothing and return a success status. In other words, all tokens will be stuck forever in the Proxy contract.

• The _implementation is an immutable variable initialized in the Proxy's constructor(): https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Proxy.sol#L40-L46

• Proxy executes the delegatecall() to the target implementation address: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Proxy.sol#L52-L56

Impact

The setContest() lacks validating the case of the implementation param pointing to a target address with no contract. As a result, all tokens sent to the Proxy address will be stuck forever.

To clarify the vulnerability, although only an owner can execute the setContest() 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

Further validating that the implementation param must point to an address with a contract bytecode, as follows.