JPEG'd contest - rayn's results

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L347 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L360 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L49

Vulnerability details

Impact

NFTVault contract allows anyone to finalize a pending NFT value proposal by calling the finalizePendingNFTValueETH function. A finalizer must lock an equivalent amount of JPEG tokens to the proposed NFT value in JPEGLock, and can only withdraw those tokens when lock expires.

However, both proposing mechanism implemented in NFTVault.setPendingNFTValueETH and finalizing logic in NFTVault.finalizePendingNFTValueETH and JPEGLock.lockFor does not check whether a lock currently exist.

Thus if a new value proposal is made before current lock expires, anyone can finalize the new proposal, and JPEGLock.lockFor overwrites the previous record.

Since JPEGLock.unlock unlocks and returns JPEG token based on the values kept in positions, the previous locker who had his position overwritten effectively loses all locked JPEG tokens.

Proof of Concept

None of the proposal and acceptance functions perform checks on whether a positions[_nftIndex] currently exist.

contract NFTVault{
    ...
    function setPendingNFTValueETH(uint256 _nftIndex, uint256 _amountETH)
        external
        validNFTIndex(_nftIndex)
        onlyRole(DAO_ROLE)
    {
        pendingNFTValueETH[_nftIndex] = _amountETH;
    }

    function finalizePendingNFTValueETH(uint256 _nftIndex)
        external
        validNFTIndex(_nftIndex)
    {
        uint256 pendingValue = pendingNFTValueETH[_nftIndex];
        require(pendingValue > 0, "no_pending_value");
        uint256 toLockJpeg = (((pendingValue *
            _ethPriceUSD() *
            settings.creditLimitRate.numerator) /
            settings.creditLimitRate.denominator) *
            settings.valueIncreaseLockRate.numerator) /
            settings.valueIncreaseLockRate.denominator /
            _jpegPriceUSD();

        //lock JPEG using JPEGLock
        jpegLocker.lockFor(msg.sender, _nftIndex, toLockJpeg);

        nftTypes[_nftIndex] = CUSTOM_NFT_HASH;
        nftValueETH[_nftIndex] = pendingValue;
        //clear pending value
        pendingNFTValueETH[_nftIndex] = 0;
    }
    ...
}

contract JPEGLock{
    ...
    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);
    }
    ...
}

Combined with the unlock mechanisms, any overwritten records of locked JPEG will never be returned correctly. Resulting is loss of JPEG of previous lockers.

    function unlock(uint256 _nftIndex) external nonReentrant {
        LockPosition memory position = positions[_nftIndex];
        require(position.owner == msg.sender, "unauthorized");
        require(position.unlockAt <= block.timestamp, "locked");

        delete positions[_nftIndex];

        jpeg.safeTransfer(msg.sender, position.lockAmount);

        emit Unlock(msg.sender, _nftIndex, position.lockAmount);
    }

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Either return JPEG to the previous locker when a new proposal is finalized.

    function lockFor(...) external onlyOwner nonReentrant {
        if(position[_nftIndex].owner != address(0)) {
            jpeg.safeTransfer(position[_nftIndex].owner, positions[_nftIndex].lockAmount);
        }
        ...
    }

Or disallow finalization of proposals that collide with an ongoing position.

    function lockFor(...) external onlyOwner nonReentrant {
        require(position[_nftIndex].owner == address(0), "position occupied");
        ...
    }

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L95 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L257 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L303

Vulnerability details

Impact

Whenever STRATEGISTs want to switch from currently used strategy to another one, they are required to call the Controller.setStrategy function. This function is responsible for first withdrawing CRV and JPEG tokens from strategy contract into vault, then proceed to reset the mapping of CRV token to strategy.

However, while the correct method to withdraw JPEG token should be through strategy.withdrawJPEG, Controller attempts to perform withdrawal through strategy.withdraw(address(jpeg)).

The strategy.withdraw(address) function is originally intended to allow strategists to withdraw unrelated tokens that are accidentally deposited to strategies, and explicitly block withdrawal of JPEG tokens. This causes Controller.setStrategy to revert whenever it tries to change strategy and withdraw JPEG tokens from old instance.

Proof of Concept

Controller.setStrategy attempts to withdraw JPEG tokens in strategy through _current.withdraw(address(jpeg)) whenever a switch is requested

    function setStrategy(IERC20 _token, IStrategy _strategy) external onlyRole(STRATEGIST_ROLE){
        ...
        IStrategy _current = strategies[_token];
        if (address(_current) != address(0)) {
            ...
            _current.withdraw(address(jpeg));
        }
        ...
    }

