Sparkn

CodeFox Inc.
DeFiFoundryProxy
15,000 USDC
View results
Submission Details
Severity: high

ProxyFactory::setContest() - Token lockup risk for ERC20 tokens(sponsor funds) in proxy address when `closeTime == block.timestamp`.

Summary

(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.)

Vulnerability Details

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.

Impact

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:

  1. 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.

Tools Used

VSC.
Manual.

Recommendations

Two recommendations:

  1. 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.

  2. 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)

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.