Juicebox V2 contest - cccz's results

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBChainlinkV3PriceFeed.sol#L42-L51

Vulnerability details

Impact

On JBChainlinkV3PriceFeed.sol, we are using latestRoundData, but there is no check if the return value indicates stale data.

  function currentPrice(uint256 _decimals) external view override returns (uint256) {
    // Get the latest round information. Only need the price is needed.
    (, int256 _price, , , ) = feed.latestRoundData();

    // Get a reference to the number of decimals the feed uses.
    uint256 _feedDecimals = feed.decimals();

    // Return the price, adjusted to the target decimals.
    return uint256(_price).adjustDecimals(_feedDecimals, _decimals);
  }

This could lead to stale prices according to the Chainlink documentation:

https://docs.chain.link/docs/historical-price-data/#historical-rounds https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Proof of Concept

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBChainlinkV3PriceFeed.sol#L42-L51 https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBPrices.sol#L69-L70 https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBPrices.sol#L76-L77

Tools Used

None

Recommended Mitigation Steps

  function currentPrice(uint256 _decimals) external view override returns (uint256) {
    // Get the latest round information. Only need the price is needed.
-    (, int256 _price, , , ) = feed.latestRoundData();
+    (uint80 roundID, int256 _price, , uint256 timestamp, uint80 answeredInRound) = feed.latestRoundData();
+    require(answeredInRound >= roundID, "Stale price");
+    require(timestamp != 0,"Round not complete");
+    require(_price > 0,"Chainlink answer reporting 0");

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L86-L88

Vulnerability details

Impact

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Proof of Concept

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L86-L88

Tools Used

None

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L349-L350

Vulnerability details

Impact

In the pay and addToBalanceOf functions of the JBPayoutRedemptionPaymentTerminal contract, if the token is a fee-on-transfer token, the actual number of tokens received by the contract will be less than _amount. This will make the amount recorded in the JBSingleTokenPaymentTerminalStore contract incorrect.

Proof of Concept

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L349-L350 https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L555-L556

Tools Used

None

Recommended Mitigation Steps

Consider getting the received amount by calculating the difference of token balance (using balanceOf) before and after the _transferFrom.

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1148-L1152

Vulnerability details

Impact

When the JBPayoutRedemptionPaymentTerminal contract distributes assets to _split, _transferFrom may be called to send tokens to _split.beneficiary. If the token is an ERC777 token, a malicious _split can revert the transaction in the tokensReceived function of _split.beneficiary, which prevents the contract from distributing assets.

Proof of Concept

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1148-L1152

Tools Used

None

Recommended Mitigation Steps

Consider letting _split withdraw the distributed assets by itself

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L588-L606

Vulnerability details

Impact

When the owner calls the changeTokenOf function of the JBController contract, the token corresponding to the current project will be changed, which will make the oldToken holder unable to redeem the overflowing assets.

Proof of Concept

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L588-L606 https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBTokenStore.sol#L236-L269

Tools Used

None

Recommended Mitigation Steps

Consider adding a delay to changeTokenOf, or adding a function to convert oldToken to newToken

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBToken.sol#L106-L114

Vulnerability details

Impact

The owner of the JBToken contract can mint arbitrary amount of tokens. In general, the owner of the JBToken contract is the JBTokenStore contract, and the minting of the tokens is controlled by the JBController contract, but when the changeTokenOf function of the JBController contract is called, the owner will be transferred to any address, which can mint arbitrary amount of tokens.

  function mint(
    uint256 _projectId,
    address _account,
    uint256 _amount
  ) external override onlyOwner {
    _projectId; // Prevents unused var compiler and natspec complaints.

    return _mint(_account, _amount);
  }

Proof of Concept

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBToken.sol#L106-L114

Tools Used

None

Recommended Mitigation Steps

Consider setting minter as the JBTokenStore contract and adding the onlyminter modifier to the mint function

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBToken.sol#L127-L135

Vulnerability details

Impact

The owner of the JBToken contract can burn anyone's tokens. In general, the owner of the JBToken contract is the JBTokenStore contract, and the burning of the tokens is controlled by the JBController contract, but when the changeTokenOf function of the JBController contract is called, the owner will be transferred to any address, which can burn anyone's tokens.

  function burn(
    uint256 _projectId,
    address _account,
    uint256 _amount
  ) external override onlyOwner {
    _projectId; // Prevents unused var compiler and natspec complaints.

    return _burn(_account, _amount);
  }

Proof of Concept

https://github.com/jbx-protocol/juice-contracts-v2-code4rena//blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBToken.sol#L127-L135

Tools Used

None

Recommended Mitigation Steps

Consider asking the user to approve the JBTokenStore contract to use his tokens before calling the JBController contract's burnTokensOf function, and only the token's owner and an approved address can burn his tokens