codehawks-bot commented 1 year ago

Organizers and Sponsors might send tokens to a wrong address, due to lack of checks in `getProxyAddress()`

Severity

High Risk

Relevant GitHub Links

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

Summary

As outlined in the project README, when the owner initiates a new contest, the organizer/sponsor is responsible for sending tokens to a "predicted" proxy address. This address is determined by a combination of:

A generated salt (which relies on the organizer, contestId, and implementation details)
The implementation contract address.

The organizer sends the tokens PRIOR to the proxy deployment by using the ProxyFactory:getProxyAddress(bytes32 salt, address implementation) view function to obtain the future deployment address of the proxy.

However, the problem is that the ProxyFactory:getProxyAddress(bytes32 salt, address implementation) function lacks a verification mechanism for the accuracy of the provided parameters.

This can lead to situations where the organizer inputs incorrect parameters, resulting in the receipt of an inaccurate proxy address. If tokens are sent to this wrong address, they will be lost forever.

Vulnerability Details

The ProxyFactory:getProxyAddress(bytes32 salt, address implementation) function essentially provides a predictable proxy address by using the given salt and implementation inputs.

During the contest setup process and the address retrieval action, the salt is likely to be computed on the frontend based on user-supplied parameters like organizer (connected wallet), contestId (User supplied?), and implementation (User supplied?).

However, even a small error in any of these parameters could result in an incorrect address calculation. This incorrect address would then be returned to the organizer, prompting them to send valuable tokens to an erroneous destination, effectively leading to permanent loss.

To mitigate this issue, a solution could involve storing all the contest information within the Factory contract and implementing a check to revert in case incorrect parameters are provided to the function. I'll outline a comprehensive solution to address this issue in the Recommendation section.

Impact

Likelihood - High

Impact - High

In my opinion this scenario is very likely to happen especially given the platform's plans to host numerous contests in the future. Additionally, the organizers or sponsors might lack technical expertise, increasing the likelihood of errors.

The argument that "This will be handled off-chain and validation will occur in the user experience" is not acceptable. Even within the UI, the organizer will link their wallet to the website. The address retrieval depends on the connected wallet's address (which impacts the salt).

Consequently, if the organizer accidentally links a different wallet to the website (a very possible scenario), then promptly retrieves the proxy address and sends tokens, those tokens will be irretrievably lost. Given the substantial probability of this scenario and the potential token loss, I classify this as a significant vulnerability.

Tools Used

Manual Review

Recommendations

To fix this problem and prevent token loss, my suggestion is to store the implementation contract address for each contest in the Factory contract. In fact, I would go a step further and propose storing the complete contest information within the factory contract. This approach would facilitate effortless retrieval of data in the future:

Struct Contest {
  address organizer;
  bytes032 contestId;
  uint256 closeTime;
  address implementation;
}

// salt --> Contest
mapping(bytes32 => Contest) contests;

Store the contest details in the ProxyFactory:setContest function, by adding this (you can get rid of saltToCloseTime since you store the entire contest info in the contests mapping):

contests[salt] = Contest(organizer, contestId, closeTime, implementation);

Then, modify the ProxyFactory:getProxyAddress function and add parameter validation:

Contest memory contest = contests[salt];
if(contest.implementation != implementation) revert ProxyFactory__ContestDoesNotExist();

This enhancement will significantly mitigate the risk associated with incorrect input parameters and ensure the secure transfer of tokens during the contest setup process.

RealJohnnyTime commented 1 year ago

In my opinion, this should be classified as High, given the likelihood of this occurring, as well as the severity of losing funds (please refer to my Impact section). Also the likelihood and damage that will be done is much higher than this validated high.

I also believe my submission should be chosen for the report since the current selected one (by "serialcoder") does not resolve all of the issues, such as the wrong organizer being passed to the function, which my recommendation addresses (please refer to my Recommendations section).

PatrickAlphaC commented 1 year ago

https://github.com/Cyfrin/2023-08-sparkn/issues/897#issuecomment-1716760265

Cyfrin / 2023-08-sparkn