However, the strategy.withdraw(address) function is not intended to be used for JPEG withdrawal and explicitly reverts if it finds target token to be JPEG

    function withdraw(IERC20 _asset) external onlyController returns (uint256 balance) {
        ...
        require(jpeg != _asset, "jpeg");
        ...
    }

This causes Controller.setStrategy to always revert in the case which an STRATEGISTs want to swap out an old strategy implementation

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Use the correct withdrawJPEG function to withdraw JPEG

    function setStrategy(IERC20 _token, IStrategy _strategy)
        external
        onlyRole(STRATEGIST_ROLE)
    {
        ...
        IStrategy _current = strategies[_token];
        if (address(_current) != address(0)) {
            ...
            _current.withdrawJPEG(vaults[_token]);
        }
        ...
    }

Lines of code

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

Vulnerability details

Impact

As specified in Convex BaseRewardPool.sol and VirtualRewardPool.sol, the function signature of earned is earned(address). However, balanceOfJPEG did not pass any arguments to earned, which would cause balanceOfJPEG to always revert.

This bug will propagate through Controller and YVault until finally reaching the source of the call in YVaultLPFarming ._computeUpdate, and render the entire farming contract unuseable.

Proof of Concept

Both BaseRewardPool.earned and VirtualBalanceRewardPool.earned takes an address as argument

    function earned(address account) public view returns (uint256) {
        return
            balanceOf(account)
                .mul(rewardPerToken().sub(userRewardPerTokenPaid[account]))
                .div(1e18)
                .add(rewards[account]);
    }

    function earned(address account) public view returns (uint256) {
        return
            balanceOf(account)
                .mul(rewardPerToken().sub(userRewardPerTokenPaid[account]))
                .div(1e18)
                .add(rewards[account]);
    }

But balanceOfJPEG does not pass any address to extraReward.earned, causing the entire function to revert when called

    function balanceOfJPEG() external view returns (uint256) {
        uint256 availableBalance = jpeg.balanceOf(address(this));

        IBaseRewardPool baseRewardPool = convexConfig.baseRewardPool;
        uint256 length = baseRewardPool.extraRewardsLength();
        for (uint256 i = 0; i < length; i++) {
            IBaseRewardPool extraReward = IBaseRewardPool(baseRewardPool.extraRewards(i));
            if (address(jpeg) == extraReward.rewardToken()) {
                availableBalance += extraReward.earned();
                //we found jpeg, no need to continue the loop
                break;
            }
        }

        return availableBalance;
    }

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Pass address(this) as argument of earned.

Notice how we modify the fetching of reward. This is reported in a separate bug report, but for completeness, the entire fix is shown in both report entries.

    function balanceOfJPEG() external view returns (uint256) {
        uint256 availableBalance = jpeg.balanceOf(address(this));

        IBaseRewardPool baseRewardPool = convexConfig.baseRewardPool;
        availableBalance += baseRewardPool.earned(address(this));
        uint256 length = baseRewardPool.extraRewardsLength();
        for (uint256 i = 0; i < length; i++) {
            IBaseRewardPool extraReward = IBaseRewardPool(baseRewardPool.extraRewards(i));
            if (address(jpeg) == extraReward.rewardToken()) {
                availableBalance += extraReward.earned(address(this));
            }
        }

        return availableBalance;
    }

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L61 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L85 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L54

Vulnerability details

Impact

A contract in construction can bypass isContract to call deposit and withdraw functions in vaults/yVault/yVault.sol, farming/LPFarming.sol, and farming/yVaultLPFarming.sol. Also, Block contracts may cause DoS if someone uses multisig contracts as a caller.

Proof of Concept

The modifier noContract uses isContract() of Address library, which has security issues:

    /**
     * @dev Returns true if `account` is a contract.
     *
     * [IMPORTANT]
     * ====
     * It is unsafe to assume that an address for which this function returns
     * false is an externally-owned account (EOA) and not a contract.
     *
     * Among others, `isContract` will return false for the following
     * types of addresses:
     *
     *  - an externally-owned account
     *  - a contract in construction
     *  - an address where a contract will be created
     *  - an address where a contract lived, but was destroyed
     * ====
     *
     * [IMPORTANT]
     * ====
     * You shouldn't rely on `isContract` to protect against flash loan attacks!
     *
     * Preventing calls from contracts is highly discouraged. It breaks composability, breaks support for smart wallets
     * like Gnosis Safe, and does not provide security since it can be circumvented by calling from a contract
     * constructor.
     * ====
     */
    function isContract(address account) internal view returns (bool) {
        // This method relies on extcodesize/address.code.length, which returns 0
        // for contracts in construction, since the code is only stored at the end
        // of the constructor execution.

        return account.code.length > 0;
    }

