The lack of 0 address and uniqueness checks for winners may result in unexpected situations.

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol#L116-L156

Summary

In the Distributor.sol::_distribute function, it is recommended to add checks for winners to ensure that there are no occurrences of 0 addresses or duplicate values to avoid unexpected situations. Otherwise, it may result in anomalies such as repeatedly distributing rewards to the same winner in a single competition or distributing rewards to a winner with a 0 address.

Vulnerability Details

1. From a developer's perspective, there are two scenarios for the competition:

   1. "Scenario 1": Each competition has only one winner.
   2. "Scenario 2": Each competition has multiple winners.

2. In "Scenario 1", the uniqueness of the winner is ensured, but a 0 address check is still required. However, in "Scenario 2", which is the case for Sparkn, a 0 address check is needed as well as ensuring the uniqueness of the winners.

Impact

1. In the reward distribution within the _distribute function, the lack of 0 address check for the winners may result in rewards being mistakenly distributed to the 0 address.

2. In the reward distribution within the _distribute function, the absence of uniqueness checks for the winners may lead to duplicate distribution of rewards to the same winner.

Tools Used

• Manual Review

Recommendations

1. Perform a uniqueness check on the winner to ensure that rewards are not repeatedly distributed to the same winner.
2. Conduct a 0 address check on the winner to ensure that rewards are not distributed to a 0 address.

Zero Address Proxies

Severity

Medium Risk

Relevant GitHub Links

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

Summary

Returned proxies can be zero addresses
Additionally low level calls are not checked for contract existence or zero address

Vulnerability Details

Since many of the deploy functions can be called many times as create2 does not fail when called again with same salt but returns address(0) after first call as there is already a contract there

function deployProxyAndDistribute(bytes32 contestId, address implementation, bytes calldata data)
// example deploy functions  use code below but don't check if proxy == address(0)
 address proxy = _deployProxy(organizer, contestId, implementation);
 _distribute(proxy, data);
// _distribute uses low level call does not check for contract existence so will always bring back success 

Consider the following process flow (maybe its a front end that doesn't check values but assumes values from function calls are correct and chain feeds them into the next step etc)

1. Owner setsContest => setContest(0xOrganizer, 0xContestId01, closeTime0001, 0xImplementation) -> salt result is 0xSalt01
2. Process flow calls => getProxyAddress(0xSalt01, 0xImplementation) -> proxy result is 0xProxy01
3. Process flow informs Sponsors users to use 0xProxy01 to send funds, for competition etc
4. Sponsor01 sends WhitelistedTokensA to 0xProxy01 and NotWhielistedTokensB to 0xProxy01
5. Competitions finishes and owner or organizer informed need to distribute funds plus rescue NotWhielistedTokensB(maybe this is built into front end in some chain)
6. Owner calls function deployProxyAndDistributeByOwner(0xProxy01, 0xOrganizer, 0xContestId01, 0xImplementation, data) which is based on 0xSalt01 and returns correctly 0xProxy01 address stored in front end chain or process flow
   [At this stage idea is to use this return value to feed into function distributeByOwner(
   address proxy, // inject here to rescue NotWhielistedTokensB
   address organizer,
   bytes32 contestId,
   address implementation,
   bytes calldata data
   )]
7. However since nothing restricts deployProxyAndDistributeByOwner(...) to be called again, owner calls this by error e.g click button again on front end or reinitatied process flow ...
   deployProxyAndDistributeByOwner(0xProxy01, 0xOrganizer, 0xContestId01, 0xImplementation, data) since same salt
   address proxy = address(new Proxy{salt: salt}(implementation)); will return address(0)
   therefore this repeated call now feeds to front end or process flow address(0) to saving funds
8. Owner now tries to save funds by calling function distributeByOwner(
   address(0),
   0xOrganizer,
   0xContestId01,
   0xImplementation,
   data [pass in token address for NotWhielistedTokensB ]
   )
   Since _distribute(proxy, data) involves low level call address(0).call(data) it will return success and function distributeByOwner() will succeed and owner will relax that NotWhielistedTokensB where rescued when nothing was done

Impact

Results in various deploy and distribute functions being able to be called many times with same values; wheres its only useful the first time as any subsequent calls are operating on address proxy = address(0) returned from calling create2 again with same salt;

Such address(0) can lead to problems in the protocol/project as illustrated in the example Process Flow Above

Tools Used

Manual Analysis

Recommendations

Recommended to check that return address from _deployProxy is not address(0) e.g consider ways like below

function _deployProxy(address organizer, bytes32 contestId, address implementation) internal returns (address) {
        bytes32 salt = _calculateSalt(organizer, contestId, implementation);
        address proxy = address(new Proxy{salt: salt}(implementation));
        if (proxy == address(0)) revert ProxyFactory__NoZeroAddress(); // add zero address checks 
        return proxy;
    }

Deterministic Address Generation flaw in getProxyAddress Function

Severity

High Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L220-L229

Summary

A vulnerability has been identified in the method getProxyAddress where an address generation mechanism doesn't check if the derived address is already in use.

Vulnerability Details

The function getProxyAddress generates an Ethereum address based on the provided salt and implementation. This is done using the create2 opcode, which allows for deterministic address generation. While this can be a powerful feature, it also presents risks if not used carefully.

The issue arises due to the absence of a check to ensure the generated address isn't already occupied by a contract. If someone, knowingly or unknowingly, deploys a contract to an address that would in the future be produced by this function, any sponsors who send tokens to the supposed "new" proxy contract address would be sending tokens to the wrong place.

POC:
Manual Address Calculation: A malicious actor or an innocent third party calculates an address using the same methodology as the getProxyAddress function.

Preemptive Deployment: The actor deploys a contract or simply sends tokens to the calculated address.

Legitimate User Interaction: A legitimate user uses the getProxyAddress function later on with the same salt and implementation combination. They are provided with the previously calculated address.

Token Transfer: Assuming the address from getProxyAddress is a fresh proxy, sponsors might send tokens to it. Those tokens would then end up in the wrong place, potentially leading to loss of funds or assets being locked without recovery options.

Impact

Any tokens or funds sent to the generated proxy address, assuming it's a fresh contract, could potentially be lost or end up in an unexpected contract.

Tools Used

Manuel reviews

Recommendations

Implement a check in the getProxyAddress function to verify if the generated Ethereum address is already occupied. It is advised not to send funds before deployment as they could be lost

Lack of Access Control for Token Whitelisting

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol

Description:

In the constructor of the ProxyFactory contract, whitelisted tokens are set without any access control checks. This allows any user to modify the whitelisted tokens, potentially introducing malicious tokens.

Impact:

Permitting unrestricted modification of whitelisted tokens could lead to the inclusion of malicious tokens, potentially undermining the contract's intended functionality.

Proof of Concept:

1 . Deploy the ProxyFactory contract.
2 . Use the constructor or accessible functions to add a malicious token to the whitelist.

The contract IS altered due to the addition of the malicious token.

Recommendation:

Implement access control mechanisms (e.g., onlyOwner modifier) to restrict the modification of whitelisted tokens to authorized users only, ensuring the integrity of the whitelist.

Hey all! Just getting started with the audit and couldn't figure out what versions of open-zep is the right one for this project.

I'd recommend specifying the version of all the libfiles in the readme, or checking the lib file into the repo. Don't think this is a vuln but could lead to problems if multiple devs are building with different versions of the libs.

Owner can renounceOwnership

Severity

Medium Risk

Relevant GitHub Links

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9e3f4d60c581010c4a3979480e07cc7752f124cc/contracts/access/Ownable.sol#L73

Summary

Owner can renounceOwnership

Vulnerability Details

ProxyFactory.sol relies on critical and key onlyOwner functions which require an owner at all times. Ownable.sol has a renounceOwner(...) function that can be called maliciously or accidentally by owner leaving the main contract without an owner

Impact

ProxyFactory is the main entry contract for the projects. Without the owner functionality the project can not function to set, deploy, create, pay, and other functionalities of proxies, implementation, distribution, contests, etc

Tools Used

Manual Analysis

Recommendations

It is recommended to overwrite renounceOwnership(..) function in the contracts to ensure that it cant be called e.g to revert etc

Protocol uses assembly not commented

Severity

Low Risk

Relevant GitHub Links

Summary

The protocol makes use of assembly in Proxy.sol

Vulnerability Details

Although this is not an attacker vector, it may introduce problems in the protocol as not all Solidity developers or event auditors understand assembly. Weirdly I have seen this as Low and QA in some reports.

Impact

Consider the following cases

• Developer takes over project maintenance tries to update this code to suit what they have seen elsewhere by seeing patterns and not necessarily understanding the code and so introduce errors
• Auditor not checking if this code is robust and therefore missing errors in Proxy.sol

Tools Used

Manual Analysis

Recommendations

Recommended the code be commented to explain every part of the assembly, why it is preferred, make use of Natspec

Risk of Duplicating Execution in deployProxyAndDistributeBySignature Function

Severity

Medium Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L152-L167

Summary

The deployProxyAndDistributeBySignature function in the contract is susceptible to replay attacks due to the absence of a nonce or unique identifier in the signed message. This vulnerability could allow an attacker to reuse a valid signature to execute the function multiple times, even if the context has changed.

Vulnerability Details

The deployProxyAndDistributeBySignature function uses an ECDSA signature to authenticate the caller's address. However, it lacks protection against replay attacks because it does not include a nonce or unique identifier in the signed message. The following code snippet illustrates this issue:

function deployProxyAndDistributeBySignature(
    address organizer,
    bytes32 contestId,
    address implementation,
    bytes calldata signature,
    bytes calldata data
) public returns (address) {
    bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(contestId, data)));
    if (ECDSA.recover(digest, signature) != organizer) revert ProxyFactory__InvalidSignature();
    bytes32 salt = _calculateSalt(organizer, contestId, implementation);
    if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
    if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
    // ...
}

