JPEG'd contest - kebabsec's results

1. Change the logic that checks if a certain address is an EOA or whitelisted contract

Line 54 of yVaultLPFarming.sol (https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L54), line 61 of yVault.sol (https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/yVault/yVault.sol#L61) and line 85 of LPFarming.sol (https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L85) have a noContract modifier. This modifier negates openzeppelin's isContract check in an attempt to stop contracts of calling the functions this modifier is used on. But as the comment above says, it leaves a lot of possible ways to bypass the check, as it's also referred in the actual openzeppelin contract to be discouraged to use this modifier as a proper check against contracts as seen here (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7392d8373873bed5da9ac9a97811d709f8c5ffbb/contracts/utils/Address.sol#L15) and it would be unsafe since it doesn't protect against being called from the constructor.

To prevent contracts from calling the functions that this modifier is used on, we suggest that this modifier is changed from the current implementation to

modifier noContract(address _account) {
        require(
            tx.origin == msg.sender || whitelistedContracts[_account],
            "Contracts aren't allowed to farm"
        );
        _;
    }

as it's more exhaustive and prevents calling from contracts, even in the cases that would bypass the current implementation, as well as saving gas due to not calling an external function.

2. Interest might be calculated wrongly in a leap year

In line 593 of NFTVault.sol, (https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/NFTVault.sol#L593) interest per second is calculated by dividing the annual interest rate by the number of seconds in a standard 365 day year. But in the case of leap years, this may lead to slightly more interest being calculated as there are more simply more seconds in a leap year.

In order to mitigate this, consider implementing a leap year check or use the number of seconds in an average year rather than a standard year.

3. Nomenclature regarding owner roles is confusing

Across the contracts we see various different roles that seem to have powers distributed through, but some roles seem to indicate almost the same level of power, some examples are onlyOwner from the Ownable.sol by openzeppelin (https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L67), DAO_ROLE (https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/NFTVault.sol#L205) and DEFAULT_ADMIN_ROLE (https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/FungibleAssetVaultForDAO.sol#L92). Assuming this is by design and they aren't supposed to have the same amount of power, or different powers altogether, the nomenclature should reflect that. With the current state and naming of the roles, it could give users a wrong idea of centralization.

We suggest that roles are named differently so they are distinct and each role shows the power it has, for both the user and the ones entitled to the roles. If they're meant to symbolize the same amount of power its recommended to use the same nomenclature across every contract and function for simplicity and readability.

4. Function structure not consistent throughout contracts

Some functions like pendingReward in line 75 in yVaultLPFarming.sol (https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L75) use a vertical arrangement of the visibility and return variables, which isn’t consistent with some other functions, including the function right below that uses a horizontal arrangement of the visibility and modifier or even _computeUpdate, in line 168 (https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L168).

To help with readability, we suggest that functions are consistent in the way they’re presented, either in a vertical or horizontal structure, instead of changing constantly.

5. Typos

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/NFTVault.sol#L67 "insuraceRepurchaseTimeLimit" should be "insuranceRepurchaseTimeLimit", same issue is present at line 888 and 928 where this variable used. While this typo in the variable name does not affect contract behavior, fix these typos for better readability. ( https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/NFTVault.sol#L888 https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/NFTVault.sol#L928)

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/escrow/NFTEscrow.sol#L30 "specifc" should be "specific" https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L129 "contrat" should be "contract" https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L117 "isntead" should be "instead" https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L95 "Whereter" should be "Whether"

Use pre-increments rather than post-increments as it saves gas. One example of this can be seen in line 181 of NFTVault.sol. (https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L181)