Sparkn

CodeFox Inc.
DeFiFoundryProxy
15,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Organizer can steal the tokens for his contest

stoicov

Summary

Malicious organizer can set close time to current time, deploy and distribute the tokens sent to the precomputed proxy address to himself

Vulnerability Details

Let's take a look at the deployProxyAndDistribute function in ProxyFactory.sol :

function deployProxyAndDistribute(bytes32 contestId, address implementation, bytes calldata data)
public
returns (address)
{
bytes32 salt = _calculateSalt(msg.sender, contestId, implementation);
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
// can set close time to current time and end it immediately if organizer wish
if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
address proxy = _deployProxy(msg.sender, contestId, implementation); // bytes32 salt = _calculateSalt(organizer, contestId, implementation); //address proxy = address(new Proxy{salt: salt}(implementation));
_distribute(proxy, data); // (bool success,) = proxy.call(data);
return proxy;
}

Notice the comment above the if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed(); check . It says :
// can set close time to current time and end it immediately if organizer wish . This is allowed by design in the protocol. However here is what can happen due to this feature:

  1. The owner sets a contest with a particular organizer address

  2. In this scenario the sponsor must not be the organizer himself. So a sponsor loads the precomputed proxy address from the getProxyAddress function with tokens

  3. Then the organizer calls deployProxyAndDistribute with the array of winners being his address and the percentage being 95 percent or 95000 basis points , so the checks in Distributor.sol._distribute() work

  4. The result is that the organizer transfers 95 percent of the prize pool to himself.

Impact

Failure to distribute earned prizers to supporters

Tools Used

Manual Review

Recommendations

I believe this feature should be disallowed. For that purpose the check in deployProxyandDistribute on L134 can be rewritten to :
if (saltToCloseTime[salt] >= block.timestamp) revert ProxyFactory__ContestIsNotClosed();

Additionaly it can be checked that winners[i] != tx.origin in the Distributor.sol._distribute() function. Tx.origin is used here because it will be equal to the initial caller of the deployProxyAndDistribute which is the orgranizer. I am convinced that none of the risks associated with the use ot tx.origin can endanger the protocol if it is used as I proposed above.

So , having in mind my missing address(0) check for winners[i] finding, the L125-L133 section of the Distributor.sol._distribute() method can look like this

uint256 winnersLength = winners.length; // cache length
for (uint256 i; i < winnersLength;) {
if(winners[i] == address(0) || winners[i] == tx.origin) revert ProxyFactory_NoZeroAddress();
uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
erc20.safeTransfer(winners[i], amount);
unchecked {
++i;
}
}

The second mitigation also keep the feautre of having the right to end a contest immediately as an organizer.

Support

FAQs

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