Juicebox V2 contest - hubble's results

Lines of code

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

Vulnerability details

Impact

There is no check if the value of _price returned by chainlink latestRoundData() is latest or stale. If stale price is returned, it may result in wrong calculation used further, and in JBPrices.sol

Proof of Concept

Contract : JBChainlinkV3PriceFeed.sol Line : 42

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

Recommended Mitigation Steps

As per chainlink's documentation, add relevant checks as below for handling return values of latestRoundData().

(uint80 roundID, int256 answer, , uint256 timeStamp, uint80 answeredInRound) = AggregatorV3Interface(feed).latestRoundData();
require(answer > 0, "Chainlink price <= 0"); 
require(answeredInRound >= roundID, "Stale answer");
require(timeStamp != 0, "Round not complete");

Lines of code

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

Vulnerability details

The function _distributePayoutsOf() has reentrancy vulnerability which is being called by the external function distributePayoutsOf() There are external calls in this function along with calls sending ETH

Impact

The contract JBPayoutRedemptionPaymentTerminal inherits ReentrancyGuard, however there is no use of nonReentrant modifier either in this abstract contract or the other contracts which are deriving this, viz., JBETHPaymentTerminal.sol or JBERC20PaymentTerminal.sol

Proof of Concept

Contract : JBPayoutRedemptionPaymentTerminal.sol Function : _distributePayoutsOf()

Recommended Mitigation Steps

Add nonReentrant modifier in the external function distributePayoutsOf()

Summary of Findings for Low / Non-Critical issues

• L-01 : use of tranfser and transferFrom in JBERC20PaymentTerminal.sol
• L-02 : missing nonReentrant modifier in function recordAddedBalanceFor()
• L-03 : Wrong event emit value for JBFundingCycleStore's configureFor function, when _mustStartAtOrAfter < block.timestamp

Details L-01

Title : use of tranfser and transferFrom in JBERC20PaymentTerminal.sol

The function _transferFrom is using transfer and transferFrom, and the return value is ignored. Failure to do so will cause silent failures of transfers and affect token accounting in contract. Also unsafe transfer might return false instead of reverting, in this case, ignoring return value leads to considering it successful.

Use safe versions of the transfer functions.

Proof of Concept

Contract : JBERC20PaymentTerminal.sol Function :

  function _transferFrom(
    address _from,
    address payable _to,
    uint256 _amount
  ) internal override {
    _from == address(this)
      ? IERC20(token).transfer(_to, _amount)
      : IERC20(token).transferFrom(_from, _to, _amount);
  }

Recommended Mitigation Steps

Use safeTransfer and safeTransferFrom functions for ERC20 transfers

Details L-02

Title : missing nonReentrant modifier in function recordAddedBalanceFor()

Impact

nonReentrant modifier is applied in other functions like recordPaymentFrom(), recordRedemptionFor(), recordDistributionFor(), recordUsedAllowanceOf(), recordMigration(). This modifier is missing in this function recordAddedBalanceFor(). For consistency and to avoid cross function reentrancy issue add the modifier to all relevant external/public functions.

Proof of Concept

Contract : JBSingleTokenPaymentTerminalStore.sol Function : recordAddedBalanceFor()

Recommended Mitigation Steps

Add nonReentrant modifier in this function also

Details L-03

Title : Wrong event emit value for JBFundingCycleStore's configureFor function, when _mustStartAtOrAfter < block.timestamp

Lines of code

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

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

Vulnerability details

As per line 340, When _mustStartAtOrAfter is less than block's timestamp, the block's timestamp or the relevant start time is stored in the configuration properties.

As per line 366, The event emitted has _mustStartAtOrAfter

Instead, it should have block's timestamp or the relevant start time.

Impact

This would effect the state of any external integrating system, existing or future, like Indexing server.

Proof of Concept

Tools Used

Manual review

Recommended Mitigation Steps

Get the startTime from _configureIntrinsicPropertiesFor, emit the relevant startTime