JPEG'd contest - WatchPug's results

Lines of code

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

Vulnerability details

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

function deposit(uint256 _amount) public noContract(msg.sender) {
    require(_amount > 0, "INVALID_AMOUNT");
    uint256 balanceBefore = balance();
    token.safeTransferFrom(msg.sender, address(this), _amount);
    uint256 supply = totalSupply();
    uint256 shares;
    if (supply == 0) {
        shares = _amount;
    } else {
        //balanceBefore can't be 0 if totalSupply is > 0
        shares = (_amount * supply) / balanceBefore;
    }
    _mint(msg.sender, shares);

    emit Deposit(msg.sender, _amount);
}

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

function withdraw(uint256 _shares) public noContract(msg.sender) {
    require(_shares > 0, "INVALID_AMOUNT");

    uint256 supply = totalSupply();
    require(supply > 0, "NO_TOKENS_DEPOSITED");

    uint256 backingTokens = (balance() * _shares) / supply;
    _burn(msg.sender, _shares);

    // Check balance
    uint256 vaultBalance = token.balanceOf(address(this));
    if (vaultBalance < backingTokens) {
        uint256 toWithdraw = backingTokens - vaultBalance;
        controller.withdraw(address(token), toWithdraw);
    }

    token.safeTransfer(msg.sender, backingTokens);
    emit Withdrawal(msg.sender, backingTokens);
}

A malicious early user can deposit() with 1 wei of asset token as the first depositor of the Vault, and get 1 wei of shares token.

Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) .

As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token.

They will immediately lose 9999e18 or half of their deposits if they withdraw() right after the deposit().

Recommendation

Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a port of the initial mints as a reserve to the DAO so that the pricePerShare can be more resistant to manipulation.

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/lock/JPEGLock.sol#L49-L63

Vulnerability details

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/lock/JPEGLock.sol#L49-L63

function lockFor(
    address _account,
    uint256 _nftIndex,
    uint256 _lockAmount
) external onlyOwner nonReentrant {
    jpeg.safeTransferFrom(_account, address(this), _lockAmount);

    positions[_nftIndex] = LockPosition({
        owner: _account,
        unlockAt: block.timestamp + lockTime,
        lockAmount: _lockAmount
    });

    emit Lock(_account, _nftIndex, _lockAmount);
}

Based on the context, and given the volatility of the NFT market, the DAO may give the same NFT at different prices at different times.

When that happens, in the current implementation, the locked JPEG tokens belonging to the previous owner will be frozen in the contract as the record will be overwritten by the new lock.

PoC

Given:

• creditLimitRate: 50%
• valueIncreaseLockRate: 30%

1. DAO setPendingNFTValueETH() for Alice's NFT#1 with a price of 10 ETH;
2. Alice finalizePendingNFTValueETH() and locked 1.5 ETH worth of JPEG tokens;
3. DAO adjusted the assessment and setPendingNFTValueETH() for Alice's NFT#1 with a price of 20 ETH;
4. Alice finalizePendingNFTValueETH() and locked 3 ETH worth of JPEG tokens.

The 1.5 ETH worth of JPEG tokens locked in step 2 won't be able to be unlocked as the LockPosition was overwritten in step 4.

Recommendation

Change to:

function lockFor(
    address _account,
    uint256 _nftIndex,
    uint256 _lockAmount
) external onlyOwner nonReentrant {
    LockPosition memory position = positions[_nftIndex];
    if (position.lockAmount > 0) {
        jpeg.safeTransfer(position.owner, position.lockAmount);
    }
    jpeg.safeTransferFrom(_account, address(this), _lockAmount);
    
    positions[_nftIndex] = LockPosition({
        owner: _account,
        unlockAt: block.timestamp + lockTime,
        lockAmount: _lockAmount
    });

    emit Lock(_account, _nftIndex, _lockAmount);
}

Lines of code

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

Vulnerability details

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

uint256 debtAmount = _getDebtAmount(_nftIndex);
require(
    debtAmount >= _getLiquidationLimit(_nftIndex),
    "position_not_liquidatable"
);

