Juicebox V2 contest - svskaushik's results

Lines of code

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

Vulnerability details

Description

JBPayoutRedemptionPaymentTerminal is an abstract contract that places the onus of handling token transfer logic on it's child contracts. One such child contract is JBERC20PaymentTerminal. However, JBERC20PaymentTerminal implements _transferFrom logic in such a manner that it ignores return value by IERC20 tokens. Lack of return value check is error-prone and can lead to unexpected results for projects that receive funding denominated in certain types of ERC20 tokens.

The code in referred to is below:

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

This _transferFrom implementation does not check for return values and hence can lead to errors when used with tokens that don't revert on transfer but just return false. ZRX and BAT are examples. If such a token is used, it would be possible to successfully call functions like pay and addToBalanceOf even when the token returns false. This would allow an attacker to manipulate funds even with insufficient balances.

Mitigation Steps

Using SafeERC20's safeTransferFrom or implementing manual logic to check return value in JBPayoutRedemptionPaymentTerminal's child contracts are possible solutions.

Irregular code practices throughout the codebase

There is a variety of styles and practices adhered to in different patches throughout the code. Such variations can affect code readability and perceived code quality. For instance, the implementation of FOR loops is done in the following ways:

JBController.sol:L913

    for (uint256 _i = 0; _i < _splits.length; _i++) 

uint is initialized with 0 value

JBController.sol:L1014

    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) 

uint is not initialized with a value, default value is used

JBPayoutRedemptionPaymentTerminal.sol:L591-594

    // Push array length in stack
    uint256 _heldFeeLength = _heldFees.length;

    // Process each fee.
    for (uint256 _i = 0; _i < _heldFeeLength; ) 

Array length is cached

JBController.sol:L913

    for (uint256 _i = 0; _i < _splits.length; _i++) 

Array length is not cached.

JBPayoutRedemptionPaymentTerminal.sol:L591-594

    // Push array length in stack
    uint256 _heldFeeLength = _heldFees.length;

    // Process each fee.
    for (uint256 _i = 0; _i < _heldFeeLength; ) 

++i and unchecked{} is utilized later in L607:

      unchecked {
        ++_i;
      }

JBController.sol:L913

    for (uint256 _i = 0; _i < _splits.length; _i++) 

No utilization of ++i and unchecked{}