Sparkn

Summary

In the current function's implementation, the used signature definition does not include the implementation address in the signed message. This allows anyone to replay any of the Organizer's previous signatures but change the implementation address to a different one.

From this, any malicious user can successfully call this public function "in the name of the Organizer" and distribute rewards on other contests this same Organizer owns, that were not intended to be distributed in such a way.

Vulnerability Details

The ProxyFactory.deployProxyAndDistributeBySignature() function calculates the digest as:

this means that an Organizer's signature results from the permutation of the contestId and the data parameters.

For example, an Organizer can sign that they want to distribute rewards (distribute() function call) on contestId 1.

The function will thus be called with parameters (OrganizerA, contest1, implementationA, sigX, distributeCall1)

Considering that the protocol architecture uses the Proxy -> Implementation pattern, it is expected that in the future the Distributor.sol contract will eventually change, in the form of a freshly deployed contract, and thus a new implementation address will be used.

Given that the implementation parameter is not included in the signature, anyone can make use of any of the Organizer's previous signatures and call the deployProxyAndDistributeBySignature() function with a different implementation address, which will be successfully executed.

Following the previous example, the malicious user can call the function with parameters (OrganizerA, contest1, implementationB, sigX, distributeCall1) successfully, thus potentially distributing the contest's rewards as per the signed message that was originally intended for another contest.

Impact

A malicious user can benefit from replaying the reward distribution of a previous contest from the same Organizer, and distribute rewards on a different contest that the distribution was not intended for.
This could be done either:

• For benefiting a particular party (e.g. An address that was prominently rewarded in the previous contest)

• For the sake of griefing the protocol (e.g. Distributing rewards to unintended winners, or distributing rewards on a contest that was not intended to be distributed yet)

I acknowledge the fact that this vulnerability is not trivial to exploit, as it requires the replayed contest and the new contest to both have the same contestId. Given that this value comes from off-chain, I have no way of knowing what the selection process for this is, but I still decided to report this to bring attention to it. In my own interpretation, this value is tightly coupled to the implementation, and thus could potentially be represented as a counter like:

in which case the replay issue would be present. (And these are valid salt permutations)

Either way, current signature is unnecessarily loose and could benefit from including the implementation address in the signed message to prevent potential issues in the future.

Tools Used

Manual Review

Recommendations

The implementation address should be included in the signed message, so that the deployProxyAndDistributeBySignature() function can only be called with the exact same parameters that were originally signed by the Organizer.

It is worth mentioning that this function can still suffer from replayed calls, as the signature is never invalidated after use. However, I do not consider this to be a vulnerability, as the deployProxyAndDistributeBySignature() function is only useful the first time when there are ERC20 tokens in the to-be-deployed Proxy contract. Subsequent calls will fail, as the Proxy contract will already exist.

Additionally, the test for this case does not properly cover this issue. The test only fails because the calculated salt ends up in an invalid key for the mapping and thus no registered contest is found. But if the combination of organizer, contestId and implementation results in a valid contest, the function will be successfully executed, which is not the case being tested.