The function generates a digest from the provided parameters and compares the recovered address with the organizer's address to validate the signature. However, due to the absence of a nonce or unique identifier, an attacker could capture a valid signature for a transaction and replay it later, potentially causing unintended consequences.

Impact

Exploiting this vulnerability could lead to an attacker repeatedly executing the deployProxyAndDistributeBySignature function, even if the original context or conditions have changed. This could result in multiple proxy deployments and prize distributions for a single contest, leading to misallocation of resources and disruptions to the intended contract behavior.

Tools Used

Manual

Recommendations

Include a nonce or unique identifier in the message that is signed. This ensures that each signature corresponds to a specific transaction and prevents replaying the same signature in different contexts.

Lack of contract existence check on delegatecall will result in unexpected behavior

Severity

Medium Risk

Summary

The contract's use of the delegatecall operation in the fallback function can lead to unintended behavior and unauthorized access due to a lack of proper context separation between the proxy and implementation contracts.

Vulnerability Details

The delegatecall operation within the fallback function can allow the implementation contract to manipulate and access the proxy contract's storage variables and state. While the intention is to delegate calls to the implementation contract, the nature of delegatecall makes it execute the code of the implementation contract as if it were running within the proxy contract's context. This can result in unintended interactions, unauthorized access, and potential security vulnerabilities.
Consider a scenario where an attacker crafts a malicious implementation contract with the intent of exploiting this behavior:

// Malicious implementation contract
contract MaliciousImplementation {
    address private owner;

    constructor() {
        owner = msg.sender;
    }

    // Malicious function to transfer ownership to the attacker
    function takeOwnership(address _newOwner) external {
        require(msg.sender == owner, "Unauthorized");
        owner = _newOwner;
    }
}

In this example, the malicious implementation contract sets up an owner during its construction. The takeOwnership function is designed to allow ownership transfer, but the authorization check is flawed, relying on msg.sender, which is not isolated between the proxy and implementation contracts due to the use of delegatecall.

An attacker could craft a transaction to call the takeOwnership function through the proxy. The attacker would set the _newOwner parameter to their own address, and because the msg.sender context is from the proxy, the attacker would gain unauthorized ownership of the malicious implementation contract.

Impact

An attacker could gain unauthorized control over crucial contract functionality, manipulate state variables, and potentially drain funds or disrupt contract behavior.

Tools Used

Manual

Recommendations

Use a more secure proxy pattern that provides proper context isolation between the proxy and implementation contracts. Also,
implement a contract existence check before each delegatecall.
Carefully review the Solidity documentation, especially the “Warnings” section,
and the pitfalls of using the delegatecall proxy pattern.

Project lacks emergency controls

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol

Summary

Project team explained that they started with less decentralized protocol like mix web2 and web3, centralized ownership that creates contests etc and hopefully will decentralize in future

Vulnerability Details

If the purpose of such guarded approach is to ensure safety of protocol it may as well implement some emergency features in case something happens e.g DOS, funds lost, funds stealing, injection of wrong winners etc. The project can implement Emergency control patterns such as Pause functionality to pause the creation contests, launch of proxies, distribution funds etc to protect its reputation, users, funds and functionality. It is already centralized and with good communication and onboarding users it can explain purpose of Pausability that it will be removed with time, sort of like guard rails

Impact

Without Pause functionality it puts the project at risk of not being able to intervene in the case of problems

Tools Used

Manual Analysis

Recommendations

It is recommended project implement one or more Emergency Patterns such as Pausability e.g inherit from OpenZepplin Pausable contracts and apply whenNotPaused modifier to critical functions

Miner manipulation block.timestamp

Severity

Medium Risk

Relevant GitHub Links

Summary

Miners can manipulate block.timestamp to disadvantage protocol/project usage

Vulnerability Details

Miners can manipulate block.timestamp as this is under their control within a certain leeway of past timestamps. For example last block.timestamp can be within say 13-15 seconds of thee next so miner can choose a timestamp that makes it so that resulting logic is different from if it was one value block.timestamp vs another but differ by seconds

Consider the following example when it comes to the various deploy and distribute functions e.g

function deployProxyAndDistribute(bytes32 contestId, address implementation, bytes calldata data) {
....
if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed(); //reverts

}

If block.timestamp < closeTime as set in saltToCloseTime[salt] the deployment reverts. If owner or organizer had anticipated that earliest they would distribute was after a given time e.g 1000 seconds as timestamp say closeTime == 1000 so saltToCloseTime[salt] = 1000. Miner can make block.timestamp expected to be 1000 seconds as 999(allowed as they have leeway to set this and difference 1 second is allowable) so that 999< 1000 and function reverts. This whereas owner or organizer had informed stakeholders distribution occurring

Impact

This can led to potential reverts in many functions relying on block.timestamp as in the example above. It can also harm reputation if expectation and communication distributions happening at particular time but is manipulated to occur in as next block by miner as opposed to expected block

Additionally miners can ignore transaction calls for functions relying on

if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();

whenever transaction is finally mined block.timestamp will be greater than saltToCloseTime[salt] so valid at a later stage. This is akin to a DOS like attack by miners to prevent distributions from happening earlier so they happen way later than expected

Tools Used

Manual Analysis

Recommendations

It is recommended to understand these issues so as to know that there is a block.timestamp window that may affect transactions and call the relevant functions only when sure block.timestamp will be greater than saltToCloseTime[salt] as opposed to the boundary edge points.

It may be ideal to work with block.number which cant be manipulated by miners, and use blocks as estimates of times however this has challenge as it may result in faulty expectations times as yo may work with each block 13 seconds in your timeline calculations when in fact other blocks may be 17 seconds etc

Incorrect Handling of Delegatecall Result in Proxy Contract's Fallback Function

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Proxy.sol#L51

Summary

In the fallback function of the Proxy contract, there is an assembly block that is responsible for delegating the call to the implementation contract. However, there is an issue with the way the result of the delegate call is being handled. Specifically, the contract is currently using a switch statement to handle the delegatecall result, and while it handles the success case, it incorrectly handles the failure case.

Vulnerability Details

The issue lies in the following section of the code:

switch result
case 0 { revert(ptr, size) }
default { return(ptr, size) }
}

In the context of a delegatecall, a result of 0 indicates a failure, and a non-zero result indicates success. In the code above, the case 0 block correctly reverts the transaction on failure, but the default block is used to return the data on success. This is incorrect because the value of result can be any non-zero value on success, not just 1.

Impact

The incorrect handling of the delegate call result could lead to unexpected behavior and vulnerabilities in the smart contract system. If a delegate call fails, the intended behavior is to revert the transaction, but due to the flawed code, the transaction may not be reverted as expected. This could potentially lead to unauthorized or unintended execution of functions and manipulation of the contract's state.

PoC

1 . Deploy the Implementation contract:

// Implementation contract
pragma solidity ^0.8.0;

contract Implementation {
    function doSomething() external {
        require(false, "Intentional require failure");
    }
}

2 . Deploy the Implementation contract.
3 . Deploy the Proxy contract, providing the address of the deployed Implementation contract as the implementation address.
Call the doSomething function on the proxy contract.

In this scenario, the doSomething function in the Implementation contract contains a require statement that will always fail. When the Proxy contract delegates the call to the Implementation contract's doSomething function, the delegate call will fail due to the require statement.

• Expected Behavior :
  The transaction should revert, indicating a failure due to the requirement in the doSomething function of the Implementation contract.

• Actual Behavior :
  Due to the bug in the proxy contract's fallback function, the transaction might not revert as expected, and unintended behavior or state changes may occur. The delegate call's failure is not properly handled, potentially leading to vulnerabilities.

Tools Used

Manual Review

Recommendations

we should explicitly check for a non-zero value in the result variable to determine if the delegate call was successful. Here's the corrected code:

