JPEG'd contest - hickuphh3's results

Lines of code

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

Vulnerability details

Details

The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”.

Proof of Concept

• Attacker deposits 1 wei to mint 1 share
• Attacker transfers exorbitant amount to the StrategyPUSDConvex contract to greatly inflate the share’s price. Note that the strategy deposits its entire balance into Convex when its deposit() function is called.
• Subsequent depositors instead have to deposit an equivalent sum to avoid minting 0 shares. Otherwise, their deposits accrue to the attacker who holds the only share.

Insert this test into yVault.ts.

it.only("will cause 0 share issuance", async () => {
  // mint 10k + 1 wei tokens to user1
  // mint 10k tokens to owner
  let depositAmount = units(10_000);
  await token.mint(user1.address, depositAmount.add(1));
  await token.mint(owner.address, depositAmount);
  // token approval to yVault
  await token.connect(user1).approve(yVault.address, 1);
  await token.connect(owner).approve(yVault.address, depositAmount);
  
  // 1. user1 mints 1 wei = 1 share
  await yVault.connect(user1).deposit(1);
  
  // 2. do huge transfer of 10k to strategy
  // to greatly inflate share price (1 share = 10k + 1 wei)
  await token.connect(user1).transfer(strategy.address, depositAmount);
  
  // 3. owner deposits 10k
  await yVault.connect(owner).deposit(depositAmount);
  // receives 0 shares in return
  expect(await yVault.balanceOf(owner.address)).to.equal(0);

  // user1 withdraws both his and owner's deposits
  // total amt: 20k + 1 wei
  await expect(() => yVault.connect(user1).withdrawAll())
    .to.changeTokenBalance(token, user1, depositAmount.mul(2).add(1));
});

Recommended Mitigation Steps

• Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when totalSupply() == 0, send the first min liquidity LP tokens to the zero address to enable share dilution.
• Ensure the number of shares to be minted is non-zero: require(_shares != 0, "zero shares minted");

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L375 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L54-L62

Vulnerability details

Details & Impact

A user’s JPEG lock schedule can be overwritten by another user’s if he (the other user) submits and finalizes a proposal to change the same NFT index’s value.

The existing user will be unable to withdraw his locked JPEGs, resulting in permanent lock up of JPEG in the locker contract.

Proof of Concept

1. user successfully proposes and finalizes a proposal to change his NFT’s collateral value
2. Another user (owner) does the same for the same NFT index
3. user will be unable to withdraw his locked JPEG because schedule has been overwritten

Insert this test case into NFTVault.ts.

it.only("will overwrite existing user's JPEG lock schedule", async () => {
  // 0. setup
  const index = 7000;
  await erc721.mint(user.address, index);
  await nftVault
    .connect(dao)
    .setPendingNFTValueETH(index, units(50));
  await jpeg.transfer(user.address, units(150000));
  await jpeg.connect(user).approve(locker.address, units(500000));
  await jpeg.connect(owner).approve(locker.address, units(500000));

  // 1. user has JPEG locked for finalization
  await nftVault.connect(user).finalizePendingNFTValueETH(index);

  // 2. owner submit proposal to further increase NFT value
  await nftVault
    .connect(dao)
    .setPendingNFTValueETH(index, units(100));
  
  // 3. owner finalizes, has JPEG locked
  await nftVault.connect(owner).finalizePendingNFTValueETH(index);

  // user schedule has been overwritten
  let schedule = await locker.positions(index);
  expect(schedule.owner).to.equal(owner.address);

  // user tries to unstake
  // wont be able to because schedule was overwritten
  await timeTravel(days(366));
  await expect(locker.connect(user).unlock(index)).to.be.revertedWith("unauthorized");
});

Recommended Mitigation Steps

1. Release the tokens of the existing schedule. Simple and elegant.

// in JPEGLock#lockFor()
LockPosition memory existingPosition = positions[_nftIndex];
if (existingPosition.owner != address(0)) {
  // release jpegs to existing owner
  jpeg.safeTransfer(existingPosition.owner, existingPosition.lockAmount);
}

