Sparkn

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

Owner can bypass expired check in `ProxyFactory::distributeByOwner`

Summary

Due to the lack of validation between the proxy parameter and the remaining parameters passed to the ProxyFactory::distributeByOwner function, it is possible to call _distribute on a non-expired contest.

Vulnerability Details

The function ProxyFactory::distributeByOwner serves the purpose of enabling the owner to distribute funds that are were sent to the distributor proxies after its deployment and thus are stuck there. By examining the code of ProxyFactory::distributeByOwner provided below, it first checks whether the contest has been registered. Subsequently, checks if the contest has surpassed its expiration period. Only then it calls ProxyFactory::_distribute.

if (proxy == address(0)) revert ProxyFactory__ProxyAddressCannotBeZero();
bytes32 salt = _calculateSalt(organizer, contestId, implementation);
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
// distribute only when it exists and expired
if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
_distribute(proxy, data);

However there is no validation that the proxy address argument matches the address corresponding to the contest salt. If the caller sends the organizer, contestId, implementation values corresponding to a valid expired contest, but sends the proxy address of non-expired contest, the saltToCloseTime[salt] == 0 and saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp checks will pass and _distribute will be called on a non-expired contest.

This allows the owner (either maliciously or by mistake) to distribute the rewards of a contest before its expiration time. The code comments make it clear that this is not the intended behaviour ("// distribute only when it exists and expired"). However as the owner role is supposed to be trusted, I classify this as a low vulnerability issue.

Impact

The internal logic of the ProxyFactory::distributeByOwner does not corresponds to its expected behaviour. Owner can call ProxyFactory::distributeByOwner on a non-expired contest.

Tools Used

Manual Review

Recommend Mitigation

Consider computing the proxy address derived from the salt parameter and check that proxy matches the computed one. As shown in the diff below.

diff --git a/ProxyFactory.sol b/ProxyFactory.mod.sol
index 7f10aeb..b212873 100644
--- a/ProxyFactory.sol
+++ b/ProxyFactory.mod.sol
@@ -214,6 +214,8 @@ contract ProxyFactory is Ownable, EIP712 {
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
// distribute only when it exists and expired
if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
+ address calculatedProxy = getProxyAddress(salt, implementation);
+ require(proxy == calculatedProxy, "Incorrect proxy address");
_distribute(proxy, data);
}

Support

FAQs

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