Juicebox V2 contest - hake's results

Lines of code

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

Vulnerability details

Impact

Oracle my return stale price.

Proof of Concept

Round completeness and the quoted timestamp are not checked to ensure that the reported price is not stale. roundId, startedAt, updatedAt, and answeredInRound are omitted from the return result of feed.latestRoundData().

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

 (, int256 _price, , , ) = feed.latestRoundData();

Tools Used

Manual Review

Recommended Mitigation Steps

(uint80 roundId, int256 price, , uint256 updatedAt, uint80 answeredInRound) = feed.latestRoundData();
require(answeredInRound >= roundId, "ChainLink: Stale price");
require(updatedAt != 0, "ChainLink: Round not complete");

Check return values of answeredInRound, roundId and updatedAt to ensure price is not stale.

Lines of code

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

Vulnerability details

Impact

migrate() might silently fail on ETH/ERC20 transfer.

Proof of Concept

As outlined in another finding, JBERC20PaymentTerminal._transferFrom() is unsafe and might not revert on failure. Potentially leading to migrate() being "successful" even though no assets were transferred.

_to.addToBalanceOf() doesnt check return value of ETH _payableValue transfer. If the new addToBalanceOf() implementation has some error the transaction won't revert and still be recorded as succesful.

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

 // If this terminal's token is ETH, send it in msg.value.
      uint256 _payableValue = token == JBTokens.ETH ? balance : 0;

Tools Used

Manual Review

Recommended Mitigation Steps

Use safeTransfer()/safeTransferFrom() for ERC20 transfers Use call to transfer ETH to another address and check it's return value.

Other instances of this issue are present in (this is not an all-inclusive list): https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1058 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1232

Lines of code

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

Vulnerability details

Impact

Projects using fee-on-transfer tokens will have incorrect accounting and will receive fewer tokens than expected leading up to loss of funds.

Proof of Concept

pay() and _pay() rely on user input for accounting _amount being transferred. If token has a fee-on-transfer this amount will be inaccurate and lead to mintTokensOf() minting the wrong amount as well several other functions transferring the wrong values and emitting events with false values.

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

  function pay(
    uint256 _projectId,
    uint256 _amount,
    address _token,
    address _beneficiary,
    uint256 _minReturnedTokens,
    bool _preferClaimedTokens,
    string calldata _memo,
    bytes calldata _metadata
  ) external payable virtual override isTerminalOf(_projectId) returns (uint256) {
    _token; // Prevents unused var compiler and natspec complaints.

    // ETH shouldn't be sent if this terminal's token isn't ETH.
    if (token != JBTokens.ETH) {
      if (msg.value > 0) revert NO_MSG_VALUE_ALLOWED();

      // Transfer tokens to this terminal from the msg sender.
      _transferFrom(msg.sender, payable(address(this)), _amount);
    }
    // If this terminal's token is ETH, override _amount with msg.value.
    else _amount = msg.value;

    return
      _pay(
        _amount,
        msg.sender,
        _projectId,
        _beneficiary,
        _minReturnedTokens,
        _preferClaimedTokens,
        _memo,
        _metadata
      );
  }

Other functions affected: _distributeToPayoutSplitsOf() _processFee()

Tools Used

Manual Review

Recommended Mitigation Steps

Use the difference between the contract's balance before and after the transfer to account for the right amount of tokens.

Lines of code

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

Vulnerability details

Impact

Payouts won't be able to be distributed if one of multiple beneficiaries decides to revert the transaction on recieval.

Proof of Concept

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

// If there's a beneficiary, send the funds directly to the beneficiary. Otherwise, send to the msg.sender.
          _transferFrom(
            address(this),
            _split.beneficiary != address(0) ? _split.beneficiary : payable(msg.sender),
            _netPayoutAmount
          );

If token used is native ETH or ERC777 a beneficiary can revert the transaction on the callback and DOS _distributeToPayoutSplitsOf() for all the other beneficiaries.

Tools Used

Manual Review

Recommended Mitigation Steps

Have beneficiaries withdraw their benefit instead of sending it to them.

QA Report

[L-01] Missing zero address check

If projects is mistakenly set to zero the whole contract would have to be redeployed.

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBTokenStore.sol#L165 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L126 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBDirectory.sol#L189 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L380 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBChainlinkV3PriceFeed.sol#L61

[N-01] Improve comments clarity

Mappings documentation could be made clearer bby adopting the following representation: _projectId => _domain => _group => countOf https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L46-L57

[N-02] ERC20 _to doesnt need to be payable

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

[N-03] Outdated compiler version

The specified compiler version pragma solidity 0.8.6 is not up to date. Older compilers might be susceptible to bugs. List of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo

Gas Report

[G-01] for loop optimisation

for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
      uint256 _permissionIndex = _permissionIndexes[_i];

      if (_permissionIndex > 255) revert PERMISSION_INDEX_OUT_OF_BOUNDS();

      if (((permissionsOf[_operator][_account][_domain] >> _permissionIndex) & 1) == 0)
        return false;
    }

Gas could be saved by:

• Not initializing variable to default value of zero (in some instances)
• Caching array length
• Using a prefix (++i) instead of a postfix (i++)
• Unchecking increment count

Example:

uint lenght = _permissionIndexes.length;

for (uint256 _i = 0; _i < length;) {
      uint256 _permissionIndex = _permissionIndexes[_i];

      if (_permissionIndex > 255) revert PERMISSION_INDEX_OUT_OF_BOUNDS();

      if (((permissionsOf[_operator][_account][_domain] >> _permissionIndex) & 1) == 0)
        return false;
			unchecked { ++_i; }
    }

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSingleTokenPaymentTerminalStore.sol#L862 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBOperatorStore.sol#L85 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1008

[G-02] Unused Variable

As outlined _token variable is not used and should be removed. https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L335