Funds will be stuck forever if implementation address is wrong when calling setContest()

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L109

Summary

Implementation address is used to calculate as well as deploy the address of the proxy. If this address is wrong, funds will be stuck forever as the wrong implementation contract doesn't have the necessary logic/permission to transfer funds.

Vulnerability Details

In setContest(), implementation address is only checked if it's not zero address. This logic lacks the validation if the right implementation is used.

if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress();

Impact

Sponsor(s) fund is lost.

POC

Paste this function into ProxyFactoryTest.t.sol:

    function testImplementationFailsToRevertWhenWrong() public {
        bytes32 randomId = keccak256(abi.encode("Jason", "001"));
        vm.startPrank(factoryAdmin);
        proxyFactory.setContest(organizer, randomId, block.timestamp + 1 days, address(0x1));
        vm.stopPrank();
    }

To run this, type forge test --mt testImplementationFailsToRevertWhenWrong

Contest is still successfully created even though the implementation address is 0x1, which is likely not to be the result calculated from getProxyAddress() in reality.

Tools Used

Manual Analysis

Recommendations

Implement a check for the appropriate implementation address.

+   import {Distributor} from "./Distributor.sol";
    ...
+   error ProxyFactory__MismatchedImplementation();
    ...

    function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)
        public
        onlyOwner
    {
        if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress();
+       if (address(this) != _isImplementation(implementation)) revert ProxyFactory__MismatchedImplementation();
        if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) {
            revert ProxyFactory__CloseTimeNotInRange();
        }
        bytes32 salt = _calculateSalt(organizer, contestId, implementation);
        if (saltToCloseTime[salt] != 0) revert ProxyFactory__ContestIsAlreadyRegistered();
        saltToCloseTime[salt] = closeTime;
        emit SetContest(organizer, contestId, closeTime, implementation);
    }

+   function _isImplementation(address implementation) internal view returns (address) {
+       return (address _FACTORY_ADDRESS,,,) = Distributor(implementation).getConstants();
+   }

Cyfrin / 2023-08-sparkn