codehawks-bot commented 1 year ago

Organizers are not incentivized to deploy and distribute to winners causing that winners may not to be rewarded for a long time and force the protocol owner to manage the distribution

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L127

https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L152

Summary

The organizer can deploy and distribute to winners at any time without restriction about the contest expiration time EXPIRATION_TIME causing that the winners to be unable to receive their rewards for a long time.

Vulnerability Details

The organizer can execute the deployProxyAndDistribute() function to deploy the distribute contract and execute the distribution to winners. The only restriction is that the current time should be greater than contest close time (code line 134).

File: ProxyFactory.sol
127:     function deployProxyAndDistribute(bytes32 contestId, address implementation, bytes calldata data)
128:         public
129:         returns (address)
130:     {
131:         bytes32 salt = _calculateSalt(msg.sender, contestId, implementation);
132:         if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
133:         // can set close time to current time and end it immediately if organizer wish
134:         if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
...
...

In the other hand, the owner can execute deployProxyAndDistributeByOwner() function after the contest expiration time (code line 187).

File: ProxyFactory.sol
179:     function deployProxyAndDistributeByOwner(
180:         address organizer,
181:         bytes32 contestId,
182:         address implementation,
183:         bytes calldata data
184:     ) public onlyOwner returns (address) {
185:         bytes32 salt = _calculateSalt(organizer, contestId, implementation);
186:         if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
187:         if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
...
...

The problem is that the organizer can execute the deployProxyAndDistribute() function after the contest close time without restriction of time. The organizer can wait indefinitely causing the winners not to be rewarded for a long time and force the owner to execute the distribution manually via deployProxyAndDistributeByOwner().

Additionally, the organizers are not incentivized to deploy and distribute to winners.

Impact

The malicious organizer can wait indefinitely until the owner calls deployProxyAndDistributeByOwner(). The bad/malicious behaivour of the organizer can cause the winners to be unable receive rewards for a long time AND force the owner to execute manually deployProxyAndDistributeByOwner(). That affects the protocol because rewards are not assigned in time AND the protocol owner needs to manage manually the deploy and distribution in order to not affect the protocol's reputation and winners.

Additionally the organizers are not incentivized to deploy and distribute to winners causing to the protocol owner to execute manually the deployProxyAndDistributeByOwner().

Tools used

Manual review

Recommendations

Add a validation that the organizer distribution must be between the saltToCloseTime and the EXPIRATION_TIME. Same in deployProxyAndDistributeBySignature()

    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();
++      if (saltToCloseTime[salt] + EXPIRATION_TIME < block.timestamp) revert();
        address proxy = _deployProxy(msg.sender, contestId, implementation);
        _distribute(proxy, data);
        return proxy;
    }

Additionally, there should be a penalization to the organizer or an incentive to deploy and distribute in time to winners.

Frankcastleauditor commented 1 year ago

ESCALATE this issue should be invalid because there is no way to incentivize the organizer to deploy and distribute the reward , and the recommendation does not incetivize the organizer to deploy and distribute the rewards , it only prevents the organizer from distributing the reward after the expiration time , so how this could incentivize the oganizer to distribute the rewards?? and addition to that , the organizer is supposed to be trusted and there is a second way to distribute the reward by the owner .

Jnrlouis commented 1 year ago

I agree with @Frankcastleauditor, this should be invalid. I believe the recommendation shouldn't be implemented as it doesn't prevent the stated issue. Also, I think the whole point of having another function were the owner can distribute rewards is to prevent this issue - so this is intended behavior. Not to mention that the organizer is a trusted role, and it would be counter intuitive to regard them as malicious.

0xSandyy commented 1 year ago

After reviewing above appeals, I suggest the judge to look at other duplicates of this issue for proper recommendation before making final decision. Thank you!

Cyfrin / 2023-08-sparkn