(Note: I'm aware of the non-whitelisted tokens issue that the protocol team is already aware of. This finding of mine is not related to that known issue at all. My finding considers whitelisted tokens only.)
Edge case related to combo of input validation and logic bug:
To avoid any confusion/misunderstandings here, let me make the following clear:
The bug I discovered disables the funds rescue functionality completely.
The comment on L196 of factory contract does not hold during this bug:
"@notice Owner can rescue funds if token is stuck after the deployment and contest is over for a while"
Passing the wrong values for parameter closeTime
in setContest() function will result in the contest ending before any supporters could submit their proposals, cause proxy contract deployment to fail/revert, and with the sponsor/organizer's ERC20 tokens stuck in the proxy address with no means of getting it out.
ALL proxy deployment & token distribution functions will revert, including the distributeByOwner() function for rescuing stuck tokens.
(Note: It is assumed that the organizer can transfer the ERC20 tokens to the proxy address independent of the protocol state, meaning that the organizer can transfer the contest funds to the proxy address either (before?) or soon after the setContest() function is executed, and regardless of the outcome of the setContest() execution.)
Since the setContest() function has this check during the if statement
on L110:
closeTime < block.timestamp
,
it makes it possible through either user error or ill intent by rogue user/owner/attacker, to pass a value for parameter closeTime
that is same as block.timestamp
for current function call, i.e. where the following will pass the if statement
check:
closeTime == block.timestamp
This effectively sets saltToCloseTime[salt] == block.timestamp
, too.
Then afterwards, by calling any of the deploy proxy or distribute functions, the fallback() in Proxy.sol should be called successfully, but during the delegatecall() execution in the Distributor.sol implementation contract, the internal _distribute() function will revert at L125, because winners.length == 0, since no proposals could be submitted:
if (winners.length == 0 || winners.length != percentages.length) revert Distributor__MismatchedArrays();
And therefore, all of the below functions will revert too when called (they will revert during _distribute() execution):
deployProxyAndDistribute()
deployProxyAndDistributeBySignature()
deployProxyAndDistributeByOwner()
distributeByOwner()
This completely DoS the normal behaviour of the protocol.
The following case is related and is covered in the recommendations section below:
when closeTime > block.timestamp
but closeTime - block.timestamp
== too little time for one or more supporters to submit their proposals.
No ERC20 contest tokens can be transferred out of proxy address because proxy contract cannot be deployed, effectively rendering the contest funds stuck until a rescue is implemented...(see recommendations section)
The expected/intended ability to transfer the tokens out of proxy address depends totally on the below normal protocol behaviour/steps to complete without revert:
set contest then 2) transfer funds to proxy address then 3) after contest ends, deploy proxy contract and distribute funds same time.
The common denominator here is that the protocol is currently completely vulnerable to getting organizer's ERC20 tokens stuck in proxy address, whenever winners.length == 0
, during _distribute() execution in distributor contract.
And one guaranteed way to get to that point is via either closeTime == block.timestamp
or closeTime - block.timestamp
being too short time period for supporters to submit proposals.
In both cases, the setContest() function call will execute successfully but the contest will end before any supporters can submit proposals, i.e. before any winners can be selected.
VSC.
Manual.
Two recommendations:
Consider implementing a new updated distributor/implementation contract that contains functionality to get the ERC20 tokens out of the proxy address in case of emergency. However, the above suggestion should be a last resort as I assume the protocol/project team are trying to stay away from such approaches, and instead I recommend the below better approach which prevents the problem from happening in the first place.
Better solution: whether closeTime == block.timestamp
or closeTime > block.timestamp
, if the time duration between closeTime
and block.timestamp
is too small then it will leave too little time for any supporters to submit their proposals/solutions. This alone should be sufficient motivation to implement a minimum time duration for each contest, to ensure enough time available for at least one or more supporters to submit their proposals.
The below bug fix suggestion should help to ensure sufficient time period for supporters to submit their proposals, and it will prevent the ERC20 funds from getting stuck in the proxy address.
Bug fix:
Instead of using:
closeTime < block.timestamp
Rather use:
closeTime < block.timestamp + MIN_CONTEST_PERIOD
Where the constant state variable MIN_CONTEST_PERIOD
>= a reasonable time period for one or more supporters to submit their proposals/solutions.
(Another potential solution for getting the stuck tokens out of the proxy address would be to add the STADIUM_ADDRESS
to the empty winners
mapping, if that was at all possible:
address[] memory winners)
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.