JPEG'd contest - rfa's results

LOW #1 No check that totalStake >= _amount https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L117-L130 As the function check that balanceOf[msg.sender] >= _amount, the function should check that totalStaked >= _amount. Then use unchecked for calculation of balanceOf and totalStaked for gas optimization:

    function withdraw(uint256 _amount) external noContract(msg.sender) {
        require(_amount > 0, "invalid_amount");
        require(balanceOf[msg.sender] >= _amount, "insufficient_amount");
	require(totalStaked >= _amount, "insufficient_amount");

        _update();
        _withdrawReward(msg.sender);

        Unchecked{
		balanceOf[msg.sender] -= _amount;
        	totalStaked -= _amount;
	}
        vault.safeTransfer(msg.sender, _amount);

        emit Withdraw(msg.sender, _amount);
    }

#2 Comment line might mismatch with the code https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L44 In case owner has reset the lockTime through setLockTime function, The comment won't match the code anymore Change it to:

    /// @notice Locks `_lockAmount` tokens for account `_account` and NFT `_nftIndex` for lockTime duration. 

#3 Unnecessary _newTime check in setLockTime function https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L40 In case owner set _newTime = 1, the check won't revert anything and it has almost the same impact as _newTime = 0 (its just 1 sec different).

RECOMMENDED MITIGATION Set the minimum time for lockTime duration or just remove the check for gas opt

#4 No check that _lockAmount != 0 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L54 IMPACT Owner can just push positions mapping with 0 amount nft and nothing to unlock with it. It might just fill positions with nothing

#1 Title: Using != is more efficient https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L114 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L218 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L239 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/staking/JPEGStaking.sol#L32 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/staking/JPEGStaking.sol#L46 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L143 Using != instead of > is more gas efficient for checking that the var is not 0

#2 Title: Using ERC20.function instead of SafeERC20.function lib for jpeg https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L128 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L130 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L339 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L356 ERC20.functions is way cheaper to use. Its unnecessary to use SafeERC20 lib because jpeg is ERC20 standard token

