cyfrin / 2023-08-sparkn Goto Github PK
View Code? Open in Web Editor NEWLicense: Other
License: Other
0
address and uniqueness checks for winners
may result in unexpected situations.Medium Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol#L116-L156
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.
From a developer's perspective, there are two scenarios for the competition:
winner
.winners
.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.
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.
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.
winner
to ensure that rewards are not repeatedly distributed to the same winner
.0
address check on the winner
to ensure that rewards are not distributed to a 0
address.Medium Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L241C14-L241C14
2023-08-sparkn/src/ProxyFactory.sol
Line 250 in 0f139b2
Returned proxies can be zero addresses
Additionally low level calls are not checked for contract existence or zero address
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)
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
Manual Analysis
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;
}
getProxyAddress
FunctionHigh Risk
https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L220-L229
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.
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.
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.
Manuel reviews
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
Low Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol
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.
Permitting unrestricted modification of whitelisted tokens could lead to the inclusion of malicious tokens, potentially undermining the contract's intended functionality.
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.
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.
Medium Risk
2023-08-sparkn/src/ProxyFactory.sol
Line 37 in 47c22b8
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9e3f4d60c581010c4a3979480e07cc7752f124cc/contracts/access/Ownable.sol#L73
Owner can renounceOwnership
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
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
Manual Analysis
It is recommended to overwrite renounceOwnership(..) function in the contracts to ensure that it cant be called e.g to revert etc
Low Risk
Line 53 in 0f139b2
The protocol makes use of assembly in Proxy.sol
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.
Consider the following cases
Manual Analysis
Recommended the code be commented to explain every part of the assembly, why it is preferred, make use of Natspec
deployProxyAndDistributeBySignature
FunctionMedium Risk
https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L152-L167
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.
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.
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.
Manual
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.
Medium Risk
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.
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.
An attacker could gain unauthorized control over crucial contract functionality, manipulate state variables, and potentially drain funds or disrupt contract behavior.
Manual
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.
Medium Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol
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
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
Without Pause functionality it puts the project at risk of not being able to intervene in the case of problems
Manual Analysis
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
Medium Risk
2023-08-sparkn/src/ProxyFactory.sol
Line 134 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 163 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 187 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 216 in 0f139b2
Miners can manipulate block.timestamp to disadvantage protocol/project usage
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
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
Manual Analysis
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
Medium Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Proxy.sol#L51
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.
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.
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.
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.
Manual Review
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)
}
}
Medium Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol
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.
If the gas limit is surpassed, the constructor will fail to execute, impeding the deployment of the contract.
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.
Medium Risk
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.
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.supporters
do not anonymize their submissions, it could result in covert operations.organizers
, I still see a significant level of susceptibility to manipulation within this protocol.Low Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L81-L90
If the protocol wants to allow new tokens to be distributed after deployment of the protocol they cannot.
Cannot whitelist new tokens.
Add whitelist token setters.
Low Risk
https://github.com/Cyfrin/2023-08-sparkn/tree/main/test
https://github.com/Cyfrin/2023-08-sparkn/blob/main/test/uint/OnlyDistributorTest.t.sol
Test coverage must be improved
Testing is critical to catching bugs, vulnerabilities, and ensuring project works as assumed
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
Medium Risk
https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L105-L117
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.
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.
Unable to distribute rewards to winners
Manuel reviews
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.
Medium Risk
2023-08-sparkn/src/ProxyFactory.sol
Line 71 in 47c22b8
2023-08-sparkn/src/Distributor.sol
Line 147 in 47c22b8
2023-08-sparkn/src/Distributor.sol
Line 164 in 47c22b8
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
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
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
Manual Analysis
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.
_distribute
FunctionMedium Risk
https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L249-L253
The _distribute
function within the ProxyFactory
contract lacks proper protection against recursive invocation risk, which could potentially lead to unexpected behavior and fund loss.
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);
}
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.
Manual
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.
_distribute
FunctionMedium Risk
https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/Distributor.sol#L144-L152
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.
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.
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.
Manual
Implement a batching mechanism that processes a limited number of winners and percentages in each iteration of the loop.
Low Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/README.md
Deployed contest contracts not explicitly disposed off after usage
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
Manual Analysis
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
Ensure address proxy != address(0) in the _deploy() function or in many of the deploy functions in the code
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
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
}
Medium Risk
https://github.com/codefox-inc/sparkn-contracts/blob/main/src/Proxy.sol#L44
Missing Zero-Address Check of constructor
of L44
of Proxy.sol
contract could cause unnecessary errors and imprecision of code execution
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.
Accidental use of zero-addresses could result in contract malfunction.
Manual review
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;
}
Medium Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol
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.
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.
The loop behave unexpectedly due to integer overflow.
Remove the unchecked keyword and use a standard loop structure with proper bounds checking to avoid potential integer overflow issues.
Low Risk
2023-08-sparkn/src/ProxyFactory.sol
Line 132 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 162 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 186 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 214 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 134 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 163 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 187 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 216 in 0f139b2
There is duplication of code in various functions in ProxyFactory.sol
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
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
Manual Analysis
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
High Risk
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
Storage layout clash has been identified between the Proxy and Distributor contracts.
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.
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.
Any attempts to call the distribute
function in Distributor.sol will revert because msg.sender != FACTORY_ADDRESS
Manuel Reviews
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.
Low Risk
2023-08-sparkn/src/Distributor.sol
Line 163 in 47c22b8
2023-08-sparkn/src/ProxyFactory.sol
Line 58 in 47c22b8
There are some critical functionalities that are missing events
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
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
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
Manual Analysis
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
Low Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol
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.
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.
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.
Low Risk
2023-08-sparkn/src/Distributor.sol
Line 64 in 47c22b8
Some events are lacking indexed parameters
Distributed.sol line 58 event Ditributed(...) event missing indexed values
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
Manual Analysis
Recommended to index identify event values e.g
event Distributed(address indexed token, address[] indexed winners, uint256[] percentages, bytes indexed data);
Medium Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L205-L218
In distributeByOwner
, there is no input validation on proxy param.
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.
Mistakenly/intentionally distribute on incorrect proxy.
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
.
Medium Risk
https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/Distributor.sol#L116-L151
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.
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
// ...
}
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.
Manual
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.
interface IStandardERC20 {
function transfer(address to, uint256 amount) external returns (bool);
}
_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
}
}
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
// ...
}
Medium Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol
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.
Below are the specific issues found in the contract:
1 . Missing Constructor Input Checks:
2 . Inefficient Loop Unrolling:
3 . Potential Overflow:
4 . Lack of Validation for Percentages:
5 . Lack of Error Handling for Token Transfer:
6 . Potential Gas Optimization:
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.
Manual Review
Medium Risk
2023-08-sparkn/src/ProxyFactory.sol
Line 37 in 47c22b8
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9e3f4d60c581010c4a3979480e07cc7752f124cc/contracts/access/Ownable.sol#L92
ProxyFactory.sol makes use of Ownable whereas Ownable2Step is the safer option
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
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.
Manual Analysis
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.
Medium Risk
2023-08-sparkn/src/Distributor.sol
Line 116 in 47c22b8
2023-08-sparkn/src/ProxyFactory.sol
Line 81 in 47c22b8
Arrays not checked for duplicates
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
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
Manual Analysis
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;
}
Medium Risk
2023-08-sparkn/src/Distributor.sol
Line 116 in 47c22b8
2023-08-sparkn/src/Distributor.sol
Line 147 in 47c22b8
2023-08-sparkn/src/ProxyFactory.sol
Line 81 in 47c22b8
2023-08-sparkn/src/Distributor.sol
Line 116 in 0f139b2
Arrays do not check if values are zero or zero addresses
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
Not checking the above addresses before transfers can lead to the following problems depending on the token implementation
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.
Manual Analysis
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;
}
}
Medium Risk
2023-08-sparkn/src/Distributor.sol
Line 147 in 0f139b2
Protocol vulnerable to callback and hooks tokens e.g ERC777
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
Callback or hooks tokens include ERC777, ERC1363 etc
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
Manual Analysis
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.
Medium Risk
https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L105-L117
In the setContest
function where the provided closeTime doesn't correctly verify against the allowed maximum duration.
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.
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.
The user can set the closeTime
to 28 days from the transaction firing.
Manual review
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.
Medium Risk
2023-08-sparkn/src/ProxyFactory.sol
Line 65 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 65 in 0f139b2
There is no minimum value set for contest only maximum value is set
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()
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
Manual Analysis
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();
Low Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L249
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.
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.
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.
Medium Risk
2023-08-sparkn/src/ProxyFactory.sol
Line 64 in 0f139b2
2023-08-sparkn/src/ProxyFactory.sol
Line 65 in 0f139b2
Every contest will have the same EXPIRATION_TIME = 7 days and MAX_CONTEST_PERIOD = 28 days
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
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
Manual Analysis
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
Medium Risk
2023-08-sparkn/src/Distributor.sol
Line 116 in 47c22b8
2023-08-sparkn/src/Distributor.sol
Line 146 in 47c22b8
Array values used together are not checked for ordering correctness
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?
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.
Manual Analysis
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
Issue | Instances | |
---|---|---|
M-1 | Centralization Risk for trusted owners | 4 |
Issue | Instances | |
---|---|---|
L-1 | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() |
1 |
Issue | Instances | |
---|---|---|
NC-1 | Typos | 60 |
Issue | Instances | |
---|---|---|
GAS-1 | Using bools for storage incurs overhead | 1 |
GAS-2 | Cache array length outside of loop | 1 |
GAS-3 | ++i costs less gas than i++ , especially when it's used in for -loops (--i /i-- too) |
1 |
GAS-4 | Using private rather than public for constants, saves gas |
2 |
Issue | Instances | |
---|---|---|
GAS-1 | Using bools for storage incurs overhead | 1 |
GAS-2 | Cache array length outside of loop | 1 |
GAS-3 | ++i costs less gas than i++ , especially when it's used in for -loops (--i /i-- too) |
1 |
GAS-4 | Using private rather than public for constants, saves gas |
2 |
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;
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;) {
++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++;
private
rather than public
for constants, saves gasIf 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;
Issue | Instances | |
---|---|---|
NC-1 | Typos | 60 |
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
Issue | Instances | |
---|---|---|
L-1 | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() |
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)));
Issue | Instances | |
---|---|---|
M-1 | Centralization Risk for trusted owners | 4 |
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 {
setContest
FunctionMedium Risk
https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/ProxyFactory.sol#L105-L117
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.
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.
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.
Manual
Check that the provided closeTime
is greater than the current block's timestamp to ensure that contests are registered for future dates.
Low Risk
2023-08-sparkn/src/Distributor.sol
Line 24 in 47c22b8
Line 24 in 47c22b8
2023-08-sparkn/src/ProxyFactory.sol
Line 24 in 47c22b8
Protocol makes uses of Solidity Version 0.8.18 whereas latest version is at 0.8.21
Protocol is using a version of solidity that is not the latest version
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
Manual Analysis
Recommended to make use of the latest Solidity version e.g 0.8.21
Low Risk
2023-08-sparkn/src/ProxyFactory.sol
Line 116 in 47c22b8
2023-08-sparkn/src/ProxyFactory.sol
Line 252 in 47c22b8
There are some critical functionalities emitting events without caller information especially onlyOwner functions
ProxyFactory.sol line 116 event is emitted when contest are set by owner. However the event does not emit the owner information
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.
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
Manual Analysis
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);
Medium Risk
https://github.com/codefox-inc/sparkn-contracts/blob/9063a0851ad6538e23728dcb4ba53dc0f722eb96/src/Distributor.sol#L59
Inconsistency between in the code comments and the actual implementation.
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.
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.
Manuel reviews
Delete this comment or add a function to change the commission fee.
Medium Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/47c22b818818af4ea7388118dd83fa308ad67b83/src/Distributor.sol#L116C13-L116C115
2023-08-sparkn/src/Distributor.sol
Line 145 in 47c22b8
Project loops over array of winners in order to pay them
Distributor.sol line 145 makes transfers of token winnings in a loop. This has several problems that
Manual Analysis
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
High Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L169-L218
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.
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.
Add a signature param and verify signature from the organizer before executing the distribution transaction to verify the organizer's intent/consent.
High Risk
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#249
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.
In the absence of reentrancy protection, malicious parties could repeatedly call the _distribute function and perform unauthorized actions, potentially compromising the contract's integrity.
Malicious reentrant behavior is possible due to the absence of reentrancy protection.
Implement reentrancy protection mechanisms such as the nonReentrant modifier or follow established patterns like the
Medium Risk
2023-08-sparkn/src/ProxyFactory.sol
Line 71 in 47c22b8
2023-08-sparkn/src/ProxyFactory.sol
Line 81 in 47c22b8
2023-08-sparkn/src/Distributor.sol
Line 147 in 47c22b8
2023-08-sparkn/src/Distributor.sol
Line 164 in 47c22b8
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
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.
High Impact - this renders the protocol useless as it can not be funded for contests/innovations or pay out the various supporters/winners.
Manual Analysis
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;
}
Low Risk
2023-08-sparkn/src/Distributor.sol
Line 186 in 0f139b2
Function that returns lots of values better off returning struct
Cleaner code that is easier to maintain and less likely to introduce errors
Manual Analysis
Can return struct e.g
struct CONSTANTS {address _FACTORY_ADDRESS; address _STADIUM_ADDRESS; uint256 _COMMISSION_FEE; uint8 _VERSION}
....
...returns (CONSTANTS memory)
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.