Sparkn

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

`distributeByOwner()` accepts arbitrary `proxy` parameter

Summary

The distributeByOwner() function in ProxyFactory is used to perform an emergency distribution e.g. when the contest organizer has failed to perform a distribution timeously. The contestId parameter indicates the contest in question, and the proxy parameter must point to the proxy contract that owns the contestId reward token pool. There is no validation that proxy is the correct proxy address for contestId, therefore it is possible for a different contest's reward pool to be irreversibly distributed to the winners of contestId if the wrong proxy address is specified.

Vulnerability Details

The distributeByOwner() function in ProxyFactory is used to perform an emergency distribution:

src/ProxyFactory.sol:205
205: function distributeByOwner(
206: address proxy,
207: address organizer,
208: bytes32 contestId,
209: address implementation,
210: bytes calldata data
211: ) public onlyOwner {
212: if (proxy == address(0)) revert ProxyFactory__ProxyAddressCannotBeZero();
213: bytes32 salt = _calculateSalt(organizer, contestId, implementation);
214: if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
215: // distribute only when it exists and expired
216: if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
217: _distribute(proxy, data);
218: }

If owner accidentally provides the proxy address for a different contestId in the call to distributeByOwner(), the ProxyFactory._distribute() call on line 217 will irreversibly send rewards for that other contest to the winners of contestId (the list of winners is encoded in the data parameter).

On the side of the proxy, in the default Distributor._distribute() implementation there is no contest ID validation prior to distribution (indeed, the contest ID is not available at all). The token parameter is validated according to a whitelist, but the whitelist is very short and tokens chosen for rewards are very likely to overlap between contests e.g. JPYC or USDC is very likely to be used. Rewards are specified as percentages, so these will scale with the size of the contest reward pool and thus not prevent mistakes either.

Proof of Concept

  1. Alice organizes contestId A with USDC rewards

  2. Alicia organizes contestId B, with a close time that happens to be 7 days after contestId A, also with USDC rewards

  3. Contest A ends, and 7 days later content B ends

  4. Alice hasn't distributed rewards for contest A yet, so owner steps in and invokes distributeByOwner(), specifying the winners of contest A in the data parameter, but accidentally specifies the proxy for contest B instead of contest A

  5. Alicia's contest B rewards are incorrectly and irreversibly distributed to the winners of contest A

Impact

This is a medium risk issue because there is a permanent loss of funds at risk, but there are external conditions required (owner error and overlapping contest periods).

Tools Used

Manual analysis.

Recommendation

The proxy parameter is error prone and unnecessary. Replace proxy with an implementation address to match the pattern used everywhere else, and use getProxyAddress(bytes32 salt, address implementation) to determine the correct value for proxy.

Support

FAQs

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