fallback() external {
    address implementation = _implementation;
    assembly {
        let ptr := mload(0x40)
        calldatacopy(ptr, 0, calldatasize())
        let result := delegatecall(gas(), implementation, ptr, calldatasize(), 0, 0)
        let size := returndatasize()
        returndatacopy(ptr, 0, size)

        if eq(result, 0) {
            revert(ptr, size)
        }
        return(ptr, size)
    }
}

Gas Limit for Loops

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol

Description:

In the constructor of the ProxyFactory contract, if _whitelistedTokens is extensive, the loop processing it could consume excessive gas, potentially exceeding the block's gas limit.

Impact:

If the gas limit is surpassed, the constructor will fail to execute, impeding the deployment of the contract.

Proof of Concept:

• Deploy the ProxyFactory contract.
• Use an extremely large _whitelistedTokens array as input to the constructor.
• Attempt to deploy the contract, and the deployment fail due to surpassing the gas limit.

Recommendation:

For scenarios where _whitelistedTokens could be large, consider breaking down the array processing into smaller chunks or optimize the code to mitigate gas consumption and prevent exceeding the gas limit.

Centralization Risk for trusted organizers

Severity

Medium Risk

Summary

According to the provided link on "Centralization Risk for trusted owners", I believe that the Organizer also carries a centralization risk. As described in the documentation, "The sponsor Sponsor is the person providing financial support. Sponsors can be anyone, including the organizer Organizer. This implies that Organizer = Sponsor", which could potentially lead to unexpected situations.

Vulnerability Details & Impact

1. Anyone can become an organizer, including the sponsor. This gives the organizer excessive power since one person can hold multiple roles, which could lead to malicious behavior, such as distributing rewards to acquaintances or oneself, prematurely ending the competition after obtaining a solution, or in the case of sponsor = organizer, running away with the funds after obtaining a solution.
2. If supporters do not anonymize their submissions, it could result in covert operations.
3. Even though there is the possibility of off-chain identity verification for organizers, I still see a significant level of susceptibility to manipulation within this protocol.

Tools Used

• Manual Review

Recommendations

• In my understanding, Sparkn is similar to the Immunefi auditing platform.
• My suggestion is to differentiate the roles of organizer and sponsor. Similar to the @CodeHawks platform, anonymize the solutions submitted by each supporter. The organizer can be any auditing platform (such as @code4rena, @sherlock, @CodeHawks), while the sponsor should only be the project itself, such as "sparkn" or "Beedle - Oracle free perpetual lending," and should not simultaneously hold the role of organizer.

ProxyFactory: no ability to add tokens to whitelist after deployment

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L81-L90

Summary

If the protocol wants to allow new tokens to be distributed after deployment of the protocol they cannot.

Vulnerability Details

Cannot whitelist new tokens.

Impact

Tools Used

Recommendations

Add whitelist token setters.

Test coverage not sufficient

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/tree/main/test

https://github.com/Cyfrin/2023-08-sparkn/blob/main/test/uint/OnlyDistributorTest.t.sol

Summary

Test coverage must be improved

Vulnerability Details

Testing is critical to catching bugs, vulnerabilities, and ensuring project works as assumed

Impact

Example is leaving out the test cases of arrays with zero addresses leads to bugs as indicated in report
Tests are crucial to catching bugs
Consider this test OnlyDistributor // any calls from non-factory address will fail, so tests end here it tests calls from NON_FACTORY but uses zero values in input
``
vm.startPrank(organizer);
...
distribute(address(0), new address, new uint256, "");

Test expects revert so reader, auditor, dev, maintainer will to be sure if reverts were really caused by calling test with account not FACTORY of the zero values without running test to see errors. Sadly some maintainers, auditors, developers maintaining project only read through tests. 

Tests need to be robust and focus on testing specific item being tested without leaving doubts 

## Tools Used
Manual Analysis 

## Recommendations
Recommended to test more functionality, edge cases, corner cases, various scopes, aspects and depths of all functions, flows, interrelations to ensure robust tests that secure the code 

Potential Distribution Failure Due to Individual Transfer Interruptions in the _distribute Function

Severity

Medium Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L105-L117

Summary

If any of the recipient addresses (winners) fails to accept the tokens (due to reasons such as being blacklisted), the entire distribution transaction will revert, preventing all winners from receiving their tokens.

Vulnerability Details

The _distribute function is designed to transfer tokens to a list of winners based on specified percentages. Each transfer operation is dependent on the previous one. If a single transfer fails for any reason, including an address being blacklisted or having no available balance, the entire transaction will be reverted, and no tokens will be distributed to any of the addresses in the list.

Impact

Unable to distribute rewards to winners

Tools Used

Manuel reviews

Recommendations

Individual Error Reporting: Implement specific error messages for each address in case of a transfer failure. This provides clarity on which address caused the transaction failure.

Such as

try erc20.safeTransfer(winners[i], amount) {
} catch {
    revert string(abi.encodePacked("Distributor__FailedTransferTo: ", winners[i]));
}

So that the distributor can remove the address from the winners array and re initiate the distribution process.

Protocol wont work well with tokens that can prevent transfers

Severity

Medium Risk

Relevant GitHub Links

Summary

Protocol although it specifies it whitelists tokens there is no indication that it will prefer tokens that cant be blocked by owners, admins, or central parties for the token. There are various tokens that have capability to stop normal functioning of token

Vulnerability Details

Although tokens will be whitelisted e.g preference major coins as stated in @NatSpec constructor "e.g. USDC, JPYCv1, JPYCv2, USDT, DAI" ProxyFactory.sol line 77 and 78 it seems preference is stablecoins and major coins without specific policies on what type of features e.g risks these coins must have. There could still arise problems with one or few coins or stablecoins or coins that have been whitelisted. Consider one of many example cases below.

There are various tokens and token standards that can result in transfers being stopped, blocked, blacklisted, paused or disallowed. This entails protocols may function well with these tokens up until a time when any of above measures activated e.g token is paused, accounts are blacklisted etc leading to inability to perform transfers into and out of the protocol

Cases in point include below

• USDT can blacklist addresses, so if the protocol contracts are blacklisted they cant transfer in funds, transfer out commissions or make payments to winners etc. If winners are blacklisted they cant receive their payments
• Other token standards such as Tokens such as ERC20Pausable, Pausable Tokens like WBTC, ERC1400, Polymath like tokens; it implies the all instances mentioned in the links provided will not function for transfers.

Impact

Medium - this renders protocol incapable of being used especially given the whitelisted tokens cant be updated. If a single whitelisted token e.g USDT blacklists the protocol, its contract addresses, winners, commission addresses or STADIUM address etc transfers in and out of that token become impossible

Tools Used

Manual Analysis

Recommendations

It is recommended token whitelisting set clear policies to avoid tokens with block, pausable, blacklisting, stop, limiting, transfer denying, overpowered control even if they are stablecoins.

Concern of Recursive Calls in _distribute Function

Severity

Medium Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L249-L253

Summary

The _distribute function within the ProxyFactory contract lacks proper protection against recursive invocation risk, which could potentially lead to unexpected behavior and fund loss.

Vulnerability Details

The _distribute function, responsible for distributing prizes through proxy contracts, is susceptible to recursive attacks due to the absence of a proper reentrancy guard. The function currently uses the call method to invoke the implementation contract's logic using the provided data argument. However, it does not prevent the function from being re-entered before it completes its execution, creating a window for malicious actors to manipulate the flow of execution and exploit the contract.

function _distribute(address proxy, bytes calldata data) internal {
    (bool success,) = proxy.call(data);
    if (!success) revert ProxyFactory__DelegateCallFailed();
    emit Distributed(proxy, data);
}

Impact

An attacker could exploit reentrancy to repeatedly call the _distribute function and alter the state of the contract in unexpected ways. This could lead to incorrect distribution of prizes, unauthorized access to funds, and overall instability of the contract.

Tools Used

Manual

Recommendations

Use the "Checks-Effects-Interactions" pattern.

bool private locked;