2. Revert in finalizePendingNFTValueETH() there is an existing lock schedule. This is less desirable IMO, as there is a use-case for increasing / decreasing the NFT value.

Lines of code

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

Vulnerability details

Details & Impact

yVault users participating in the farm have to trust that:

• vault.balanceOfJPEG() returns the correct claimable JPEG amount by its strategy / strategies
• the strategy / strategies will send all claimable JPEG to the farm

Should either of these assumptions break, then it could be possibly be the case that currentBalance is less than previousBalance, causing deposits and crucially, withdrawals to fail due to subtraction overflow.

Proof of Concept

For instance,

• Farm migration occurs. A new farm is set in yVault, then withdrawJPEG() is called, which sends funds to the new farm. Users of the old farm would be unable to withdraw their deposits.

it.only("will revert old farms' deposits and withdrawals if yVault migrates farm", async () => {
  // 0. setup
  await token.mint(owner.address, units(1000));
  await token.approve(yVault.address, units(1000));
  await yVault.depositAll();
  await yVault.approve(lpFarming.address, units(1000));
  // send some JPEG to strategy prior to deposit
  await jpeg.mint(strategy.address, units(100));
  // deposit twice, so that the second deposit will invoke _update()
  await lpFarming.deposit(units(250));
  await lpFarming.deposit(units(250));
	
  // 1. change farm and call withdrawJPEG()
  await yVault.setFarmingPool(user1.address);
  await yVault.withdrawJPEG();
	
  // deposit and withdrawal will fail
  await expect(lpFarming.deposit(units(500))).to.be.revertedWith('reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)');
  await expect(lpFarming.withdraw(units(500))).to.be.revertedWith('reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)');
});

• Strategy migration occurs, but JPEG funds held by the old strategy were not claimed, causing vault.balanceOfJPEG() to report a smaller amount than previously recorded
• jpeg could be accidentally included in the StrategyConfig, resulting in JPEG being converted to other assets
• A future implementation takes a fee on the jpeg to be claimed

Recommended Mitigation Steps

A simple fix would be to return if currentBalance ≤ previousBalance. A full fix would properly handle potential shortfall.

if (currentBalance <= previousBalance) return;

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#L266

Vulnerability details

Details

The controller calls the withdraw() method to withdraw JPEGs from the contract, but the strategy might blacklist the JPEG asset, which is what the PUSDConvex strategy has done.

The migration would therefore revert.

Proof of Concept

Insert this test into StrategyPUSDConvex.ts.

it.only("will revert when attempting to migrate strategy", async () => {
  await controller.setVault(want.address, yVault.address);
  await expect(controller.setStrategy(want.address, strategy.address)).to.be.revertedWith("jpeg");
});

Recommended Mitigation Steps

Replace _current.withdraw(address(jpeg)); with _current.withdrawJPEG(vaults[_token]).

Lines of code

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

Vulnerability details

Details & Impact

The deprecated latestAnswer() API is being used, which may at any time fail to work if Chainlink ends support for it.

In addition, the data freshness should be checked. The oracle could, for example, not have been updated in a while, causing outdated prices to be used. This would result in improper collateral valuations and cause systemic risk.

Recommended Mitigation Steps

Switch to latestRoundData() checks on the return data with proper revert messages if the price is stale or the round is incomplete.

