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();
+ }