JPEG'd contest - TerrierLover's results

Should use the consistent increment (orefix or postfix) in the for loop in LPFarming.sol

Target codebase

There are two for loops used in LPFarming.sol. ++pid is used at one loop while i++ is used at the other loop.

2022-04-jpegd/contracts/farming/LPFarming.sol 281: for (uint256 pid = 0; pid < length; ++pid) { 348: for (uint256 i = 0; i < poolInfo.length; i++) {

Potential workaround

At the whole codebase, postfix increment is used. So it may be logical to use the postfix increment.

---

Error message in require function should be consistent among the codebase

Target codebase

Error message in the require check seems not to be consistent. The below code is an example: one uses invalid_amount while the other uses INVALID_AMOUNT.

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/staking/JPEGStaking.sol#L32

require(_amount > 0, "invalid_amount");

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/yVault/yVault.sol#L143

require(_amount > 0, "INVALID_AMOUNT");

Potential workaround

For the consistency, it should standardize the error message among the codebase for the better code quality.

---

_msgSender() should be used instead of msg.sender

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/tokens/JPEG.sol#L16

_mint(msg.sender, totalSupply);
_setupRole(DEFAULT_ADMIN_ROLE, _msgSender());

To be consistent, _msgSender() should be used.

Potential workaround

_mint(_msgSender(), totalSupply);
_setupRole(DEFAULT_ADMIN_ROLE, _msgSender());

---

The naming of _collateralUnit variable is inconsistent at FungibleAssetVaultForDAO.sol

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/FungibleAssetVaultForDAO.sol#L51

uint256 private _collateralUnit;

Other variables does not have _ at its prefix, but only _collateralUnit contains _.

Potential workaround

Simply remove _ from its prefix.

uint256 private collateralUnit;

---

Consider adopting nonReentrant at stake function in JPEGStaking.sol

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/staking/JPEGStaking.sol#L31

function stake(uint256 _amount) external {

unstake function has nonReentrant modifier but stake function does not. For the consistency, it may not harm adding nonReentrant modifier at stake function as well.

Potential workaround

function stake(uint256 _amount) external nonReentrant {

---

Using != 0 instead of > 0 can reduce the gas cost at various contracts

Target codebase

2022-04-jpegd/contracts/farming/LPFarming.sol 114: require(_rewardPerBlock > 0, "Invalid reward per block"); 218: require(_amount > 0, "invalid_amount"); 239: require(_amount > 0, "invalid_amount"); 320: if (pending > 0) { 337: require(rewards > 0, "no_reward"); 354: require(rewards > 0, "no_reward");

2022-04-jpegd/contracts/farming/yVaultLPFarming.sol 84: if (block.number > lastRewardBlock && staked > 0) { 101: require(_amount > 0, "invalid_amount"); 118: require(_amount > 0, "invalid_amount"); 139: require(rewards > 0, "no_reward"); 181: if (pending > 0) userPendingRewards[account] += pending;

2022-04-jpegd/contracts/lock/JPEGLock.sol 40: require(_newTime > 0, "Invalid lock time");

2022-04-jpegd/contracts/staking/JPEGStaking.sol 32: require(_amount > 0, "invalid_amount"); 46: _amount > 0 && _amount <= balanceOf(msg.sender),

2022-04-jpegd/contracts/vaults/FungibleAssetVaultForDAO.sol 94: _creditLimitRate.denominator > 0 && 142: require(amount > 0, "invalid_amount"); 164: require(amount > 0, "invalid_amount"); 180: require(amount > 0, "invalid_amount"); 194: require(amount > 0 && amount <= collateralAmount, "invalid_amount");

2022-04-jpegd/contracts/vaults/NFTVault.sol 278: require(_newFloor > 0, "Invalid floor"); 327: _type == bytes32(0) || nftTypeValueETH[_type] > 0, 365: require(pendingValue > 0, "no_pending_value"); 402: rate.denominator > 0 && rate.denominator >= rate.numerator, 637: uint256 debtAmount = positions[_nftIndex].liquidatedAt > 0 687: require(_amount > 0, "invalid_amount"); 764: require(_amount > 0, "invalid_amount"); 770: require(debtAmount > 0, "position_not_borrowed"); 882: require(position.liquidatedAt > 0, "not_liquidated"); 926: require(position.liquidatedAt > 0, "not_liquidated");

2022-04-jpegd/contracts/vaults/yVault/yVault.sol 100: _rate.numerator > 0 && _rate.denominator >= _rate.numerator, 143: require(_amount > 0, "INVALID_AMOUNT"); 167: require(_shares > 0, "INVALID_AMOUNT"); 170: require(supply > 0, "NO_TOKENS_DEPOSITED");

2022-04-jpegd/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol 182: _performanceFee.denominator > 0 && 322: if (balance > 0) 334: require(wethBalance > 0, "NOOP");

Potential improvement

Use use != 0 instead of > 0 at the above codebase. It checks with uint256 variable, so != and > 0 has a same meaning gramatically. Using != 0 can reduce the gas deployment cost of these contracts.

---

bytecode variable does not need to be defined

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/escrow/NFTEscrow.sol#L93-L101

bytes memory bytecode = _encodeFlashEscrow(_idx);

//hash from which the contract address can be derived
bytes32 hash = keccak256(
    abi.encodePacked(
        bytes1(0xff),
        address(this),
        salt,
        keccak256(bytecode)

Potential improvement

Simply do not define bytecode variable to reduce the gas cost.

//hash from which the contract address can be derived
bytes32 hash = keccak256(
    abi.encodePacked(
        bytes1(0xff),
        address(this),
        salt,
        keccak256(_encodeFlashEscrow(_idx))

---

Do not need to set 0 at the variable used in for loop

Target codebase

2022-04-jpegd/contracts/farming/LPFarming.sol 281: for (uint256 pid = 0; pid < length; ++pid) { 348: for (uint256 i = 0; i < poolInfo.length; i++) {

2022-04-jpegd/contracts/vaults/NFTVault.sol 181: for (uint256 i = 0; i < _typeInitializers.length; i++) { 184: for (uint256 j = 0; j < initializer.nfts.length; j++) {

2022-04-jpegd/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol 145: for (uint256 i = 0; i < _strategyConfig.rewardTokens.length; i++) { 231: for (uint256 i = 0; i < length; i++) { 319: for (uint256 i = 0; i < rewardTokens.length; i++) {

Potential improvement

Just do not initialize with 0 for the variable used in for loop. The example code is as follows:

for (uint256 pid; pid < length; ++pid) {

---

No need to define staked variable at yVaultLPFarming.sol

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/farming/yVaultLPFarming.sol#L81-L84

uint256 staked = totalStaked;
//if blockNumber is greater than the pool's `lastRewardBlock` the pool's `accRewardPerShare` is outdated,
//we need to calculate the up to date amount to return an accurate reward value
if (block.number > lastRewardBlock && staked > 0) {
    (rewardShare, ) = _computeUpdate();
}

staked variable is not used anywhere, so there is no need to define staked variable.

Potential improvement

//if blockNumber is greater than the pool's `lastRewardBlock` the pool's `accRewardPerShare` is outdated,
//we need to calculate the up to date amount to return an accurate reward value
if (block.number > lastRewardBlock && totalStaked > 0) {
    (rewardShare, ) = _computeUpdate();
}

This change can reduce the gas cost since it does not define variable.

---

No need to define newRewards variable at yVaultLPFarming.sol

Target codebase

newRewards is only used once so no need to be defined.

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/farming/yVaultLPFarming.sol#L170-L172

uint256 newRewards = currentBalance - previousBalance;

newAccRewardsPerShare = accRewardPerShare + newRewards * 1e36 / totalStaked;

Potential improvement

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

---

No need to define account variable at _transferFrom function in CryptoPunksHelper.sol

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/helpers/CryptoPunksHelper.sol#L72-L77

address account = punks.punkIndexToAddress(_idx);

//if the owner is this address we don't need to go through {NFTEscrow}
if (account != address(this)) {
    _executeTransfer(_from, _idx);
}

Potential improvement

//if the owner is this address we don't need to go through {NFTEscrow}
if (punks.punkIndexToAddress(_idx) != address(this)) {
    _executeTransfer(_from, _idx);
}

---

unchecked can be used to reduce the gas cost at repay function in FungibleAssetVaultForDAO.sol

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/FungibleAssetVaultForDAO.sol#L184

function repay(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {
    require(amount > 0, "invalid_amount");

    amount = amount > debtAmount ? debtAmount : amount;

    debtAmount -= amount;

At line 184, debtAmount - amount becomes more than or equals to 0. So debtAmount -= amount; can be wrapped by unchecked.

Potential improvement

function repay(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {
    require(amount > 0, "invalid_amount");

    amount = amount > debtAmount ? debtAmount : amount;

    unchecked {
      debtAmount -= amount;
    }

---

unchecked can be used to reduce the gas cost at withdraw function in FungibleAssetVaultForDAO.sol

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/FungibleAssetVaultForDAO.sol#L199

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;

At line 184, collateralAmount - amount becomes more than or equals to 0. So collateralAmount -= amount; can be wrapped by unchecked.

Potential improvement

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");

    unchecked {
      collateralAmount -= amount;
    }

---

No need to define collateralValue variable at getCreditLimit function in FungibleAssetVaultForDAO.sol

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/FungibleAssetVaultForDAO.sol#L132

Potential improvement

Simply avoiding defining collateralValue.

function getCreditLimit(uint256 amount) public view returns (uint256) {
    return
        (_getCollateralValue(amount) * creditLimitRate.numerator) /
        creditLimitRate.denominator;
}

This would reduce the deployment gas cost.

---

No need to define creditLimit variable at borrow function in FungibleAssetVaultForDAO.sol

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/FungibleAssetVaultForDAO.sol#L166-L168

uint256 creditLimit = getCreditLimit(collateralAmount);
uint256 newDebtAmount = debtAmount + amount;
require(newDebtAmount <= creditLimit, "insufficient_credit");

creditLimit variable is only used once, so it can avoid be defined.

Potential improvement

Simply avoid defining creditLimit. This would decrease the deployment gas cost.

uint256 newDebtAmount = debtAmount + amount;
require(newDebtAmount <= getCreditLimit(collateralAmount), "insufficient_credit");

---

No need to define creditLimit variable at withdraw function in FungibleAssetVaultForDAO.sol

Target codebase

https://github.com/code-423n4/2022-04-jpegd/blob/59e288c27e1ff1b47505fea2e5434a7577d85576/contracts/vaults/FungibleAssetVaultForDAO.sol#L196-L197

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

creditLimit variable is only used once, so it can avoid be defined.

Potential improvement

Simply avoid defining creditLimit. This would decrease the deployment gas cost.

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

---