All contracts contain a floating pragma. It is recommended to deploy all contracts with a single, specific compiler version to reduce the risk of compiler-specific bugs and contracts deployed with different versions.
In LPFarming.sol, the claim() and claimAll() functions do not follow a proper checks-effects-interactions pattern. There is no vulnerability in these functions due to the nonReentrant modifier and apparent lack of control flow transfer with the JPEG token, however, it is a simple security upgrade to move the userRewards[msg.sender] = 0 line above jpeg.safeTransfer(msg.sender, rewards);
Issue #3 (Low) - Comment vs code conflict for newEpoch
As discussed in my previously submitted Medium finding called "Owner of LPFarming.sol can DOS rewards by stopping epoch at any time", there are two discrepancies between the comments of the newEpoch() function and the code implementation.
Comment says "@notice Allows the owner to start a new epoch. Can only be called when there's no ongoing epoch". There are no checks in the code whether there is an ongoing epoch or not.
Comment says "@param _startBlock The new epoch's start block. Has to be greater than the previous epoch's endBlock". There are no checks that the new epoch's start block is greater than the original epoch's end block.
Issue #6 (Low) - Call to .decimals() which may not be supported for all tokens
Depending on the function used to deposit in yVault.sol, a call to token.decimals() may return errors since decimals() is not and ERC20 standard function.
Issue #7 (Low) - Comment vs Code conflict for setContractWhitelisted
The setContractWhitelisted() function explains that calling this function allows the owner to whitelist contracts. This is slightly untrue since the function does not include any logic to determine if the address is a contract or not. Therefore, EOA's can be added to the contract address. The logic to decipher if the account is a contract or EOA is used higher up in the same contract. Adding a check for only contracts will make the comments match the function execution.
Issue #1 (Low) - Floating Pragma
All contracts contain a floating pragma. It is recommended to deploy all contracts with a single, specific compiler version to reduce the risk of compiler-specific bugs and contracts deployed with different versions.
A single example can be found here: https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/tokens/JPEG.sol#L2
Issue #2 (Low) - Lack of CEI Pattern
In
LPFarming.sol
, theclaim()
andclaimAll()
functions do not follow a proper checks-effects-interactions pattern. There is no vulnerability in these functions due to thenonReentrant
modifier and apparent lack of control flow transfer with the JPEG token, however, it is a simple security upgrade to move theuserRewards[msg.sender] = 0
line abovejpeg.safeTransfer(msg.sender, rewards);
Link to code
https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L339-L340
Issue #3 (Low) - Comment vs code conflict for newEpoch
As discussed in my previously submitted Medium finding called "Owner of LPFarming.sol can DOS rewards by stopping epoch at any time", there are two discrepancies between the comments of the
newEpoch()
function and the code implementation.endBlock
". There are no checks that the new epoch's start block is greater than the original epoch's end block.Link to code
https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L103-L104
Issue #4 (Low) - Front-runnable initializer
Lack of access control on initialize(), can be front-run requiring re-deployment.
Link to code
https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L139-L149
Issue #5 (Low) - Input addresses should verify != 0
Several addresses set in the following initialize function do not verify that the input address are not the zero address.
Links to code
https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L170-L176
Issue #6 (Low) - Call to
.decimals()
which may not be supported for all tokensDepending on the function used to deposit in
yVault.sol
, a call totoken.decimals()
may return errors sincedecimals()
is not and ERC20 standard function.https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/yVault.sol#L70-L72
Issue #7 (Low) - Comment vs Code conflict for setContractWhitelisted
The
setContractWhitelisted()
function explains that calling this function allows the owner to whitelist contracts. This is slightly untrue since the function does not include any logic to determine if the address is a contract or not. Therefore, EOA's can be added to the contract address. The logic to decipher if the account is a contract or EOA is used higher up in the same contract. Adding a check for only contracts will make the comments match the function execution.https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L65-L70
Issue #8 (Non-critical) - Insurance typo
Insurace should be spelled insurance.
https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L67
Issue #9 (Non-critical) - Condition order continuity
It would make sense to order the conditions of the following statements in the same fashion to increase code clarity.
address(0) == positionOwner[_nftIndex]
positionOwner[_nftIndex] == address(0)
https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L682-L695