JPEG'd contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

The first depositor into yVault is able to maliciously manipulate the share price by depositing the lowest possible amount (1 wei) and then artificially blowing up the yVault token balance. Following depositors will loose their deposited funds due to precisions issues.

This is a well known issue, Uniswap and other protocols had similar issues when supply == 0.

Proof of Concept

1. Alice (attacker) deposits 1 wei via yVault.deposit()
2. Alice receives 1e-18 yVault shares
3. Alice transfers 1 ether token via transfer() to yVault to artificially blow up balance without minting new shares. yVault balance is now 1 ether + 1 wei -> share price is now very high (= 1000000000000000000001 wei ~ 1000 * 1e18)
4. Bob (victim) deposits 100 ether via yVault.deposit()
5. Bob receives 0 yVault shares

Bob receives 0 shares due to precision issue. His deposited funds are lost.

The formula shares = (_amount * supply) / balanceBefore; calculates in case of such a high share price the following:

shares = (500000000000000000000 * 1) / 1000000000000000000001 => shares = 0

This is due to balanceBefore > (_amount * supply).

Following test verifies this issue:

it.only("should mint the correct amount of tokens for very high share price", async () => {
  expect(await yVault.getPricePerFullShare()).to.equal(0);
  await token.mint(user1.address, units(1001));
  await token.mint(user2.address, units(1000));
  await token.connect(user1).approve(yVault.address, units(1000));
  await token.connect(user2).approve(yVault.address, units(1000));

  await yVault.connect(user1).deposit(1); // @audit-info deposit `1 wei` token
  expect(await yVault.balanceOf(user1.address)).to.equal(1);
  await token.connect(user1).transfer(yVault.address, units(1000)); // @audit-info transfer tokens to vault to artificially increase balance
  expect(await token.balanceOf(yVault.address)).to.equal(units(1000).add(1));

  await yVault.connect(user2).deposit(units(500));

  expect(await yVault.balanceOf(user2.address)).not.to.equal(0); // @audit-info FAILS, user 2 should receive shares but does _not_. tokens deposited by user 2 are now lost
});

Fails as user2 did receive 0 yVault shares.

Tools Used

Manual review

Recommended mitigation steps

For the first deposit, mint fixed amount of shares, e.g. 10**decimals()

if (supply == 0) {
    shares = 10**decimals(); // @audit-info recommended mitigation
} else {
    //balanceBefore can't be 0 if totalSupply is > 0
    shares = (_amount * supply) / balanceBefore;
}

Following test verifies this fix:

it.only("should mint the correct amount of tokens for very high share price (fixed)", async () => {
  expect(await yVault.getPricePerFullShare()).to.equal(0);
  await token.mint(user1.address, units(1001));
  await token.mint(user2.address, units(1000));
  await token.connect(user1).approve(yVault.address, units(1000));
  await token.connect(user2).approve(yVault.address, units(1000));

  await yVault.connect(user1).deposit(1);
  expect(await yVault.balanceOf(user1.address)).to.equal(units(1)); // @audit-info first depositor receives fixed amount of shares

  await token.connect(user1).transfer(yVault.address, units(1000)); // @audit-info send token to vault to artificially increase balance
  expect(await token.balanceOf(yVault.address)).to.equal(units(1000).add(1));

  await yVault.connect(user2).deposit(units(500));

  expect(await yVault.balanceOf(user2.address)).not.to.equal(0); // @audit-info user 2 receives shares
});

Fix inspired by Uniswap V2 and Harvest Finance

Extra caution has to be put on initial deployment and on the first deposit into yVault. It's even recommended for the protocol owner to provide the initial deposit.

Lines of code

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

Vulnerability details

The yVault.getPricePerFullShare() function calculates the price per share by multiplying with 1e18 token decimals with the assumption that the underlying token always has 18 decimals. yVault has the same amount of decimals as it's underlying token see (yVault.decimals())

But tokens don't always have 1e18 decimals (e.g. USDC).

Impact

