QA Report

[code423n4]

[N] Use the Checks-Effects-Interactions Pattern

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L339-L340

        jpeg.safeTransfer(msg.sender, rewards);
        userRewards[msg.sender] = 0;

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L356-L357

        jpeg.safeTransfer(msg.sender, rewards);
        userRewards[msg.sender] = 0;

[L] Critical operations should emit events

Across the contracts, there are certain critical operations that change critical values that affect the users of the protocol.

It's a best practice for these setter functions to emit events to record these changes on-chain for off-chain monitors/tools/interfaces to register the updates and react if necessary.

Instances include:

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/yVault.sol#L108-L111

    function setController(address _controller) public onlyOwner {
        require(_controller != address(0), "INVALID_CONTROLLER");
        controller = IController(_controller);
    }

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/yVault.sol#L115-L118

    function setFarmingPool(address _farm) public onlyOwner {
        require(_farm != address(0), "INVALID_FARMING_POOL");
        farm = _farm;
    }

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/Controller.sol#L44-L51

    function setVault(IERC20 _token, address _vault)
        external
        onlyRole(STRATEGIST_ROLE)
    {
        require(vaults[_token] == address(0), "ALREADY_HAS_VAULT");
        require(_vault != address(0), "INVALID_VAULT");
        vaults[_token] = _vault;
    }

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/Controller.sol#L56-L64

    function approveStrategy(IERC20 _token, IStrategy _strategy)
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
    {
        require(address(_token) != address(0), "INVALID_TOKEN");
        require(address(_strategy) != address(0), "INVALID_STRATEGY");

        approvedStrategies[_token][_strategy] = true;
    }

[L] Precision loss due to div before mul

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L593-L595

        uint256 interestPerSec = interestPerYear / 365 days;

        return elapsedTime * interestPerSec;

Can be changed to:

        return elapsedTime * interestPerYear / 365 days;

[N] transfer() is not recommended for sending native token

Since the introduction of transfer(), it has typically been recommended by the security community because it helps guard against reentrancy attacks. This guidance made sense under the assumption that gas costs wouldn’t change. It's now recommended that transfer() and send() be avoided, as gas costs can and will change and reentrancy guard is more commonly used.

Any smart contract that uses transfer() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.

It's recommended to stop using transfer() and switch to using call() instead.

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/FungibleAssetVaultForDAO.sol#L193-L206

    function withdraw(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {
        require(amount > 0 && amount <= collateralAmount, "invalid_amount");

        uint256 creditLimit = getCreditLimit(collateralAmount - amount);
        require(creditLimit >= debtAmount, "insufficient_credit");

        collateralAmount -= amount;

        if (collateralAsset == ETH) payable(msg.sender).transfer(amount);
        else
            IERC20Upgradeable(collateralAsset).safeTransfer(msg.sender, amount);

        emit Withdraw(msg.sender, amount);
    }

Consider using OpenZeppelin's AddressUpgradeable#sendValue():

    function withdraw(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {
        require(amount > 0 && amount <= collateralAmount, "invalid_amount");

        uint256 creditLimit = getCreditLimit(collateralAmount - amount);
        require(creditLimit >= debtAmount, "insufficient_credit");

        collateralAmount -= amount;

        if (collateralAsset == ETH) payable(msg.sender).sendValue(amount);
        else
            IERC20Upgradeable(collateralAsset).safeTransfer(msg.sender, amount);

        emit Withdraw(msg.sender, amount);
    }

See: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v4.5.2/contracts/utils/AddressUpgradeable.sol#L60-L65