Juicebox V2 contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

Chainlink's latestRoundData might return stale or incorrect results

Proof of Concept

According to Chainlink documentation, there must be a check for stale prices.

It's a link of the same issue.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

currentPrice() function should be modified like below. It's modified according to this one.

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

  require(
      timeStamp != 0,
      “ChainlinkOracle::getLatestAnswer: round is not complete”
  );
  require(
      answeredInRound >= roundID,
      “ChainlinkOracle::getLatestAnswer: stale data”
  );

  // 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);
}

Lines of code

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

Vulnerability details

Impact

ERC20.approve() missing return value check. Some tokens do not revert if the approval failed but return false instead. Tokens that don't perform the approve and return false are still counted as a correct approve.

Proof of Concept

The _beforeTransferTo() function performs an ERC20.approve() call but does not check the success return value.

There is the same issue for a past contest. https://github.com/code-423n4/2021-08-notional-findings/issues/67

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeIncreaseAllowance(), safeDecreaseAllowance() functions.