Sparkn

CodeFox Inc.

DeFiFoundryProxy

15,000 USDC

View results

Previous Next

Submission Details

Severity: low

Valid

Low QA Report

Rolezn

QA Summary

Low Risk Issues

	Issue	Contexts
LOW‑1	Avoid double casting	2
LOW‑2	Consider implementing two-step procedure for updating protocol addresses	1
LOW‑3	Double type casts create complexity within the code	2
LOW‑4	Large transfers may not work with some `ERC20` tokens	2
LOW‑5	Possible rounding issue	1
LOW‑6	Constructor is missing zero address check	1
LOW‑7	Missing Checks for Address(0x0)	1
LOW‑8	Re-org attack	1
LOW‑9	Use `Ownable2Step` rather than `Ownable`	3
LOW‑10	Use `safeTransferOwnership` instead of `transferOwnership` function	1
LOW‑11	External call recipient may consume all transaction gas	1
LOW‑12	Missing Contract-existence Checks Before Low-level Calls	1

Total: 17 contexts over 12 issues

Low Risk Issues

[LOW‑1] Avoid double casting

Consider refactoring the following code, as double casting may introduce unexpected truncations and/or rounding issues.

Furthermore, double type casting can make the code less readable and harder to maintain, increasing the likelihood of errors and misunderstandings during development and debugging.

Proof Of Concept

File: ProxyFactory.sol

226: bytes memory code = abi.encodePacked(type(Proxy).creationCode, uint256(uint160(implementation)));

228: proxy = address(uint160(uint256(hash)));

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

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

[LOW‑2] Consider implementing two-step procedure for updating protocol addresses

A copy-paste error or a typo may end up bricking protocol functionality, or sending tokens to an address with no known private key. Consider implementing a two-step procedure for updating protocol addresses, where the recipient is set as pending, and must 'accept' the assignment by making an affirmative call. A straight forward way of doing this would be to have the target contracts implement EIP-165, and to have the 'set' functions ensure that the recipient is of the right interface type.

Proof Of Concept

File: ProxyFactory.sol

105: function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation)

public

onlyOwner

{

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

[LOW‑3] Double type casts create complexity within the code

Double type casting should be avoided in Solidity contracts to prevent unintended consequences and ensure accurate data representation. Performing multiple type casts in succession can lead to unexpected truncation, rounding errors, or loss of precision, potentially compromising the contract's functionality and reliability. Furthermore, double type casting can make the code less readable and harder to maintain, increasing the likelihood of errors and misunderstandings during development and debugging. To ensure precise and consistent data handling, developers should use appropriate data types and avoid unnecessary or excessive type casting, promoting a more robust and dependable contract execution.

Proof Of Concept

File: ProxyFactory.sol

226: bytes memory code = abi.encodePacked(type(Proxy).creationCode, uint256(uint160(implementation)));

228: proxy = address(uint160(uint256(hash)));

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

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

[LOW‑4] Large transfers may not work with some `ERC20` tokens

Some IERC20 implementations (e.g UNI, COMP) may fail if the valued transferred is larger than uint96. Source

Proof Of Concept

File: Distributor.sol

147: erc20.safeTransfer(winners[i], amount);

https://github.com/Cyfrin/2023-08-sparkn/tree/main/src/Distributor.sol#L147

File: Distributor.sol

164: token.safeTransfer(STADIUM_ADDRESS, token.balanceOf(address(this)));

https://github.com/Cyfrin/2023-08-sparkn/tree/main/src/Distributor.sol#L164

[LOW‑5] Possible rounding issue

Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator

Proof Of Concept

File: Distributor.sol

146: uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;

https://github.com/Cyfrin/2023-08-sparkn/tree/main/src/Distributor.sol#L146

[LOW‑6] Constructor is missing zero address check

In Solidity, constructors often take address parameters to initialize important components of a contract, such as owner or linked contracts. However, without a check, there's a risk that an address parameter could be mistakenly set to the zero address (0x0). This could occur due to a mistake or oversight during contract deployment. A zero address in a crucial role can cause serious issues, as it cannot perform actions like a normal address, and any funds sent to it are irretrievable. Therefore, it's crucial to include a zero address check in constructors to prevent such potential problems. If a zero address is detected, the constructor should revert the transaction.

Proof Of Concept

File: Proxy.sol

44: constructor: address implementation

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

[LOW‑7] Missing Checks for Address(0x0)

Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.

Proof Of Concept

File: ProxyFactory.sol

249: function _distribute: address proxy

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

Recommended Mitigation Steps

Consider adding explicit zero-address validation on input parameters of address type.

[LOW‑8] Re-org attack

Re-orgs can happen in all EVM chains. For example, in ethereum, where currently Frankencoin is deployed, it is not “super common” but it still happens, being the last one less than a year ago: ethereum-beacon-chain-blockchain-reorg The issue increases the changes of happening if deploying on L2’s/ rollups. where re-orgs have been much more active: polygon-hit-by-157-block-reorg-despite-hard-fork-to-reduce-reorgs being the last one, less than a year ago. Deploy the cloned Contract via create2 with a specific salt that includes msg.sender and address _existing

Proof Of Concept

File: ProxyFactory.sol

241: address proxy = address(new Proxy{salt: salt}(implementation));

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

[LOW‑9] Use `Ownable2Step` rather than `Ownable`

Ownable2Step and Ownable2StepUpgradeable prevent the contract ownership from mistakenly being transferred to an address that cannot handle it (e.g. due to a typo in the address), by requiring that the recipient of the owner permissions actively accept via a contract call of its own.

Proof Of Concept

File: ProxyFactory.sol

26: import {Ownable} from "openzeppelin/access/Ownable.sol";

37: contract ProxyFactory is Ownable, EIP712 {

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

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

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

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

[LOW‑10] Use `safeTransferOwnership` instead of `transferOwnership` function

Use safeTransferOwnership which is safer. Use it as it is more secure due to 2-stage ownership transfer.
The Ownable.sol only uses transferOwnership rather than safeTransferOwnership