#3 Title: Using if statement instead else if https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L129 The execution on condition in L127 and L129 won't executed at the same time. Using if statement can reduce 37 gas consumption

        if (remainingRewards > newRewards) {
            jpeg.safeTransfer(msg.sender, remainingRewards - newRewards);
        }
	if (remainingRewards < newRewards) { //@audit-info: Replacing else if with if statement here
            jpeg.safeTransferFrom(
                msg.sender,
                address(this),
                newRewards - remainingRewards
        );

#4 Title: gas opt in set() function https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L159 Checking that prevAllocPoint != _allocPoint early can save gas consumption (or just throw an error message earlier), than just execute all the line above it Change to:

    function set(uint256 _pid, uint256 _allocPoint) external onlyOwner {
	_massUpdatePools();
	uint256 prevAllocPoint = poolInfo[_pid].allocPoint;
        if (prevAllocPoint != _allocPoint) { //@audit-info: check here
		

        	poolInfo[_pid].allocPoint = _allocPoint; //@audit-info: prevent SSTORE here
        
            	totalAllocPoint = totalAllocPoint - prevAllocPoint + _allocPoint;
        }
    }

Its will prevent unnecessary SSTORE

#5 Title: Using += to increase value on var https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L226 Change:

        user.amount = user.amount + _amount;

To

        user.amount += _amount;

Also at: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L145

#6 Title: Using delete statement to empty userReward https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L340 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L357 Change:

        userRewards[msg.sender] = 0;

To:

	delete userRewards[msg.sender];

#7 Title: Gas improvement on returning _withdrawReward value https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L315-L327 By setting pending in function returns and deleting L326 can save gas. Change to:

    function _withdrawReward(uint256 _pid) internal returns (uint256 pending) { //@audit-info: return
        UserInfo storage user = userInfo[_pid][msg.sender];
        uint256 pending = (user.amount *
            (poolInfo[_pid].accRewardPerShare - user.lastAccRewardPerShare)) /
            1e36;
        if (pending > 0) {
            userRewards[msg.sender] += pending;
        }

        user.lastAccRewardPerShare = poolInfo[_pid].accRewardPerShare;
	//@audit-info: no return
    }

Same at: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L177-L186

#8 Title: Unnecessary var init with default value https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L281 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L348 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L231 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L181-L184 Declaring uint with 0 value is gas consuming. Just remove = 0

#9 Title: Using unchecked and prefix increment https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L281 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L348 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L231 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L181-L184 Change to:

        for (uint256 pid = 0; pid < length;) {
            _updatePool(pid);
	unchecked{
		++pid //@audit-info: Place here with unchecked
		};
        }

#10 Title: Caching poolInfo.length can save gas https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L348 As the loop in _massUpdatePools which caching poolInfo.length, same method can be used in claimAll function

#11 Title: Unnecessary MSTORE newReward https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L170 MSTORing the calculation is unnecessary and cost gas. Just pass cuerrnetBalance - previousBalance to the newAccRewardPerShare calculation

newAccRewardsPerShare = accRewardPerShare + (currentBalance - previousBalance) * 1e36 / totalStaked;

#12 Title: Using msg.sender instead _msgSender() https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L26 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L40 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L54 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L68 Using msg.sender instead _msgSender() is more effective. I recommend to replace all _msgSender() with msg.sender

#13 Title: Using && is less gas optimum in require https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L68 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L100 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L93-L98 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L194 Using multiple require() than && can save 15 execution gas fee, however it cost more on deployment gas fee(better use on function which are called many times):

require(
            _performanceFee.denominator > 0,
            "INVALID_RATE"
        );
require(
                _performanceFee.denominator >= _performanceFee.numerator,
            "INVALID_RATE"
        );

#14 Title: Using delete statement to set mapping false https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L76 Using delete statement to set mapping with bool value to false is more effective

        delete approvedStrategies[_token][_strategy];

#15 Title: Don't need to use == true to validate bool value == true https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L87 Change to:

require(
            approvedStrategies[_token][_strategy], // @audit-info remove == true
            "STRATEGY_NOT_APPROVED"
        );

#16 Title: Using calldata to declare read only struct parameter https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L70 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L212 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L222 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L232 Using calldata to store _creditLimitRate can save gas because it is read only var

#17 Title: Using if statement to determine amount value https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L182 Changing the code to:

if(amount > debtAmount)amount = debtAmount;

Can save execution gas fee in most case:

• if amount < debtAmount, it will save 16 gas because of avoiding MSTORE
• if amount == debtAmount, same
• if amount > debtAmount, 2 more expensive gas cost. Considering that WHITELISTED_ROLE will inputting amount is > than debtAmount is the rarest case from all cases above, this way is the most effective way

#18 Title: Using unchecked to calculate collateralAmount in withdraw() https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L199 amount value was checked that it won't > than collateralAmount so using unchecked can save gas:

unchecked{
	collateralAmount -= amount;
}

#19 Title: Unnecessary creditLimit MSTORE in withdraw() https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L196 By passing getCreditLimit(collateralAmount - amount) directly to L197 without storing it to credit limit can save gas without damaging readability of the code (The function name is clear that we are getting credit limit value):

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

#20 Title: Using storage to store initializer struct var can save gas https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L182 Reading from storage than caching struct to memory can save execution gas cost:

            NFTCategoryInitializer storage initializer = _typeInitializers[i];

#21 Title: Tight vars packing in PositionPreview struct https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L610-L623 Bool has 1 byte size, address has 20 bytes size (< 32). Grouping them together can save 1 slot, which can save gas:

struct PositionPreview {
        address owner;
        bool liquidatable; //@audit-info: move here
        uint256 nftIndex;
        bytes32 nftType;
        uint256 nftValueUSD;
        VaultSettings vaultSettings;
        uint256 creditLimit;
        uint256 debtPrincipal;
        uint256 debtInterest;
        BorrowType borrowType;
        uint256 liquidatedAt;
        address liquidator;
    }

#22 Title: Avoiding MLOAD by using msg.sender https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L559-L574 It is clear that _owner in _openPosition is msg.sender. Replacing it with msg.sender can save gas Change it to:

    function _openPosition(address, uint256 _nftIndex) internal {
        nftContract.transferFrom(msg.sender, address(this), _nftIndex); //@audit-info: here

        positions[_nftIndex] = Position({
            borrowType: BorrowType.NOT_CONFIRMED,
            debtPrincipal: 0,
            debtPortion: 0,
            debtAmountForRepurchase: 0,
            liquidatedAt: 0,
            liquidator: address(0)
        });
        positionOwner[_nftIndex] = msg.sender;
        positionIndexes.add(_nftIndex);

        emit PositionOpened(msg.sender, _nftIndex);
    }