The price per share calculation does not return the correct price for underlying tokens that do not have 18 decimals. This could lead to paying out too little or too much and therefore to a loss for either the protocol or the user.

Proof of Concept

Following test will fail with the current implementation when the underlying vault token has 6 decimals:

NOTE: units() helper function was adapted to accept the desired decimals.

it.only("should mint the correct amount of tokens for tokens with 6 decimals", async () => {
  const DECIMALS = 6;

  await token.setDecimals(DECIMALS);
  expect(await yVault.decimals()).to.equal(DECIMALS);

  expect(await yVault.getPricePerFullShare()).to.equal(0);
  await token.mint(user1.address, units(1000, DECIMALS));
  await token.connect(user1).approve(yVault.address, units(1000, DECIMALS));

  await yVault.connect(user1).deposit(units(500, DECIMALS));
  expect(await yVault.balanceOf(user1.address)).to.equal(units(500, DECIMALS));

  await token.mint(strategy.address, units(500, DECIMALS));
  expect(await yVault.getPricePerFullShare()).to.equal(units(2, DECIMALS));
});

Fails with following error: AssertionError: Expected "2000000000000000000" to be equal 2000000

Tools Used

Manual review

Recommended mitigation steps

Use vault decimals() instead of hardcoded 1e18 decimals.

function getPricePerFullShare() external view returns (uint256) {
    uint256 supply = totalSupply();
    if (supply == 0) return 0;
    return (balance() * (10**decimals())) / supply; // @audit-info use `decimals()` instead of hardcoded `1e18`
}

Lines of code

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

Vulnerability details

Impact

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

The function is not present in the latest API reference (AggregatorInterfaceV3).

Proof of Concept

vaults/NFTVault._normalizeAggregatorAnswer()
vaults/FungibleAssetVaultForDAO._collateralPriceUsd()

Tools Used

Manual review

Recommended mitigation steps

Use the latestRoundData function to get the price instead. Add checks on the return data with proper revert messages if the price is stale or the round is incomplete, for example:

(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = oracle.latestRoundData();
require(answeredInRound >= roundID, "...");
require(timeStamp != 0, "...");

QA Report

Table of Contents

• Non-Critical Findings
  • NC-01: Boolean constants can be used directly and do not need to be compare to true or false
• Low Risk
  • L-01: Zero-address check is missing
  • L-02: Events not emitted for important state changes
  • L-03: Divide before multiple can lead to precision errors
  • L-04: Validate input
  • L-05: Unsafe call to ERC20 decimals()

Non-Critical Findings

NC-01: Boolean constants can be used directly and do not need to be compare to true or false

Description

Boolean constants can be used directly and do not need to be compare to true or false.

Findings

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

Recommended mitigation steps

Remove the equality to the boolean constant.

Low Risk

L-01: Zero-address check is missing

Description

Zero-address checks are a best-practice for input validation of critical address parameters.

Findings

lock/JPEGLock.constructor()#_jpeg
vaults/FungibleAssetVaultForDAO.initialize()#_collateralAsset
vaults/FungibleAssetVaultForDAO.initialize()#_stablecoin
vaults/yVault/Controller.constructor()#_jpeg
vaults/yVault/yVault.constructor()#_token
vaults/NFTVault.initialize()
staking/JPEGStaking.initialize()#_jpeg
farming/LPFarming.constructor()#_jpeg
escrow/NFTEscrow.constructor()#target\

Recommended mitigation steps

Add zero-address check:

require(address(_nftLoanFacilitator) != address(0), "Zero address");

L-02: Events not emitted for important state changes

Description

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Findings

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L39\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L33\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L44\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L56\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L69\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L82\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L177\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L191\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L201\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L203 and all following state variable setter functions
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L65\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L96\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L141\ https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L159\

Recommended mitigation steps

Emit events for state variable changes.

L-03: Divide before multiple can lead to precision errors

Description

Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision. Slither explanation

Findings

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

Recommended mitigation steps

Consider ordering multiplication before division.

L-04: Validate input

Description

The _settings function input parameter struct is validated for invalid input, but the struct properties insuraceRepurchaseTimeLimit and borrowAmountCap are not validated.

It is especially important to validate insuraceRepurchaseTimeLimit as it can not be changed later on.

Note: There's a spelling mistake. Instead of insuraceRepurchaseTimeLimit it should be insuranceRepurchaseTimeLimit.

Findings

vaults/NFTVault.initialize()

Recommended mitigation steps

Validate input.

L-05: Unsafe call to ERC20 decimals()

Description

The ERC20 function decimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a valid concern to prevent future issues.

The calling function of the non-existent decimals() will immediately return.

Findings

vaults/yVault/yVault.decimals()
vaults/FungibleAssetVaultForDAO.initialize()

Recommended mitigation steps

Consider using a helper function to safely retrieve token decimals. E.g.:

/// @notice Provides a safe ERC20.decimals version which returns '18' as fallback value.
/// @param token The address of the ERC-20 token contract.
/// @return (uint8) Token decimals.
function tokenDecimals(address token) internal view returns (uint8) {
    // 0x313ce567 = bytes4(keccak256("decimals()"))
    (bool success, bytes memory data) = token.staticcall(abi.encodeWithSelector(0x313ce567));
    return success && data.length == 32 ? abi.decode(data, (uint8)) : 18;
}

Gas Optimizations

Table of Contents

• G-01: Public functions that could be declared external to save gas
• G-02: > 0 is less efficient than != 0 for unsigned integers
• G-03: An array's length should be cached to save gas in for-loops
• G-04: Use unchecked {} primitive within for loops

G-01: Public functions that could be declared external to save gas

Description

Following functions should be declared external, as functions that are never called by the contract internally should be declared external to save gas.

Findings

vaults/yVault/Controller.withdraw()
vaults/yVault/yVault.setFarmingPool()

Recommended mitigation steps

Use the external attribute for functions never called from the contract.

G-02: > 0 is less efficient than != 0 for unsigned integers

Description

!= 0 is a cheaper (less gas) operation for unsigned integers within require statements compared to > 0.

Findings

There are many occurrences of > 0 within require statements in the following contracts:

lock/JPEGLock.sol
vaults/FungibleAssetVaultForDAO.sol
vaults/yVault/strategies/StrategyPUSDConvex.sol
vaults/yVault/yVault.sol
vaults/NFTVault.sol
staking/JPEGStaking.sol
farming/yVaultLPFarming.sol
farming/LPFarming.sol\

Recommended mitigation steps

Change > 0 to != 0.

G-03: An array's length should be cached to save gas in for-loops

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.

Findings

vaults/yVault/strategies/StrategyPUSDConvex.sol#L145
vaults/yVault/strategies/StrategyPUSDConvex.sol#L319
vaults/NFTVault.sol#L181
vaults/NFTVault.sol#L184
farming/LPFarming.sol#L348\

Recommended mitigation steps

uint length = arr.length;

for (uint i; i < length; ++i) {
  // Operations not effecting the length of the array.
}

G-04: Use unchecked {} primitive within for loops

Given the use of Solidity compiler >= 0.8.0, there are default arithmetic checks for mathematical operations which consume additional gas for such checks internally. In expressions where we are absolutely sure of no overflows/underflows, one can use the unchecked primitive to wrap such expressions to avoid checks and save gas.

Recommended mitigation steps

Adapt foor loops to increase index variable within unchecked block. e.g

for (uint256 i = 0; i < length;) {
  // ...

  unchecked { ++i; }
}

Findings

vaults/yVault/strategies/StrategyPUSDConvex.sol#L145
vaults/yVault/strategies/StrategyPUSDConvex.sol#L231
vaults/yVault/strategies/StrategyPUSDConvex.sol#L319
vaults/NFTVault.sol#L181
vaults/NFTVault.sol#L184
farming/LPFarming.sol#L281
farming/LPFarming.sol#L348\