// burn all payment
stablecoin.burnFrom(msg.sender, debtAmount);

In the current design/implementation, the liquidator must fully repay the user's outstanding debt in order to get the NFT.

When the market value of the NFT fell rapidly, the liquidators may not be able to successfully liquidate as they can not sell the NFT for more than the debt amount.

In that case, the protocol will have positions that are considered bad debts.

However, these loans, which may never be repaid, are still accruing interest. And every time the DAO collects interest, new stablecoin will be minted.

When the proportion of bad debts is large enough since the interest generated by these bad debts is not backed. It will damage the authenticity of the stablecoin.

PoC

Given:

• NFT 1 worth 30,000 USD
• creditLimitRate = 60%
• liquidationLimitRate = 50%
• debtInterestApr = 10%

1. Alice borrowed 10,000 USD with NFT #1;
2. After 1 year, NFT 1's market value in USD has suddenly dropped to 10,000 USD, no liquidator is willing to repay 11,000 USD for NFT #1;
3. The DAO collect() and minted 1,000 stablecoin;
4. After 1 year, the DAO call collect() will mint 1,100 stablecoin. and so on...

Recommendation

Consider adding a stored value to record the amount of bad debt, and add a public function that allows anyone to mark a bad debt to get some reward. and change accrue to:

uint256 internal badDebtPortion;

function accrue() public {
    uint256 additionalInterest = _calculateAdditionalInterest();

    totalDebtAccruedAt = block.timestamp;

    totalDebtAmount += additionalInterest;

    uint256 collectibleInterest = additionalInterest * (totalDebtPortion - badDebtPortion) / totalDebtPortion;
    totalFeeCollected += collectibleInterest;
}

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L311-L334

Vulnerability details

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L311-L334

