LOW RISK

[L‑01] Signature use at deadlines should be allowed

According to EIP-2612, signatures used on exactly the deadline timestamp are supposed to be allowed. While the signature may or may not be used for the exact EIP-2612 use case (transfer approvals), for consistency's sake, all deadlines should follow this semantic. If the timestamp is an expiration rather than a deadline, consider whether it makes more sense to include the expiration timestamp as a valid timestamp, as is done for deadlines.

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

[L‑02] Unsafe downcast

When a type is downcast to a smaller type, the higher order bits are truncated, effectively applying a modulo to the original value. Without any other checks, this wrapping will lead to unexpected behavior and bugs

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

[L‑03] Missing checks for address(0x0) in the constructor

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

[L-04] Array lengths not checked

If the length of the arrays are not required to be of the same length, user operations may not be fully executed due to a mismatch in the number of items iterated over, versus the number of items provided in the second array

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

[L-05] Using zero as a parameter

Taking 0 as a valid argument in Solidity without checks can lead to severe security issues. A historical example is the infamous 0x0 address bug where numerous tokens were lost. This happens because '0' can be interpreted as an uninitialized address, leading to transfers to the '0x0' address, effectively burning tokens. Moreover, 0 as a denominator in division operations would cause a runtime exception. It's also often indicative of a logical error in the caller's code. It's important to always validate input and handle edge cases like 0 appropriately. Use require() statements to enforce conditions and provide clear error messages to facilitate debugging and safer code.

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

[L‑06] Functions calling contracts/addresses with transfer hooks are missing reentrancy guards

Even if the function follows the best practice of check-effects-interaction, not using a reentrancy guard when there may be transfer hooks will open the users of this protocol up to read-only reentrancies with no way to protect against it, except by block-listing the whole protocol.

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

[L‑07] 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.

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

[L‑08] External call recipient may consume all transaction gas

There is no limit specified on the amount of gas used, so the recipient can use up all of the transaction's gas,

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

[L‑09] Owner can renounce while system is paused

The contract owner or single user with a role is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely

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

[L‑10] Some tokens may revert when zero value transfers are made

In spite of the fact that EIP-20 states that zero-valued transfers must be accepted, some tokens, such as LEND will revert if this is attempted, which may cause transactions that involve other tokens (such as batch operations) to fully revert. Consider skipping the transfer if the amount is zero, which will also save gas.

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