function _distribute(address proxy, bytes calldata data) internal {
    require(!locked, "Reentrancy guard");
    locked = true;
    
    (bool success,) = proxy.call(data);
    if (!success) revert ProxyFactory__DelegateCallFailed();
    
    locked = false;
    emit Distributed(proxy, data);
}
```
By introducing the locked boolean variable, the function checks if it's currently executing. If it's not locked (i.e., not currently executing), it proceeds with the distribution logic, sets the locked flag to prevent reentrancy, and then resets the flag after the execution is complete. This safeguard helps to prevent multiple invocations of the _distribute function from overlapping and mitigates the risk of reentrancy attacks.

Potential DOS due to Gas Exhaustion Due to Large Array Iteration in _distribute Function

Severity

Medium Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/Distributor.sol#L144-L152

Summary

The _distribute function in the provided contract contains a loop that iterates through arrays of winners and percentages to distribute tokens. If these arrays are very large, this loop could lead to excessive gas consumption, potentially causing transactions to run out of gas and fail.

Vulnerability Details

The _distribute function is responsible for distributing tokens to winners based on their percentages. This function iterates through arrays of winners and percentages, calculating the amount to transfer to each winner based on their percentage. While the function's purpose is to fairly distribute tokens, a potential vulnerability arises when dealing with a large number of winners and percentages.

function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
    internal
{
    // ...

    uint256 winnersLength = winners.length;
    for (uint256 i; i < winnersLength;) {
        uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
        erc20.safeTransfer(winners[i], amount);
        unchecked {
            ++i;
        }
    }

    // ...
}

The loop's gas cost increases linearly with the size of the winners and percentages arrays. If these arrays contain a significant number of elements, the gas consumption of the transaction could exceed the gas limit, causing the transaction to fail due to out-of-gas.

Impact

The impact of this issue is that transactions attempting to distribute tokens to a large number of winners in a single execution may fail due to running out of gas. Users may experience frustration and inconvenience if their intended distributions cannot be completed successfully.

Tools Used

Manual

Recommendations

Implement a batching mechanism that processes a limited number of winners and percentages in each iteration of the loop.

Deployed proxies not disposed after usage

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/README.md

Summary

Deployed contest contracts not explicitly disposed off after usage

Vulnerability Details

Project README states the following
Proxy contracts are supposed to be disposed after the contest is over.
However after contest usage and deploys to distribute; the proxy contracts at their deployed addresses are still in existence without any restrictions that ProxyFactory.sol functions cant still call them; additionally funds can still be sent to the addresses

It is possible to call the various deploy...() functions in ProxyFactory.sol many times as they will not revert since redoing create2 will result in zero address return for Proxy which succeeds in low level calls that don't check for zero address

 function _deployProxy(address organizer, bytes32 contestId, address implementation) internal returns (address) {
        bytes32 salt = _calculateSalt(organizer, contestId, implementation);
        address proxy = address(new Proxy{salt: salt}(implementation));
        return proxy;
    }
// when above function is called many times will not revert but after first time with same parameters and salt will return address(0) for proxy 
 function _distribute(address proxy, bytes calldata data) internal {
        (bool success,) = proxy.call(data);
        if (!success) revert ProxyFactory__DelegateCallFailed();
        emit Distributed(proxy, data);
    }
// function above will succeed since address(0).call(data) will return success when doing low level call to zero address 

Impact

1. This is not consistent with the interpretation of documentation
2. Sponsors may continue to send funds to these contest contracts thinking still active
3. Other users or random people may continue to send funds these contract by error or deliberately implying admin/owner needs to rescue these funds using gas
4. Way after these contracts not being used; ProxyFactory.sol Owner may be able to use rescue funds functionality to redeem tokens sent by error to these addresses redeeming to own address for own benefit

Tools Used

Manual Analysis

Recommendations

It may be recommended to favor explicitness over implicitness and make it clear in code that the proxies after life cycle are disposed and not fit for use by using any combination of the following options

1. Ensure address proxy != address(0) in the _deploy() function or in many of the deploy functions in the code

2. Safe way to selfdestruct them(given dangers of delegatecall) so using proper access control and other security mechanisms to destroy the contracts with selfdestruct safely so that there truly is no contract code there after usage

3. When contest lifecycle finishes have a mapping changed to false of all addresses that have deployed and distributed so these can be queried e.g

mapping(address => bool) ended; 
function deploy....(...) {
   ...existing logic;
   _distribute(proxy, data);
   ended[proxy] = true; // update various deploy functions to add this 
   return proxy
}

function isEnded(address _proxy) .. {
     return ended[_proxy]; //so stakeholders before interacting with contest can query if it is disposed 
}

4. Or alternatively ensure the various deployAnd... functions in ProxyFactory.sol can only be called once using some sort of a flag

Zero-Address Checks absent in Constructor

Severity

Medium Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/main/src/Proxy.sol#L44

Summary

Missing Zero-Address Check of constructor of L44 of Proxy.sol contract could cause unnecessary errors and imprecision of code execution

Vulnerability Details

Missing zero-address checks for constructor of L44 in the Proxy.sol contract may lead to unwanted errors and accuracy of code execution, which may affect the functions linked to it. For example, assuming the constructor passes in a zero address, all function calls to the Proxy contract will fail because it will delegate the call to a zero-address contract.

Impact

Accidental use of zero-addresses could result in contract malfunction.

Tools Used

Manual review

Recommendations

• reference material：Missing zero-address checks in Curve.transferOwnership and Router.constructor

• E.g.

  error Proxy__NoZeroAddress();
  constructor(address implementation) {
      if(implementation == address(0)) revert Proxy__NoZeroAddress();
      _implementation = implementation;
  }

Potential Integer Overflow

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol

Description:

In the constructor of the ProxyFactory contract, the unchecked keyword is used in a loop to increment the loop variable. This could potentially lead to an integer overflow scenario if the _whitelistedTokens array is close to its maximum length.

Impact:

If the _whitelistedTokens array length is large, using unchecked for the loop increment might cause unexpected behavior due to integer overflow, leading to unintended loop termination.

Proof of Concept:

• Deploy the ProxyFactory contract.
• Create an extremely large _whitelistedTokens array or replicate the scenario in a test contract.
• Execute the loop in the constructor using the unchecked keyword.

The loop behave unexpectedly due to integer overflow.

Recommendation:

Remove the unchecked keyword and use a standard loop structure with proper bounds checking to avoid potential integer overflow issues.

Duplicated code e.g use modifiers

Severity

Low Risk

Relevant GitHub Links

Summary

There is duplication of code in various functions in ProxyFactory.sol

Vulnerability Details

There are various code parts reused in functions in ProxyFactory.sol that can be put in a modifier

if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
// above used in deployProxyAndDistribute(), deployProxyAndDistributeBySignature() and deployProxyAndDistributeByOwner, and 
// distributeByOwner
if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
// above used in deployProxyAndDistribute(), deployProxyAndDistributeBySignature(), 

Above are example snippets reused in functions but can be turned into modifiers and applied to the functions

Impact

above reuse can introduce errors, makes code harder to read and reason, impacts maintainability, readability, cleanliness of code. It is best to ensure reused code parts that enforce certain checks can be in modifier

Tools Used

Manual Analysis

Recommendations

Recommended to create modifiers and apply to functions e.g

modifier onlyRegisteredContest() {
    if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
    _;
}
// apply to appropriate functions e.g 
function deployProxyAndDistribute(...) onlyRegisteredContest() ..... {....} //etc 

Storage Layout Clash between Proxy and Distributor Contracts

Severity

High Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/Proxy.sol#L38-L46

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/Distributor.sol#L56-L62

Summary

Storage layout clash has been identified between the Proxy and Distributor contracts.

Vulnerability Details

The Proxy contract, built using the delegate proxy pattern, holds an _implementation state variable that resides in its first storage slot (slot 0). Conversely, the Distributor contract's initial storage slot (also slot 0) is shared by a numeric variable 1 and the FACTORY_ADDRESS. This overlap means that when the Distributor's functions try to access FACTORY_ADDRESS, they could inadvertently modify or read the _implementation variable of the Proxy instead.

POC

Deploying Contracts: A user deploys the Proxy contract, setting its _implementation to point towards a pre-deployed Distributor contract. The intention is that the Proxy will delegate calls to the Distributor.

Reading _implementation Directly: When querying the _implementation address from the Proxy contract, the user accurately sees the Distributor contract's address.

Delegate Call to Distributor: A user calls a function on the Proxy that is designed to interact with the FACTORY_ADDRESS in the Distributor using a delegate call.

Unexpected Behavior: Due to the storage layout conflict, rather than accessing FACTORY_ADDRESS, the function reads the value in the Proxy's storage slot 0, which is _implementation. As a result, the user unexpectedly receives the Distributor's address.

The storage of the Proxy contract is what gets "overlapped" or unintentionally accessed. This is because the delegate call makes the Distributor contract read and write to the Proxy contract's storage.

The clash occurs because the Distributor contract's code expects its first state variable (FACTORY_ADDRESS) to be in storage slot 0, but when accessed via a delegate call from the Proxy, slot 0 contains the Proxy's _implementation address.

Impact

Any attempts to call the distribute function in Distributor.sol will revert because msg.sender != FACTORY_ADDRESS

Tools Used

Manuel Reviews

Recommendations

To prevent such issues, it's essential to ensure that the storage layouts of proxy and implementation contracts are synchronized and do not clash.

There are many ways to reserve special storage slots for specific variables so that you can be sure they wont overlap.

Missing Events

Severity

Low Risk

Relevant GitHub Links

Summary

There are some critical functionalities that are missing events

Vulnerability Details

1. Distributor.sol line 163 function _commissionTransfer(IERC20 token) should emit its own event, especially given that commissions transfers occur after distribution to winners. Its a critical function that can report to offchain tooling the commissions going to STADIUM at every moment

2. ProxyFactory.sol all deploy and distribute functions in the contract emit Distributed event. This is not truly reflective of happenings as they are different and events should reflect differences. The functions should emit specific events related to them e.g

• deployProxyAndDistributeByOwner //should differentiate that organizer not around so owner called this by having event with owner and organizer details emitted
• distributeByOwner // should differentiate that organizer called it , important information like above
• deployProxyAndDistribute
  are all functions with different intricacies and dynamics that can be captured by adding additional event or updating Distributed event to capture these differences

Impact

This shortchanges various offchain tooling, monitoring, reporting, frontend services that may rely on events to adequately capture real time activities of the contracts. It may even be critical for security monitoring so project can respond adequately if events sufficiently detailed and informative. Any emissions suspicious can allow protocol to react quickly

Tools Used

Manual Analysis

Recommendations

Recommended to add events for the cases detailed above e.g
DistributeByOrganizer, DistributedSignature, DistributedOwner or keep Distributed and add other specific events in function e.g BySignatureEvent in function deployProxyAndDistributeBySignature() etc

Missing Error Message in Revert Statements

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol

Description:

In the constructor of the ProxyFactory contract, the revert statements are missing accompanying error messages. This omission makes it challenging to diagnose the cause of a transaction failure when interacting with the contract.

Impact:

When transactions fail due to issues related to zero addresses or empty arrays, users and developers won't receive meaningful feedback about what went wrong. This can result in confusion and hinder efficient debugging.

Proof of Concept:

• Deploy the ProxyFactory contract.
• Call the constructor with an empty _whitelistedTokens array.
• The transaction will revert, but the error message will provide no information about the underlying problem.

Recommendation:

Add explicit error messages to the revert statements in the constructor to provide context on the specific issue. For instance:

if (_whitelistedTokens[i] == address(0)) revert ProxyFactory__NoZeroAddress();

This will improve the user experience by providing clearer error messages.

Missing indexing event values

Severity

Low Risk

Relevant GitHub Links

Summary

Some events are lacking indexed parameters

Vulnerability Details

Distributed.sol line 58 event Ditributed(...) event missing indexed values

Impact

This short changes offchain tooling for reporting, monitoring, front end and even security that could use the searchable indexes of events to filter events for specific values e.g address XXX being in every winning pool of contests etc as suspicious

Tools Used

Manual Analysis

Recommendations

Recommended to index identify event values e.g

event Distributed(address indexed token, address[] indexed winners, uint256[] percentages, bytes indexed data);

ProxyFactory: no input validation on proxy param

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L205-L218

Summary

In distributeByOwner, there is no input validation on proxy param.

Vulnerability Details

    function distributeByOwner(
        address proxy,
        address organizer,
        bytes32 contestId,
        address implementation,
        bytes calldata data
    ) public onlyOwner {
        if (proxy == address(0)) revert ProxyFactory__ProxyAddressCannotBeZero();
        bytes32 salt = _calculateSalt(organizer, contestId, implementation);

This allows for owner to pass in any proxy address and execute arbitrary call on it. Even bypass expiration checks and call distribute on not expired contests. But there is no way to deploy it before expiry, so not that big of a problem.

Impact

Mistakenly/intentionally distribute on incorrect proxy.

Tools Used

Recommendations

Verify that the proxy address passed is correct. Or do not take the proxy as input at all. Compute the address from organizer and contestId with _calculateSalt and getProxyAddress.

Inconsistent Behavior of ERC-20 Token Transfer Functions

Severity

Medium Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/Distributor.sol#L116-L151

Summary

The issue stems from the historical variability in the behavior of transfer functions among different ERC-20 tokens. Early tokens were developed before the finalization of the ERC-20 standard, leading to discrepancies in whether transfer functions should revert or return a boolean value. This inconsistency in behavior poses challenges when interacting with these tokens and integrating them into smart contracts.

Vulnerability Details

Some tokens, although claiming ERC-20 compatibility, deviate from the current best practice regarding transfer functions. The inconsistency lies in whether a transfer function a) should revert/fail on error and return a boolean value (true on success and false on failure) or b) should only return true or false without reverting on error.

For example, consider a contract similar to the "Distributor" contract provided earlier. In the _distribute function, the contract interacts with the ERC-20 token using the balanceOf function to determine the available balance for distribution. However, if the token being used adheres to the incorrect behavior (returning only true or false on success), the contract might receive incorrect information about the token balance.

function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
    internal
{
    // ...
    IERC20 erc20 = IERC20(token);
    uint256 totalAmount = erc20.balanceOf(address(this)); // Potential vulnerability
    // ...
}

Impact

The inconsistent behavior of ERC-20 token transfer functions can lead to incorrect calculations, potential underestimation of available token balance, and improper distribution of tokens. This can result in financial discrepancies, errors in distribution percentages, and unexpected behavior in smart contract execution.

Tools Used

Manual

Recommendations

Before interacting with any ERC-20 token, verify that the token's transfer functions adhere to the expected behavior of reverting/failing on error and returning true on success.

1. Define an interface for ERC-20 tokens that includes the standard transfer function signature:

interface IStandardERC20 {
    function transfer(address to, uint256 amount) external returns (bool);
}

2. Modify your _isWhiteListed function to include a check for token compatibility:

function _isWhiteListed(address token) internal view returns (bool) {
    IStandardERC20 erc20 = IStandardERC20(token);
    try erc20.transfer(address(0), 0) returns (bool success) {
        return true; // Token is compatible with expected behavior
    } catch {
        return false; // Token does not conform to expected behavior
    }
}

3. Before using the token's balance in your _distribute function, validate its compatibility:

function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
    internal
{
    // ...
    if (!_isWhiteListed(token)) {
        revert Distributor__InvalidTokenAddress();
    }
    IERC20 erc20 = IERC20(token);
    uint256 totalAmount = erc20.balanceOf(address(this)); // Safe to use now
    // ...
}

Potential issue on Distributor Contract

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol

Summary

This bug report highlights several potential issues and areas of improvement in the Distributor contract. The contract is intended to distribute ERC20 tokens to winners based on specified percentages, and it plays a crucial role in the token distribution process. The identified issues range from input validation to gas optimization and potential arithmetic overflow.

Vulnerability Details

Below are the specific issues found in the contract:

1 . Missing Constructor Input Checks:

• In the constructor, the contract does not use require statements to enforce the conditions that factoryAddress and stadiumAddress cannot be zero addresses.
• Without these checks, the contract could be deployed with invalid addresses, leading to unexpected behavior during token distribution.

2 . Inefficient Loop Unrolling:

• The _distribute function contains unnecessary loop unrolling using the increment operator (++i).
• Loop unrolling here provides no optimization benefits and adds unnecessary complexity to the code.

3 . Potential Overflow:

• The calculation totalAmount * percentages[i] / BASIS_POINTS in the _distribute function could lead to arithmetic overflow if totalAmount is large and percentages[i] is close to BASIS_POINTS.

4 . Lack of Validation for Percentages:

• The _distribute function does not validate whether individual percentage values in the percentages array are within the valid range (0 to 10000).

5 . Lack of Error Handling for Token Transfer:

• The _distribute function uses safeTransfer for token transfers but lacks error handling for transfer failures.
  This could lead to failed transfers going unnoticed, leaving winners without their rewards.

6 . Potential Gas Optimization:

• The _commissionTransfer function transfers the entire remaining token balance to the STADIUM_ADDRESS.
  This might consume excessive gas and result in inefficient token transfers.

Impact

The potential impact of these issues could range from incorrect token distribution to smart contract vulnerabilities that could be exploited by malicious actors. These issues might lead to unexpected behavior, incorrect distributions, loss of funds, or contract failures.

Tools Used

Manual Review

Recommendations

• Implement proper input validation checks in the constructor to prevent deployment with zero addresses.
• Remove unnecessary loop unrolling in the _distribute function for better readability.
• Implement overflow checks when performing arithmetic operations involving totalAmount and percentages.
• Validate percentage values in the percentages array to ensure they are within the valid range.
• Implement error handling for token transfers using safeTransfer to handle transfer failures gracefully.
• Optimize gas usage in the _commissionTransfer function by transferring only the required commission fee.

Ownable2Step vs Ownable ownership change risks

Severity

Medium Risk

Relevant GitHub Links

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9e3f4d60c581010c4a3979480e07cc7752f124cc/contracts/access/Ownable.sol#L92

Summary

ProxyFactory.sol makes use of Ownable whereas Ownable2Step is the safer option

Vulnerability Details

Ownable.sol has a transferOwnership function that occurs in a single step. An address that is changed to new owner may be account that has lost control of keys, is now under control malicious user, incorrect address, not part of trusted entities etc

Impact

Medium - Such single step can lead to ownership of contracts being lost and ProxyFactory being main entry point of project implies onlyOwner functions will no longer work or be trusted. Ownable2Step on the other hand ensures any change is ownership is first claimed by the new owner to ensure they are still in control of keys.

Tools Used

Manual Analysis

Recommendations

It is recommended to make use of inheriting from OpenZeppelin Ownable2Step contract so that any transfers or change of ownerships are safe to addresses that are capable of resuming ownership roles.

Arrays not checked for duplicates

Severity

Medium Risk

Relevant GitHub Links

Summary

Arrays not checked for duplicates

Vulnerability Details

There are functions that take arguments/parameters/values with array values of addresses and do not check if these addresses are duplicates before using them in function logic. Consider the following examples

ProxyFactory.sol line 81, address[] memory _whitelistedTokens does not check if could be whitelisting the same token

Distributor.sol line 92, address[] memory winners does not check if a winner has been duplicated

Impact

For the whitelisting tokens case, owner could whitelist by mistake [USDT,USDT]when intention was to whitelist [USDT, USDC] with communication to stakeholders that tokens for funding and payments are USDC and USDT which will not be the case to due to duplicate error; This results in any whitelist checks for token failing for e.g USDC in example above
if (!_isWhiteListed(token)) {revert Distributor__InvalidTokenAddress();} hence protocol not working as expected

For the case of winners, this results in a winner potentially being paid twice where in case another was missed they are not paid at all e.g intention was array winners [OxAA, 0xAB] but entries are [0xAA, 0xAA] means 0xAA paid twice whereas 0xAB not paid

Tools Used

Manual Analysis

Recommendations

It is recommended that in all cases where arrays must not have duplicates that values are checked if they have been seen before using them in function logic to avoid errors and problems explained earlier. Example could be a mapping that checks existence e.g or some other ideal duplicate checking ways

mapping(uint => bool) public exists;
function doSomethingWithArray(address _adds,...) {..
     for(unit i=0, i < _adds.length; i++) {
        require(!exists[_adds[i]],"...");
        ...doSomething(...)
        exists[_adds[i]] = true;
}

Arrays not checked for zero values

Severity

Medium Risk

Relevant GitHub Links

Summary

Arrays do not check if values are zero or zero addresses

Vulnerability Details

There is functionality that makes use of array arguments but does not check sanity check if values in the array may be zero values e.g uint256(0) or address(0) which leads to the functionality breaking or not working as intended.

Distributing to winners Distribute.sol line 116 _distribute(..., address[] memory winners, makes use of values in the winners array to make payments without checking if each individual address is address(0) or not

Distributing to winners Distribute.sol line 146 calculates amounts to each winner and if percentage is 0, winner gets 0 payout and this may have been by error because uint256[] memory percentages array was not checked for zero values e.g Intention was to enter [1000,80] but by error enters [1000,00] in winners array since this is done offchain

Impact

Not checking the above addresses before transfers can lead to the following problems depending on the token implementation

• tokens being lost as sent to zero address
• reverts in loops which means all other distributions fails
• if percentages value in array percentages is zero value by error or malicious choices; a winner is not paid

Another impact is whitelisting tokens that does not check if address(0) for tokens submitted in array for constructor in
ProxyFactory.sol line 81 leading to project not working as expected especially given you cant update whitelisted tokens.

Tools Used

Manual Analysis

Recommendations

It is recommended that before using each individual value from the array check that the value is not zero or address(0) may do checks like this

for (uint256 i; i < winnersLength;) {
            require(winners[i] != address(0), ""); 
            require(percentages[i] != address(0), ""); 
            uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
            
            erc20.safeTransfer(winners[i], amount);
            unchecked {
                ++i;
            }
        }

Protocol vulnerable to callback and or hooks tokens

Severity

Medium Risk

Relevant GitHub Links

Summary

Protocol vulnerable to callback and hooks tokens e.g ERC777

Vulnerability Details

The protocol does not have clear policy on types of whitelisted tokens and according to notes @NatSpec constructor "e.g. USDC, JPYCv1, JPYCv2, USDT, DAI" ProxyFactory.sol line 77 and 78 it seems preference is stablecoins and major coins without specific policies on what type fo features e.g risks these coins must have. There are major coins that have problems due to callback and hooks functionality

// Distributor.sol line 145 - 151 
for (uint256 i; i < winnersLength;) {
            uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
            erc20.safeTransfer(winners[i], amount);
            unchecked {
                ++i;
            }
        }

A callback or hook token after being paid to a winner in the above can attack protocol in the following

1. Run a gas intensive computation in its callback consuming all gas or revert such that all the transfers to the winners will fail due to payments in loop

Callback or hooks tokens include ERC777, ERC1363 etc

Impact

Medium Impact - this renders the protocol useless as it cant payout all the winners due it making transfers in a loop and one of the transfers fails since a token with hook or callback causes a revert

Tools Used

Manual Analysis

Recommendations

It is recommended to ensure protocol not base its policies on major coins but token risks and ensure tokens with hooks, callbacks such as ERC777 are not allowed.

Potential for Users to Set closeTime Beyond Intended Maximum in setContest Function

Severity

Medium Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L105-L117

Summary

In the setContest function where the provided closeTime doesn't correctly verify against the allowed maximum duration.

Vulnerability Details

In the setContest function, a check is done to ensure the closeTime is within the allowable time range. However, the condition:

if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp)

is potentially problematic. The condition allows a closeTime that is exactly block.timestamp + MAX_CONTEST_PERIOD, which might not be the intended behavior given the comment annotation indicating it's supposed to be a strict less than 28 days.

With >: Setting closeTime to exactly 28 days from the current time is valid.

POC

Consider a scenario where MAX_CONTEST_PERIOD is set to 28 days.
A user can set the closeTime to block.timestamp + 28 days using the current function without any revert.
However, if the strict inequality as indicated by the comment is enforced, this would not be possible, and the function would revert.

Impact

The user can set the closeTime to 28 days from the transaction firing.

Tools Used

Manual review

Recommendations

Instead of

if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) {

do

if (closeTime >= block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp)

Because

With >: Setting closeTime to exactly 28 days from the current time is valid.
With >=: Setting closeTime to exactly 28 days from the current time is invalid.

Lack minimum contest time period

Severity

Medium Risk

Relevant GitHub Links

Summary

There is no minimum value set for contest only maximum value is set

Vulnerability Details

This implies as block.timestamp is an increasing function with time, immediately after a contest is set the functions
deployProxyAndDistribute(); deployProxyAndDistributeBySignature(); can be called if closeTime entered in setContest() was a value just slightly above block.timestamp used in setContest()

Impact

It may be reasonable to have a minimum period before which deploy and distributions cant be called by owner or organizer. It makes the system more easier to reason about and more reflective given that it has a Maximum Contest Period a Minimum should flow logically

Say for example contest is set and there are two sponsors. SponsorA deposits funds into contest, but sponsorB delays deposits. Owner or organizers when winners finalized immediately call deployProxyAndDistribute() or deployProxyAndDistributeBySignature() to distribute to winners andsoon after delaying sponsorB deposits their amount, this will only distribute SponsorA funds whereas owner or organizer may be thinking sponsorB should have also deposited by now if they didn't check balances. Therefore a minimum period can allow for the above or any other aspects like time to change winners etc

Tools Used

Manual Analysis

Recommendations

It is recommended to set a contest minimum period e.g

uint256 public constant MIN_CONTEST_PERIOD = 2 days;
mapping(bytes32 => uint256) public saltToSetTime; //time when contest was set 

// set time in setContest() function
saltToCloseTime[salt] = closeTime;
saltToSetTime[salt] = block.timestamp;

// usage within relevant functions apply below check 
 if ( saltToSetTime[salt]  < 2 days  + block.timestamp ) revert ProxyFactory__ContestIsNotOpen();

Incorrect Error Handling for Mapping Access

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L249

Description:

The _distribute function in the ProxyFactory contract uses a delegate call to execute logic on a proxy contract. However, the success of a delegate call is not accurately checked, leading to potential misunderstandings about the outcome of the delegate call.

Impact:

This incorrect error handling could lead to developers and users wrongly assuming the success or failure of the delegate call, which may cause unintended consequences when interacting with the contract.

Proof of Concept:

• Deploy the ProxyFactory contract.
• Deploy a proxy contract that reverts during a delegate call.
• Call the _distribute function on the ProxyFactory contract with the proxy contract address.
  The delegate call will fail, but the contract may not capture the failure due to the incorrect success check.

Recommendation:

Use the abi.decode function to interpret the return data from the delegate call and properly handle success or failure. Since the delegate call will revert in case of an exception, the direct success check isn't necessary.

Protocol is brittle as fixed expiry and close time limits

Severity

Medium Risk

Relevant GitHub Links

Summary

Every contest will have the same EXPIRATION_TIME = 7 days and MAX_CONTEST_PERIOD = 28 days

Vulnerability Details

Contests are not going to come in one size fits all so finalizing their logistics, winners may need different timelines
However the values are fixed and apply for all contests/created innovations/projects without differentiation

Impact

The above makes the protocol inflexible and brittle as these values once ProxyFactory is deployed cant be changed. It may be that with learning via experience it may be ideal to have shorter or longer MAX_CONTEST_PERIOD etc
This one size fits all may even not be ideal as it may disincentivise sponsors who wanted a shorter or longer timelines for their specific project they want to fund etc

Tools Used

Manual Analysis

Recommendations

It is recommended either introduce setter functions that can change above values

Ideally each contest must have its own values so these values of closeTime, expirations and timelines can be parameters for each individual contest e.g in setContest(,,,uin256 _maxContestTime, uint256 _expirationTime) so that you set uniquely for each contest maybe withing certain acceptable bounds

Arrays not checking for order if two or more used in together

Severity

Medium Risk

Relevant GitHub Links

Summary

Array values used together are not checked for ordering correctness

Vulnerability Details

Distributor.sol line 116 _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data) has two arrays winners, and percentages that are used in tandem implying ordering of elements in the array is very important as each element is used alongside the other element in same index position in other array.

It is critical that these arrays are ordered correctly. E.g [0xAlice, 0xBob] = winners and [6000, 3000]=percentages may be an error where in fact ordering was supposed to be [0xAlice, 0xBob] = winners and [3000, 6000]=percentages. These percentages determine who is paid what?

Impact

Medium impact as it disadvantages the users in that if not ordered correctly can result in winners being paid incorrectly as some are paid lower than they expected due to mismatch in ordering of the arrays.

Tools Used

Manual Analysis

Recommendations

It is recommended instead of using multiple arrays make use of struct that has values of the winners and their percentage so that each struct has information for its winner e.g

struct Winner { address winner, uint256 percentage }
....function _distribute(..., Winner[] memory winners,...) // takes an array of Structs of winners

The above avoids problem of mismatch ordering arrays winners and array percentages, that can result in making incorrect payouts to winners

Report

All Issues

Gas Optimizations

[GAS-1] Using bools for storage incurs overhead

Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past. See source.

Instances (1):

File: src/ProxyFactory.sol

71:     mapping(address => bool) public whitelistedTokens;

[GAS-2] Cache array length outside of loop

If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).

Instances (1):

File: src/ProxyFactory.sol

83:         for (uint256 i; i < _whitelistedTokens.length;) {

[GAS-3] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

Instances (1):

File: src/ProxyFactory.sol

87:                 i++;

[GAS-4] Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

Instances (2):

File: src/ProxyFactory.sol

64:     uint256 public constant EXPIRATION_TIME = 7 days;

65:     uint256 public constant MAX_CONTEST_PERIOD = 28 days;

Non Critical Issues

[NC-1] Typos

Instances (60):

File: src/Distributor.sol

- 3: // Layout of Contract:
+ 3: // Layout of Contract: version, imports, errors, interfaces, libraries, contracts, type declarations, state variables, events, modifiers, functions

- 4: // version
+ 4: // Layout of Functions: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private, view & pure functions

- 5: // imports
+ 5: //////////////////////

- 6: // errors
+ 6: /////// Error ////////

- 7: // interfaces, libraries, contracts
+ 7: //////////////////////

- 8: // Type declarations
+ 8: //////////////////////////////////////

- 9: // State variables
+ 9: /////// Immutable Variables //////////

- 10: // Events
+ 10: //////////////////////////////////////

- 11: // Modifiers
+ 11: uint8 private constant VERSION = 1; // version is 1 for now

- 12: // Functions
+ 12: uint256 private constant COMMISSION_FEE = 500; // this can be changed in the future

- 14: // Layout of Functions:
+ 14: // a constant value of 10,000 (basis points) = 100%

- 15: // constructor
+ 15: // prize distribution event. data is for logging purpose

- 16: // receive function (if exists)
+ 16: ////////////////////////////

- 17: // fallback function (if exists)
+ 17: /////// Constructor ////////

- 18: // external
+ 18: ////////////////////////////

- 19: // public
+ 19: /// @dev initiate the contract with factory address and other key addresses, fee rate

- 20: // internal
+ 20:     // uint256 version, // for future use

- 21: // private
+ 21:     FACTORY_ADDRESS = factoryAddress; // initialize with deployed factory address beforehand

- 22: // view & pure functions
+ 22:     STADIUM_ADDRESS = stadiumAddress; // official address to receive commission fee

- 40:     //////////////////////
+ 40: ////////////////////////////////////////////

- 41:     /////// Error ////////
+ 41: /////// External & Public functions ////////

- 42:     //////////////////////
+ 42: ////////////////////////////////////////////

- 52:     //////////////////////////////////////
+ 52: ////////////////////////////////////////////

- 53:     /////// Immutable Variables //////////
+ 53: /////// Internal & Private functions ///////

- 54:     //////////////////////////////////////
+ 54: ////////////////////////////////////////////

- 56:     uint8 private constant VERSION = 1; // version is 1 for now
+ 56:     // token address input check

- 59:     uint256 private constant COMMISSION_FEE = 500; // this can be changed in the future
+ 59:     // winners and percentages input check

- 60:     // a constant value of 10,000 (basis points) = 100%
+ 60:     // check if totalPercentage is correct

- 63:     // prize distribution event. data is for logging purpose
+ 63:     // if there is no token to distribute, then revert

- 66:     ////////////////////////////
+ 66:     uint256 winnersLength = winners.length; // cache length

- 67:     /////// Constructor ////////
+ 67:     // send commission fee as well as all the remaining tokens to STADIUM_ADDRESS to avoid dust remaining

- 68:     ////////////////////////////
+ 68: ///////////////////////////////////////////

- 69:     /// @dev initiate the contract with factory address and other key addresses, fee rate
+ 69: /////// Getter pure/view functions ////////

- 71:         // uint256 version, // for future use
+ 71: ///////////////////////////////////////////

78:         FACTORY_ADDRESS = factoryAddress; // initialize with deployed factory address beforehand

79:         STADIUM_ADDRESS = stadiumAddress; // official address to receive commission fee

82:     ////////////////////////////////////////////

83:     /////// External & Public functions ////////

84:     ////////////////////////////////////////////

101:     ////////////////////////////////////////////

102:     /////// Internal & Private functions ///////

103:     ////////////////////////////////////////////

119:         // token address input check

124:         // winners and percentages input check

134:         // check if totalPercentage is correct

141:         // if there is no token to distribute, then revert

144:         uint256 winnersLength = winners.length; // cache length

153:         // send commission fee as well as all the remaining tokens to STADIUM_ADDRESS to avoid dust remaining

176:     ///////////////////////////////////////////

177:     /////// Getter pure/view functions ////////

178:     ///////////////////////////////////////////

File: src/Proxy.sol

- 3: // Layout of Contract:
+ 3: // Layout of a Contract:

- 14: // Layout of Functions:
+ 14: // Layout of a Function:

File: src/ProxyFactory.sol

- 68:     /// @dev The contest doesn't exist when value is 0
+ 68:     /// @dev The contest doesn't exist when the value is 0

- 70:     /// @dev record whitelisted tokens
+ 70:     /// @dev Record whitelisted tokens

- 133:         // can set close time to current time and end it immediately if organizer wish
+ 133:         // Can set close time to current time and end it immediately if organizer wish

- 215:         // distribute only when it exists and expired
+ 215:         // Distribute only when it exists and expired

- 221:     /// @dev Calculate the proxy address using salt and implementation address
+ 221:     /// @dev Calculate the proxy address using the salt and implementation address

- 246:     /// @dev the data passed in should be the calling data of the distributing logic
+ 246:     /// @dev The data passed in should be the calling data of the distributing logic

- 255:     /// @dev Calculate salt using contest organizer address and contestId, implementation address
+ 255:     /// @dev Calculate salt using contest organizer address and contestId, and implementation address

Low Issues

[L-1] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead

Instances (1):

File: src/ProxyFactory.sol

227:         bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, keccak256(code)));

Medium Issues

[M-1] Centralization Risk for trusted owners

Impact:

Contracts have owners with privileged rights to perform admin tasks and need to be trusted to not perform malicious updates or drain funds.

Instances (4):

File: src/ProxyFactory.sol

37: contract ProxyFactory is Ownable, EIP712 {

81:     constructor(address[] memory _whitelistedTokens) EIP712("ProxyFactory", "1") Ownable() {

184:     ) public onlyOwner returns (address) {

211:     ) public onlyOwner {

Premature Contest Closure via setContest Function

Severity

Medium Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L105-L117

Summary

The setContest function in the ProxyFactory contract has the potential to allow the owner to inadvertently close contests prematurely by providing a closeTime in the past. This can lead to contests being considered closed before they even start.

Vulnerability Details

The setContest function is designed to allow the contract owner to set properties for a new contest. One of the properties that can be set is the closeTime, which specifies when the contest is scheduled to close. However, the function includes a condition that checks whether the provided closeTime is within a valid range:

if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) {
    revert ProxyFactory__CloseTimeNotInRange();
}

The condition includes two parts: one that checks if the closeTime is greater than the current block's timestamp plus a predefined maximum contest period (MAX_CONTEST_PERIOD), and another that checks if the closeTime is less than the current block's timestamp.

If the condition closeTime < block.timestamp evaluates to true, the function reverts with the ProxyFactory__CloseTimeNotInRange error. This effectively prevents contests from being registered with a closeTime in the past.

Impact

The impact of this issue is that the contract owner could mistakenly input a closeTime for a contest that has already passed. As a result, the contest will be considered closed prematurely, even though it hasn't started yet. This can lead to confusion, inaccurate contest registration, and a disruption of the intended contest schedule.

Tools Used

Manual

Recommendations

Check that the provided closeTime is greater than the current block's timestamp to ensure that contests are registered for future dates.

Use latest solidity version

Severity

Low Risk

Relevant GitHub Links

Summary

Protocol makes uses of Solidity Version 0.8.18 whereas latest version is at 0.8.21

Vulnerability Details

Protocol is using a version of solidity that is not the latest version

Impact

Older versions like this may have may have bugs not fixed in latest compiler versions that have updated for security fixes and these bugs may impact the protocol or be used as attack vectors to the protocol

Tools Used

Manual Analysis

Recommendations

Recommended to make use of the latest Solidity version e.g 0.8.21

Missing caller information in events

Severity

Low Risk

Relevant GitHub Links

Summary

There are some critical functionalities emitting events without caller information especially onlyOwner functions

Vulnerability Details

1. ProxyFactory.sol line 116 event is emitted when contest are set by owner. However the event does not emit the owner information

2. ProxyFactory.sol all line 252 event Distributed that is called in _distribute(..) which is further called in many organizer or onlyOnwer functions e.g distributeByOwner() does not emit the caller of the function which should be owner as expected.

Impact

This short changes various offchain tooling, monitoring, reporting, front end services that may rely on events to adequately capture real time activities of the contracts. It may even be critical for security monitoring so project can respond adequately if events show unexpected owner value for these calls leading to likelihood ownership has been compromised from trusted party

Tools Used

Manual Analysis

Recommendations

Recommended to ensure events emit the caller e.g add address owner to events in ProxyFactory.sol

event SetContest(
        address owner, address indexed organizer, bytes32 indexed contestId, uint256 closeTime, address indexed implementation
    );
    event Distributed(address owner, address indexed proxy, bytes data);

"Immutable COMMISSION_FEE Contradicts Comment Indicating Mutability"

Severity

Medium Risk

Relevant GitHub Links

https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/Distributor.sol#L59

Summary

Inconsistency between in the code comments and the actual implementation.

Vulnerability Details

Misleading Code Comment on Mutability of COMMISSION_FEE.
The COMMISSION_FEE variable is declared as a private constant with a value of 500. However, the accompanying code comment indicates that it can be changed in the future.

So there is no way to change this commission fee and it is marked as constant, until a deployment this value won't bulge.

Impact

While the code's functionality isn't directly impacted, developers or auditors who rely on comments for understanding might be misled. This can lead to incorrect assumptions about the contract's behavior, especially regarding future modifications or upgrades.

Tools Used

Manuel reviews

Recommendations

Delete this comment or add a function to change the commission fee.

Transfers in loops

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/47c22b818818af4ea7388118dd83fa308ad67b83/src/Distributor.sol#L116C13-L116C115

Summary

Project loops over array of winners in order to pay them

Vulnerability Details

Distributor.sol line 145 makes transfers of token winnings in a loop. This has several problems that

• it adds gas overhead for the protocol
• it may lead to Out of Gas, if the array of winners is too large, meaning no one can be paid
• if one of the payments fail in the loop e.g blacklisted address for blacklisted token or some other reason that may cause a single of the erc20.safeTransfer(winners[i], amount); such DOS leads to to fail all the other transfers fail due to atomicity of blockchain meaning that just one winner revert implies all other winners cant be paid

Impact

Tools Used

Manual Analysis

Recommendations

It is recommended to make use of Pull Over Push Withdrawal pattern for payments to prevent especially DOS failure or attacks
Pull Method implies saving winnings in some way e.g mapping and winners can withdraw their payments themselves
By using pull method it reduces chances winners don't get paid due to loop transfers that can lead to DOS, reverts, over use of gas etc

ProxyFactory: owner can distribute however they want after expiry

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L169-L218

Summary

If the ProxyFactory owner is compromised OR the owner wants to act maliciously, they can steal assets from expired(7 days from contest close time) contests however they'd like.

Vulnerability Details

There is no onchain verification of organizer's consent on how the distribution should be in deployProxyAndDistributeByOwner and distributeByOwner calls. If the organizer fails/misses to distribute the contest assets within 7 days a malicious admin can distribute it however they'd like. Or if the admin account is compromised, the attacker can steal assets from all expired but not distributed contests or steal mis-sent assets.

Impact

Tools Used

Recommendations

Add a signature param and verify signature from the organizer before executing the distribution transaction to verify the organizer's intent/consent.

Vunerable to Reentrancy

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#249

Description:

The _distribute function in the ProxyFactory contract executes a delegate call to a proxy contract. However, the contract lacks protection against reentrancy attacks, potentially allowing malicious actors to exploit vulnerabilities.

Impact:

In the absence of reentrancy protection, malicious parties could repeatedly call the _distribute function and perform unauthorized actions, potentially compromising the contract's integrity.

Proof of Concept:

• Deploy the ProxyFactory contract.
• Deploy a proxy contract with logic that includes an external call.
• Use the deployProxyAndDistribute function to trigger the proxy contract's logic that makes an external call.

Malicious reentrant behavior is possible due to the absence of reentrancy protection.

Recommendation:

Implement reentrancy protection mechanisms such as the nonReentrant modifier or follow established patterns like the

Protocol has no way to update whitelisted tokens

Severity

Medium Risk

Relevant GitHub Links

Summary

Protocol only sets the whitelisted tokens only once and there is no updates capability
There is no way to add or remove tokens from whitelist

Vulnerability Details

The protocol owner only whitelists tokens at the deployments of the ProxyFactory.sol and there is no capability to update these tokens. Although tokens will be whitelisted e.g preference major coins as stated in @NatSpec constructor "e.g. USDC, JPYCv1, JPYCv2, USDT, DAI" ProxyFactory.sol line 77 and 78 it seems preference is stablecoins and major coins without specific policies on what type fo features e.g risks these coins must have. There could still arise problems with one or few coins or stablecoins or coins that have been whitelisted. Consider one of many example cases below.

• Owner could consider USDT best coin and deploy with only this token. However Tether as centralized coin can be blocked or stopped due to reserve issues, depeg, legal or regulatory issues implying this projects only whitelisted token is not capable of being used in the protocol
• Owner could deploy project with only [USDT, DAI, USDC] but majority sponsors prefer JPYCv1, JPYCv2 so no one will send funds to the Proxies to support the projects rendering the protocol/project useless

Impact

High Impact - this renders the protocol useless as it can not be funded for contests/innovations or pay out the various supporters/winners.

Tools Used

Manual Analysis

Recommendations

It is recommended to ensure protocol makes use of as many good coins as possible for whitelist to mitigate risks
It is recommended to ensure coins are chosen not because they are major coins as stated in their notes but because they have low risks that ensure continuity of protocol and project.
It is recommended to add access controlled owner only functionality to ProxyFactory.sol to add and remove tokens e.g

function addRemoveWhitelistToken(address _token, bool _addRemove) external onlyOwner  {
     whitelistedTokens[_token] = _addRemove;
}

Use struct to return lots of values

Severity

Low Risk

Relevant GitHub Links

Summary

Function that returns lots of values better off returning struct

Vulnerability Details

Distributor.sol line 186 ...returns (address _FACTORY_ADDRESS, address _STADIUM_ADDRESS, uint256 _COMMISSION_FEE, uint8 _VERSION)

Impact

Cleaner code that is easier to maintain and less likely to introduce errors

Tools Used

Manual Analysis

Recommendations

Can return struct e.g

struct CONSTANTS {address _FACTORY_ADDRESS; address _STADIUM_ADDRESS; uint256 _COMMISSION_FEE; uint8 _VERSION}
....
...returns (CONSTANTS memory)