function harvest(uint256 minOutCurve) external onlyRole(STRATEGIST_ROLE) {
    convexConfig.baseRewardPool.getReward(address(this), true);

    //Prevent `Stack too deep` errors
    {
        DexConfig memory dex = dexConfig;
        IERC20[] memory rewardTokens = strategyConfig.rewardTokens;
        IERC20 _weth = weth;
        for (uint256 i = 0; i < rewardTokens.length; i++) {
            uint256 balance = rewardTokens[i].balanceOf(address(this));

            if (balance > 0)
                //minOut is not needed here, we already have it on the Curve deposit
                _swapUniswapV2(
                    dex.uniswapV2,
                    rewardTokens[i],
                    _weth,
                    balance,
                    0
                );
        }

        uint256 wethBalance = _weth.balanceOf(address(this));
        require(wethBalance > 0, "NOOP");
        ...

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L410-L430

 function _swapUniswapV2(
    IUniswapV2Router router,
    IERC20 tokenIn,
    IERC20 tokenOut,
    uint256 amountIn,
    uint256 minOut
) internal {
    tokenIn.safeIncreaseAllowance(address(router), amountIn);

    address[] memory path = new address[](2);
    path[0] = address(tokenIn);
    path[1] = address(tokenOut);

    router.swapExactTokensForTokens(
        amountIn,
        minOut,
        path,
        address(this),
        block.timestamp
    );
}

In the current implementation, rewardTokens from the underlying strategy will be swapped to weth first then weth -> usdc.

However, the path used for swapping from rewardToken -> weth is hardcoded as [rewardToken, weth], which may not be the optimal route.

For example, the majority liquidity for a particular rewardToken may actually be in the rewardToken/USDC pool. Swapping through the rewardToken/WETH with low liquidity may end up getting only a dust amount of WETH.

Recommendation

Consider allowing the admin to set a path for the rewardTokens.

Lines of code

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

Vulnerability details

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

    function _normalizeAggregatorAnswer(IAggregatorV3Interface aggregator)
        internal
        view
        returns (uint256)
    {
        int256 answer = aggregator.latestAnswer();
        uint8 decimals = aggregator.decimals();

        require(answer > 0, "invalid_oracle_answer");
        //converts the answer to have 18 decimals
        return
            decimals > 18
                ? uint256(answer) / 10**(decimals - 18)
                : uint256(answer) * 10**(18 - decimals);
    }

According to Chainlink's documentation, the latestAnswer function is deprecated. This function does not error if no answer has been reached but returns 0, causing an incorrect price.

Recommendation

Consider using the latestRoundData method instead.

See: https://docs.chain.link/docs/historical-price-data/#solidity

[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

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S] Use else if can save gas

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

        if (blockNumber < epoch.startBlock) return epoch.startBlock;

        if (blockNumber > epoch.endBlock) return epoch.endBlock;

        return blockNumber;

Change to else if can save gas:

        if (blockNumber < epoch.startBlock) {
            return epoch.startBlock;
        } 
        else if (blockNumber > epoch.endBlock){
            return epoch.endBlock;
        } 
        return blockNumber;

[S] Cache array length in for loops can save gas

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

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

• poolInfo.length

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L145-L150

• _strategyConfig.rewardTokens.length

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L319-L331

• rewardTokens.length

[M] ++i is more efficient than i++

Using ++i is more gas efficient than i++, especially in for loops.

For example:

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

        for (uint256 i = 0; i < poolInfo.length; i++) 

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

        for (uint256 i = 0; i < _strategyConfig.rewardTokens.length; i++) 

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

        for (uint256 i = 0; i < length; i++)

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

        for (uint256 i = 0; i < rewardTokens.length; i++) 

[G-S] Using immutable variables can save gas

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/staking/JPEGStaking.sol#L17-L18

    /// @notice The stake token, JPEG
    IERC20Upgradeable public jpeg;

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/staking/JPEGStaking.sol#L21-L26

    function initialize(IERC20Upgradeable _jpeg) external initializer {
        __ReentrancyGuard_init();
        __ERC20_init("sJPEG", "sJPEG");
        __ERC20Permit_init("sJPEG");
        jpeg = _jpeg;
    }

Considering that jpeg will never change, changing it to immutable variable instead of storage variable can save gas.

[S] Outdated versions of OpenZeppelin library

Outdated versions of OpenZeppelin library are used.

It's a best practice to use the latest version of libraries.

New versions of OpenZeppelin libraries can be more gas efficient.

For example:

ERC20.sol in @openzeppelin/contracts@4.3.1:

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/yarn.lock#L794-L797

"@openzeppelin/contracts@^4.0.0":
  version "4.3.1"
  resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-4.3.1.tgz#c01f791ce6c9d3989ac1a643267501dbe336b9e3"
  integrity sha512-QjgbPPlmDK2clK1hzjw2ROfY8KA5q+PfhDUUxZFEBCZP9fi6d5FuNoh/Uq0oCTMEKPmue69vhX2jcl0N/tFKGw==

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.3.1/contracts/token/ERC20/ERC20.sol#L154-L160

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) public virtual override returns (bool) {
        _transfer(sender, recipient, amount);

        uint256 currentAllowance = _allowances[sender][_msgSender()];
        require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance");
        unchecked {
            _approve(sender, _msgSender(), currentAllowance - amount);
        }

        return true;
    }

A gas optimization upgrade has been added to @openzeppelin/contracts@4.5.0:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.5.0/contracts/token/ERC20/ERC20.sol#L163-L165

    function transferFrom(
        address from,
        address to,
        uint256 amount
    ) public virtual override returns (bool) {
        address spender = _msgSender();
        _spendAllowance(from, spender, amount);
        _transfer(from, to, amount);
        return true;
    }

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.5.0/contracts/token/ERC20/ERC20.sol#L330-L342

    function _spendAllowance(
        address owner,
        address spender,
        uint256 amount
    ) internal virtual {
        uint256 currentAllowance = allowance(owner, spender);
        if (currentAllowance != type(uint256).max) {
            require(currentAllowance >= amount, "ERC20: insufficient allowance");
            unchecked {
                _approve(owner, spender, currentAllowance - amount);
            }
        }
    }