JPEG'd contest - cmichel's results

Lines of code

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

Vulnerability details

Impact

The yVault.deposit function mints initial shares equal to the deposited amount. The deposit / withdraw functions also use the balance(), which includes the contract balance token.balanceOf(address(this)), to compute the shares.

It's possible to increase the share price to very high amounts, such that a depositor mints zero shares for their deposit. They lose all funds and they can then be stolen by the attacker when they redeem their fair share of the strategy TVL.

POC

Assume the attacker is the first minter and balance() == 0 and token == USDC. Some victim V wants to deposit 1000.0 USDC = 1e9 USDC. The attacker frontruns the transaction and does the following:

• deposit(amount = 1). This amount is transferred and the attacker receives shares = _amount = 1 share. Then balance() == 1.
• The attacker now increases contract's token balance by directly transfering 1e9 USDC to the controller such that balance() = token.balanceOf(this) = 1e9 + 1.
• The victim deposit is now mined and their _amountToDeposit = 1e9. The shares computation is now shares = (_amount * supply) / balanceBefore = 1e9 * 1 / (1e9 + 1) = 0. They contributed 1e9 USDC to the strategy but receive zero shares.
• The attacker can now call withdraw(1) to burn their single share and receive the entire balance() as they redeem 100% of the share total supply: backingTokens = (balance() * _shares) / supply = balance() * 1 / 1 = 2e9 + 1
• They made 1e9 USDC in profit, the totalSupply() is back to zero and they can repeat this attack for the next depositor.

Recommended Mitigation Steps

The way UniswapV2 prevents this is by requiring a minimum deposit amount and sending 1000 initial shares to the zero address to make this attack more expensive. The same mitigation can be done here. Alternatively, scale the initial amount by a large value like 1e18: if (totalSupply() == 0) _shares = _amountToDeposit * 1e18.

Lines of code

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

Vulnerability details

Impact

In deposit, the balance is cached and then a token.transferFrom is triggered which can lead to exploits if the token is a token that gives control to the sender, like ERC777 tokens.

POC

Initial state: balance() = 1000, shares supply = 1000. Depositing 1000 amount should mint 1000 supply, but one can split the 1000 amounts into two 500 deposits and use re-entrancy to profit.

• Outer deposit(500): balanceBefore = 1000. Control is given to attacker ...
• Inner deposit(500): balanceBefore = 1000. shares = (_amount * supply) / balanceBefore = 500 * 1000 / 1000 = 500 shares are minted ...
• Outer deposit(500) continues with the mint: shares = (_amount * supply) / balanceBefore = 500 * 1500 / 1000 = 750 are minted.
• Withdrawing the 500 + 750 = 1250 shares via withdraw(1250), the attacker receives backingTokens = (balance() * _shares) / supply = 2000 * 1250 / 2250 = 1111.111111111. The attacker makes a profit of 1111 - 1000 = 111 tokens.
• They repeat the attack until the vault is drained.

Recommended Mitigation Steps

The safeTransferFrom should be the last call in deposit.

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L170 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/yVault.sol#L108

Vulnerability details

Impact

The accruals in yVaultLPFarming will fail if currentBalance < previousBalance in _computeUpdate.

currentBalance = vault.balanceOfJPEG() + jpeg.balanceOf(address(this));
uint256 newRewards = currentBalance - previousBalance;

No funds can be withdrawn anymore as the withdraw functions first trigger an _update.

The currentBalance < previousBalance case can, for example, be triggerd by decreasing the vault.balanceOfJPEG() due to calling yVault.setController:

function setController(address _controller) public onlyOwner {
    // @audit can reduce balanceofJpeg which breaks other masterchef contract
    require(_controller != address(0), "INVALID_CONTROLLER");
    controller = IController(_controller);
}

function balanceOfJPEG() external view returns (uint256) {
    // @audit new controller could return a smaller balance
    return controller.balanceOfJPEG(address(token));
}

Recommended Mitigation Steps

Setting a new controller on a vault must be done very carefully and requires a migration.

Lines of code

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

Vulnerability details

Impact

The setDebtInterestApr changes the debt interest rate without first accruing the debt. This means that the new debt interest rate is applied retroactively to the unaccrued period on next accrue() call.

It should never be applied retroactively to a previous time window as this is unfair & wrong. Borrowers can incur more debt than they should.

Recommended Mitigation Steps

Call accrue() first in setDebtInterestApr before setting the new settings.debtInterestApr.

Lines of code

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

Vulnerability details

Impact

The owner of an insured position that has been liquidated can claim back their NFT without paying back the debt by calling closePosition instead of repurchase.

function closePosition(uint256 _nftIndex)
    external
    // @audit NFT is still valid (ownerOf(nft) == this)
    validNFTIndex(_nftIndex)
{
    accrue();
    // @audit owner is still original owner (depositor)
    require(msg.sender == positionOwner[_nftIndex], "unauthorized");
    // @audit debt is zero as it's been repaid by liquidator
    require(_getDebtAmount(_nftIndex) == 0, "position_not_repaid");

    positionOwner[_nftIndex] = address(0);
    delete positions[_nftIndex];
    positionIndexes.remove(_nftIndex);

    // transfer nft back to owner if nft was deposited
    if (nftContract.ownerOf(_nftIndex) == address(this)) {
        nftContract.safeTransferFrom(address(this), msg.sender, _nftIndex);
    }

    emit PositionClosed(msg.sender, _nftIndex);
}

The liquidate function sets position.debtPortion = 0; and does not clear the owner which means all closePosition checks pass. This means that PUSD can be minted for free by:

• Attacker deposits NFT collateral in insured state and borrow PUSD against it
• Let it be liquidated (the liquidator unexpectedly loses as they cannot get the NFT through claimExpiredInsuranceNFT anymore)
• Attacker claims it back through closePosition
• Repeat

Recommended Mitigation Steps

Consider checking in closePosition that the position has not been liquidated.