A contract in construction can bypass isContract due to the code length is 0.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

While we are uncertain why developers made the decision of disallowing contracts from calling specific functions, !_account.isContract() could be replaced by msg.sender == tx.origin to perform a thorough check. However, it would probably be more desirable to add locks to guard against reentrancy attacks (if this is the reason for blocking calls from contract), and allow contracts as caller for interoperability with other defi systems.

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L454 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L410 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L441 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L447 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L105

Vulnerability details

Impact

Use latestAnswer in vaults/NFTVault.sol may get stale ETH price in USD (_ethPriceUSD()), stale value in ETH of the NFT (​​_getNFTValueETH()), stale JPEG price in USD (_jpegPriceUSD()), and also stale USD price of one unit of collateral asset (_collateralPriceUsd of vaults/FungibleAssetVaultForDAO.sol).

Proof of Concept

In the _normalizeAggregatorAnswer function of vaults/NFTVault.sol, it uses aggregator.latestAnswer() which will return the last value, but it doesn’t check if the value is fresh or not.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Use latestRoundData rather than latestAnswer and check the value:

       (
          roundId,
          answer,
          startedAt,
          updatedAt,
          answeredInRound
        ) = aggregator.latestRoundData();
        require(answer > 0, "Invalid answer");
        require(updatedAt != 0, "Invalid updatedAt");
        require(answeredInRound >= roundId, "Stale price");

Summary

We list 5 low-critical findings and 2 non-critical findings here:

• (Low) A address can be hijack to do bad thing
• (Low) setAvailableTokensRate() has a mistake requirement
• (Low) getNFTInfo does not check against invalid NFT
• (Low) Owner can extract all remaining rewards by specifying a new epoch
• (Low) The permission of STRATEGIST_ROLE is too powerful
• (Non) add function doesn’t return any information
• (Non) Uniswap v3 router does not have the quoteExactInput function

In conclusion, it's better to check invalid NFT, restrict owner’s permission and use minimum permission of roles.

(Low) A address can be hijack to do bad thing

Impact

In _executeTransfer of escrow/NFTEscrow.sol. Though it is extremely unlikely to happen, the non 0 chance of address collision with FlashEscrow may cause users to lose their NFT.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/escrow/NFTEscrow.sol#L68-L72

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Remove FlashEscrow, call _encodeFlashEscrowPayload() directly and use a mapping to record transactions relative to users.

(Low) setAvailableTokensRate() has a mistake requirement

Impact

The setAvailableTokensRate() function in vaults/yVault/yVault.sol should check denominator > 0, instead of numerator.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L100

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

_rate.numerator > 0 should be _rate.denominator > 0:

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

(Low) getNFTInfo does not check against invalid NFT

Impact

getNFTInfo doesn’t check against invalid NFT, it will be mistaken for a valid NFT if other contracts call getNFTInfo.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L481

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Use validNFTIndex to check validation.

(Low) Owner can extract all remaining rewards by specifying a new epoch

Impact

In farming/LPFarming.sol, Owner can call newEpoch() to extract all remaining rewards by specifying a new epoch.

Proof of Concept

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

Owner can create a new epoch with the same _endBlock and _startBlock, which will let newRewards equal to 0, and get all remainingRewards.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Check _endBlock - _startBlock and newRewards should be a valid value.

(Low) The permission of STRATEGIST_ROLE is too powerful

Impact

The STRATEGIST_ROLE can transfer any amount of token and call _strategy.withdraw without any check. It’s dangerous when a private key of a STRATEGIST_ROLE is stolen.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L131 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L141

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Remove inCaseTokensGetStuck and inCaseStrategyTokensGetStuck functions or restrict the ability of STRATEGIST_ROLE.

(Non) add function doesn’t return any information

Impact

add function in farming/LPFarming.sol doesn’t return any information (e.g. pid).

Proof of Concept

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

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

It’s better to return pid for other functions to use (e.g. set function).

(Non) Uniswap v3 router does not have the quoteExactInput function

Impact

In interface ISwapRouter of interfaces/ISwapRouter.sol, it define quoteExactInput but Uniswap V3 router doesn’t have this function.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/interfaces/ISwapRouter.sol#L23

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Delete quoteExactInput function.

Save gas in for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L181-L187 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L145 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/yVault/strategies/StrategyPUSDConvex.sol#L319 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

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}