(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = oracle.latestRoundData();
require(price > 0, "invalid_oracle_answer"); 
require(answeredInRound >= roundID, "...");
// eg. price must have been last updated within past hour
require(timeStamp >= block.timestamp - 1 hour, "outdated_price");

Codebase Impressions & Summary

The JPEG’d contracts are in general of high quality. The core feature is the issuance of a stablecoin PUSD that is backed by NFTs, whose collateral pricing is determined by the DAO / submission of governance proposals. Supporting features are staking and farming JPEG tokens, and a convex autocompounding strategy for the PUSD/USDC/USDT/MIM curve pool.

A noteworthy mechanism is the support for non-ERC721 NFTs like CryptoPunks and EtherRocks. By combining the deterministic generation of contract addresses with the selfdestruct opcode, the same address can be used for multiple deployments of new code. One concern for this is that EIP-4758 may see the deprecation of the selfdestruct opcode, causing this mechanism to break.

Supporting documentation and inline comments were adequate in helping understand the contracts’ functionalities.

An area of improvement is to have more integration tests. For example, testing the yVault contract with the non-ERC721 NFTs and flash escrow mechanism would help to ensure things work as expected. Also, a high severity bug on the failure of strategy migration would have been caught if it was tested with the actual PUSDConvex strategy being used instead of a mock strategy.

Nevertheless, test coverage yielded close to 100% coverage for most contracts, which is commendable.

Low Severity Findings

L01: Stablecoin: Unpausing should be done by admin instead of pauser

Line References

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L68

Description

For better security, the resumption of functionality (unpausing) should be performed only by the admin (DAO).

Recommended Mitigation Steps

Change PAUSER_ROLE to DEFAULT_ADMIN_ROLE.

hasRole(DEFAULT_ADMIN_ROLE, _msgSender()),
"StableCoin: only admin can unpause"

L02: FungibleAssetVaultForDAO: Use call() instead of transfer() for native asset withdrawal

Line References

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

Description

It is recommended to use call() instead of transfer() because the former fowards all remaining gas with the call, while the latter has a gas limit of 2300. This would cause withdrawals to fail if the caller is a smart contract that:

• lacks receive() / payable fallback() functions
• implements a payable fallback function which uses more than 2300 gas
• implements a payable fallback function which uses less than 2300 gas, but is called through a proxy that raise the call's gas usage to above 2300

Recommended Mitigation Steps

(bool success, ) = payable(msg.sender).call{ value: amount }("");
require(success, "ETH_transfer_failed");

L03: StrategyPUSDConvex: Improve correctness of curve config

Description

I recommend checking that the curve config indexes correspond to the token addresses because it verifies the correctness of the following parameters:

1. The curve pool address _curveConfig.curve
2. The curve config index _curveConfig.pusdIndex and _curveConfig.usdcIndex
3. The USDC and PUSD token addresses

Recommended Mitigation Steps

Add these checks in the constructor.

require(_curveConfig.curve.coins(_curveConfig.pusdIndex) == pusd, "invalid_pusd_config");
require(_curveConfig.curve.coins(_curveConfig.usdcIndex) == usdc, "invalid_pusd_config");

These existing checks can be removed because they are implicitly checked.

// zero address checks
require(_pusd != address(0), "INVALID_PUSD");
require(_usdc != address(0), "INVALID_USDC");
require(address(_curveConfig.curve) != address(0), "INVALID_CURVE");

// config checks
require(
  _curveConfig.pusdIndex != _curveConfig.usdcIndex,
  "INVALID_CURVE_INDEXES"
);
require(_curveConfig.pusdIndex < 4, "INVALID_PUSD_CURVE_INDEX");
require(_curveConfig.usdcIndex < 4, "INVALID_USDC_CURVE_INDEX");

L04: StrategyPUSDConvex: Derive want from curve pool

Description

In a similar vein to L03, to reduce human error, it would be better to derive the want address (PUSD/USDC/USDT/MIM Curve LP token) directly from the curve pool.

Recommended Mitigation Steps

want = IERC20(_curveConfig.curve.lpToken());

L05: StrategyPUSDConvex: Ensure Strategy config reward tokens are not JPEG

Line References

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

Description

It is possible for JPEG to be accidentally added as a reward token. This means that JPEG would have to be converted to USDC / PUSD as well, potentially bricking yVaultLPFarming's functionality.

Recommended Mitigation Steps

Add the following check.

require(
  address(_strategyConfig.rewardTokens[i]) != _jpeg,
  "INVALID_REWARD_TOKEN"
);

L06: StrategyPUSDConvex: Out of place comment

Line Reference

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

Description

The comment seems to be outdated and should be removed.

L07: FlashEscrow: selfdestruct opcode may be deprecated in the future

References

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

https://eips.ethereum.org/EIPS/eip-4758

https://ethereum.stackexchange.com/questions/76116/can-you-selfdestruct-a-contract-more-than-once

Description

The FlashEscrow self-destructs once the call is executed. In tandem with a deterministic way of precomputing the contract address, it would allow new / the same code to be deployed at the same address. Unfortunately, as outlined in EIP-4758, the selfdestruct opcode may be deprecated in the future, causing this mechanism to be incompatible with the change.

Recommended Mitigation Steps

Avoid the use of selfdestruct. This however means that you would have to introduce more entropy to the salt used for pre-computation of the flash escrow contract.

Non-Critical Findings

NC01: Typo

contrat → contract

NC02: Update LPFarming Natspec

Description

The README notes that the natspec is incorrect for the LPFarming’s newEpoch() function. Do remember to update it.

Suggestion

S01: Make yVault contract ERC4626 Compliant

Description

The EIP4626 standard has just been finalised. I recommend making the yVault contract compliant to the standard. As our favourite optimisoooor @t11s succinctly puts it, “there are a terrifying amount of pitfalls that you can run into when writing a vault from scratch, i learned this first hand working on them at rari. If you want to sleep at night, skip that bs and use 4626”.

The other reason why you would want to be ERC4626 compliant is for composability. To quote yearn, “build an app on top of one ERC-4626 vault, and it will work for all other ERC-4626 tokens. Contributors are already working hard implementing the standard for Yearn’s V3 vaults. So are devs at @AlchemixFi, @balancer, @RariCapital, @feiprotocol, @OpenZeppelin and elsewhere.”

References

Twitter thread by @t11s shilling EIP4626

Solmate’s base implementation

Twitter thread by Yearn

G01: Set position’s borrowType earlier for NOT_CONFIRMED case

Line References

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

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

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

Description

A couple of conditions can be removed by setting the position’s borrowType earlier for a new position.

• The "invalid_insurance_mode" can remove the position.borrowType == BorrowType.NOT_CONFIRMED case
• Either of the cases in L719 can be removed, preferably the former position.borrowType == BorrowType.USE_INSURANCE since _useInsurance will definitely be equal to the USE_INSURANCE borrow type.

In fact, consider passing the _useInsurance parameter in the _openPosition() function to directly set the borrow type.

Recommended Mitigation Steps

// _openPosition()
positions[_nftIndex] = Position({
  borrowType: _useInsurance ? _BorrowType.USE_INSURANCE : BorrowType.NON_INSURANCE,
  ...
});

// borrow()
if (positionOwner[_nftIndex] == address(0)) {
  _openPosition(msg.sender, _nftIndex, _useInsurance);
}
...
// removed position.borrowType == BorrowType.NOT_CONFIRMED
require(
  (position.borrowType == BorrowType.USE_INSURANCE &&
    _useInsurance) ||
  (position.borrowType == BorrowType.NON_INSURANCE &&
    !_useInsurance),
  "invalid_insurance_mode"
);
...
// if the position is insured, calculate the insurance fee
if (_useInsurance) { // removed position.borrowType == BorrowType.USE_INSURANCE

G02: Combine organizationFee in feeAmount

Line References

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

Description

organizationFee can be directly incorporated into feeAmount, just like how the insurance fee is.

// calculate the borrow (organization) fee
uint256 feeAmount = (_amount *
  settings.organizationFeeRate.numerator) /
  settings.organizationFeeRate.denominator;

G03: Unnecessary uint256 casting

Line References

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

Description

liquidityAmounts is already a uint256 array; the uint256 casting of 0 is redundant;

Recommended Mitigation Steps

uint256[4] memory liquidityAmounts = [0, 0, 0, 0];

G04: Remove == true comparison in setStrategy()

Line References

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

Description

Since the approvedStrategies[_token][_strategy] mapping evaluates to a boolean, its comparison against true is redundant.

Recommended Mitigation Steps

require(
	approvedStrategies[_token][_strategy] == true,
	"STRATEGY_NOT_APPROVED"
);