JPEG'd contest - hyh's results

Lines of code

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

Vulnerability details

Impact

An attacker can become the first depositor for a recently created YVault contract, providing a tiny amount of token by calling deposit(1) (raw values here, 1 is 1 wei, 1e18 is 1 token if it is 18 decimals). Then the attacker can directly transfer, for example, 10^6*1e18 - 1 of the token to YVault contract, effectively setting the cost of 1 of vault token to be 10^6 * 1e18 of token. The attacker will still own 100% of the remaining token pool being the only depositor.

All subsequent depositors will have their token investments rounded to 10^6 * 1e18, due to the lack of precision which initial tiny deposit caused, with the remainder divided between all current depositors, i.e. the subsequent depositors lose value to the attacker.

For example, if the second depositor brings in 1.9*10^6 * 1e18, only 1 of new JPEGDtoken to be issued, which means that 2.9*10^6 * 1e18 total token pool be divided 50/50 between the second depositor and the attacker, as each have 1 wei of the total 2 wei of JPEGDtoken, i.e. the depositor lost and the attacker gained 0.45*10^6 * 1e18 of token.

As there are no penalties to exit, the attacker can remain staked for an arbitrary time, gathering the share of all new deposits' remainder amounts.

Proof of concept

deposit() doesn't require minimum amount and mints according to the provided amount:

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

When YVault is new the balance() is just initially empty contract balance:

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

Any deposit lower than total attacker's stake will be fully stolen from the depositor as 0 vault tokens will be issued in this case.

References

The issue is similar to the TOB-YEARN-003 one of the Trail of Bits audit of Yearn Finance:

https://github.com/yearn/yearn-security/tree/master/audits/20210719_ToB_yearn_vaultsv2

Recommended Mitigation Steps

A minimum for deposit value can drastically reduce the economic viability of the attack. I.e. deposit() can require each amount to surpass the threshold, and then an attacker would have to provide too big direct investment to capture any meaningful share of the subsequent deposits.

An alternative is to require only the first depositor to freeze big enough initial amount of liquidity. This approach has been used long enough in Uniswap V2:

https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L119-L121

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L87 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L56 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L63

Vulnerability details

Impact

LPFarming, yVault and yVaultLPFarming contracts' white list checks are performed with isContract() and can be surpassed. isContract() can only be used for positive confirmations, i.e. filtering out EOAs.

Setting high severity as it is a direct access control logic loophole for the number of user facing functions.

Proof of Concept

OZ documents 'it is unsafe to assume that an address for which this function returns false is an externally-owned account (EOA) and not a contract':

https://docs.openzeppelin.com/contracts/3.x/api/utils#Address-isContract-address-

https://forum.openzeppelin.com/t/how-to-write-an-onlynoncontract-modifier-function-cant-be-called-by-a-contract/3042/4

This way the approach implemented in OZ's Address can be used for positive confirmations only:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L36-L42

I.e. a contract can choose not to identify itself as a contract, but EOA can't choose to be identified as a contract, so only the cases where EOA should be filtered out are valid for isContract().

However, LPFarming, yVault and yVaultLPFarming control that an address isn't a contract with Address.isContract():

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

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

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

Recommended Mitigation Steps

Consider removing the filtration of contract callers altogether, focusing on direct attack surfaces (nonReentrant is a great example).

If there are strong enough considerations for filtering out of the contracts consider using tx.origin == msg.sender, which might not be deprecated for a long enough time as there is a reasonably big number of use cases around and backward compatibility is a strong argument in the ongoing discussion.

Lines of code

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

Vulnerability details

Impact

Chainlink's latestAnswer() usage can yield stale price information, which is crucial for borrowing and liquidation. latestAnswer() is having less ways to be controlled compared to latestRoundData(), which is advised for price sensitive operations.

Staling prices can lead to partial fund losses due via introduction of undercollateralized borrowing and well collateralized liquidation.

Placing severity to medium as the issue is for preventing the translation of Chainlink malfunction to functionality of the system, where the probability of the former is low (but not zero, especially for newer NFT price feeds).

Proof of Concept

FungibleAssetVaultForDAO uses latestAnswer to price collateral:

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

More importantly, NFTVault uses it as a default way to gather current ETH, JPEG and all the market valued NFT prices:

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

Recommended Mitigation Steps

Consider using latestRoundData(), which provides more flexibility and is advised by Chainlink for price retrieval:

https://docs.chain.link/docs/get-the-latest-price/

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

https://docs.chain.link/docs/feed-registry/#latestrounddata

An example:

(int256 roundID, int256 priceInUsd, , int256 updatedAt, int256 answeredInRound) = aggregator.latestRoundData();

require(priceInUsd > 0 && updatedAt > 0 && answeredInRound >= roundID , "Price invalid");

1. User facing functions miss emergency lever

Impact

If there be any emergency with system contracts or outside infrastructure, there is no way to temporary stop the operations.

Proof of Concept

Most core user-facing contracts do not have pausing functionality for new operation initiation.

For example, NFTVault’s user facing functions cannot be temporary paused:

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

LPFarming also can benefit from having pausing functionality:

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

Recommended Mitigation Steps

Consider making all new actions linked user facing functions pausable.

For example, by using OpenZeppelin's approach:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol

2. Floating pragma is used in most contracts

As different compiler versions have critical behavior specifics if the contracts get accidentally deployed using another compiler version compared to the one they were tested with, various types of undesired behavior can be introduced

Proof of Concept

pragma solidity ^0.8.0 is used in most contracts, for example:

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

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

Recommended Mitigation Steps

Consider fixing the version to 0.8.x across all the codebase, for example set x to 10

3. YVaultLPFarming's claim() doesn't have partial claim option

Impact

If there will be not enough reward funds the biggest claimers will have their share of rewards frozen.

I.e. if there be not enough jpeg in the strategy for any reason the claim will fail as it goes for the full amount each time.

Proof of Concept

Only full claim is now allowed in yVaultLPFarming:

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L132-L134

Recommended Mitigation Steps

Introduce an argument to perform the partial claim.

4. StrategyPUSDConvex withdraw withdrawAndUnwrap

Impact

User facing YVault withdraw will fail with low level lack of funds message if StrategyPUSDConvex fails to obtain enough funds as this isn't controlled.

For example, in case of underlying strategy having any liquidity issues the withdraw will fail without proper error even when there are enough funds due for withdrawal.

Proof of Concept

StrategyPUSDConvex's withdraw assumes that withdrawAndUnwrap will always obtain the _amount - balance needed:

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

yVault's withdraw also doesn't check how much was retrieved:

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

Recommended Mitigation Steps

Consider checking for the necessary amount obtained by withdrawAndUnwrap in StrategyPUSDConvex's withdraw and failing with clear error if there is not enough funds available at the moment to fulfil the request