Juicebox V2 contest - Limbooo's results

Lines of code

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

Vulnerability details

In JBERC20PaymentTerminal.sol#_transferFrom(...) ignoring return value when IERC20(token).transferFrom(...) Some of ERC20 token implementations return false upon unsuccess transfer like USDT token, this can lead to some unpredictable balances to rise up without actual success transfer.

PoC

JBERC20PaymentTerminal.sol#_transferFrom(...)

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

Recommendation

Do not ignore return value, or use Safe Transfer Library.

GAS

List of contents:

• Setting uint256 variables to 0 is redundant.
• State variables should be cached in stack variables rather than re-reading them from storage.
• <array>.length should not be looked up in every loop of a for-loop.
• Functions guaranteed to revert when called by normal users can be marked payable.

1. Setting uint256 variables to 0 is redundant

It costs more gas to initialize variables to zero than to let the default of zero be applied

(10) instances

contracts/JBSingleTokenPaymentTerminalStore.sol:
  862:     for (uint256 _i = 0; _i < _terminals.length; _i++)

contracts/JBSplitsStore.sol:
  165:     for (uint256 _i = 0; _i < _groupedSplitsLength; ) {
  204:     for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
  211:       for (uint256 _j = 0; _j < _splits.length; _j++) {
  227:     uint256 _percentTotal = 0;
  229:     for (uint256 _i = 0; _i < _splits.length; _i++) {
  304:     for (uint256 _i = 0; _i < _splitCount; _i++) {

contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:
   594:     for (uint256 _i = 0; _i < _heldFeeLength; ) {
  1008:     for (uint256 _i = 0; _i < _splits.length; ) {
  1396:     for (uint256 _i = 0; _i < _heldFeesLength; ) {

Recommendation

don't assign zero, use declaration only uint256 _i.

2. State variables should be cached in stack variables rather than re-reading them from storage

Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read.

(5) instances

contracts/JBDirectory.sol:
  133      if (
  134:       _primaryTerminalOf[_projectId][_token] != IJBPaymentTerminal(address(0)) &&
  135:       isTerminalOf(_projectId, _primaryTerminalOf[_projectId][_token])
  136:     ) return _primaryTerminalOf[_projectId][_token];

contracts/JBReconfigurationBufferBallot.sol:
  72      // If there is a finalized state, return it.
  73:     if (finalState[_projectId][_configured] != JBBallotState.Active)
  74        return finalState[_projectId][_configured];

contracts/JBSingleTokenPaymentTerminalStore.sol:
  372      // Add the amount to the token balance of the project.
  373:     balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] =
  374:       balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] +
  375        _amount.value;

  513      // The amount being reclaimed must be within the project's balance.
  514:     if (reclaimAmount > balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId])
  515        revert INADEQUATE_PAYMENT_TERMINAL_STORE_BALANCE();

  518      if (reclaimAmount > 0)
  519:       balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] =
  520:         balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] -
  521          reclaimAmount;

  588      // The amount being distributed must be available.
  589:     if (distributedAmount > balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId])
  590        revert INADEQUATE_PAYMENT_TERMINAL_STORE_BALANCE();

  597      // Removed the distributed funds from the project's token balance.
  598:     balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] =
  599:       balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] -
  600        distributedAmount;

  680      // Update the project's balance.
  681:     balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] =
  682:       balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] -
  683        usedAmount;

  697      // Increment the balance.
  698:     balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] =
  699:       balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] +
  700        _amount;

  726      // Return the current balance.
  727:     balance = balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId];

3. <array>.length should not be looked up in every loop of a for-loop, Use unchecked {++i}

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset Use unchecked {} for calculations that cannot overflow/underflow, ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for-loops.

(16) instances

contracts/JBController.sol:
   913:     for (uint256 _i = 0; _i < _splits.length; _i++) {
  1014:     for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {

contracts/JBDirectory.sol:
  139:     for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
  167:     for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
  275:       for (uint256 _i; _i < _terminals.length; _i++)
  276:         for (uint256 _j = _i + 1; _j < _terminals.length; _j++)

contracts/JBETHERC20SplitsPayer.sol:
  466:     for (uint256 i = 0; i < _splits.length; i++) {

contracts/JBFundingCycleStore.sol:
  724:     for (uint256 i = 0; i < _discountMultiple; i++) {

contracts/JBOperatorStore.sol:
   85:     for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
  135:     for (uint256 _i = 0; _i < _operatorData.length; _i++) {
  165:     for (uint256 _i = 0; _i < _indexes.length; _i++) {

contracts/JBSingleTokenPaymentTerminalStore.sol:
  862:     for (uint256 _i = 0; _i < _terminals.length; _i++)

contracts/JBSplitsStore.sol:
  204:     for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
  211:       for (uint256 _j = 0; _j < _splits.length; _j++) {
  229:     for (uint256 _i = 0; _i < _splits.length; _i++) {
  304:     for (uint256 _i = 0; _i < _splitCount; _i++) {

4. Functions guaranteed to revert when called by normal users can be marked payable

the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function

(18) functions

contracts/JBETHERC20ProjectPayer.sol:
  203:   ) external virtual override onlyOwner {

contracts/JBETHERC20SplitsPayer.sol:
  209:   ) external virtual override onlyOwner {

contracts/JBPrices.sol:
  113:   ) external override onlyOwner {

contracts/JBProjects.sol:
  179:   function setTokenUriResolver(IJBTokenUriResolver _newResolver) external override onlyOwner {

contracts/JBToken.sol:
  110:   ) external override onlyOwner {
  131:   ) external override onlyOwner {

contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:
  622:   function setFee(uint256 _fee) external virtual override onlyOwner {
  644:   function setFeeGauge(IJBFeeGauge _feeGauge) external virtual override onlyOwner {
  661:   function setFeelessAddress(address _address, bool _flag) external virtual override onlyOwner {

contracts/JBController.sol:
  464    function launchFundingCyclesFor(
            ...
  473    )
  474      external
  475      virtual
  476      override
  477:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.RECONFIGURE)
  478      returns (uint256 configuration)
  479    {

  520    function reconfigureFundingCyclesOf(
            ...
  528    )
  529      external
  530      virtual
  531      override
  532:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.RECONFIGURE)
  533      returns (uint256 configuration)
  534    {

  562    function issueTokenFor(
  563      uint256 _projectId,
  564      string calldata _name,
  565      string calldata _symbol
  566    )
  567      external
  568      virtual
  569      override
  570:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.ISSUE)
  571      returns (IJBToken token)
  572    {

  588    function changeTokenOf(
  589      uint256 _projectId,
  590      IJBToken _token,
  591      address _newOwner
  592    )
  593      external
  594      virtual
  595      override
  596:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.CHANGE_TOKEN)
  597    {

  711    function burnTokensOf(
            ...
  717    )
  718      external
  719      virtual
  720      override
  721:     requirePermissionAllowingOverride(
  722        _holder,
  723        _projectId,
  724        JBOperations.BURN,
  725        directory.isTerminalOf(_projectId, IJBPaymentTerminal(msg.sender))
  726      )
  727    {

  800    function migrate(uint256 _projectId, IJBMigratable _to)
  801      external
  802      virtual
  803      override
  804:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.MIGRATE_CONTROLLER)
  805    {

contracts/JBDirectory.sol:
  211    function setControllerOf(uint256 _projectId, address _controller)
  212      external
  213      override
  214:     requirePermissionAllowingOverride(
  215        projects.ownerOf(_projectId),
  216        _projectId,
  217        JBOperations.SET_CONTROLLER,
  218        (msg.sender == address(controllerOf[_projectId]) ||
  219          (isAllowedToSetFirstController[msg.sender] && controllerOf[_projectId] == address(0)))
  220      )
  221    {

  251    function setTerminalsOf(uint256 _projectId, IJBPaymentTerminal[] calldata _terminals)
  252      external
  253      override
  254:     requirePermissionAllowingOverride(
  255        projects.ownerOf(_projectId),
  256        _projectId,
  257        JBOperations.SET_TERMINALS,
  258        msg.sender == address(controllerOf[_projectId])
  259      )
  260    {

  297    function setPrimaryTerminalOf(
  298      uint256 _projectId,
  299      address _token,
  300      IJBPaymentTerminal _terminal
  301    )
  302      external
  303      override
  304:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.SET_PRIMARY_TERMINAL)
  305    {

contracts/JBProjects.sol:
  162    function setMetadataOf(uint256 _projectId, JBProjectMetadata calldata _metadata)
  163      external
  164      override
  165:     requirePermission(ownerOf(_projectId), _projectId, JBOperations.SET_METADATA)
  166    {

contracts/JBSplitsStore.sol:
  147    function set(
  148      uint256 _projectId,
  149      uint256 _domain,
  150      JBGroupedSplits[] calldata _groupedSplits
  151    )
  152      external
  153      override
  154:     requirePermissionAllowingOverride(
  155        projects.ownerOf(_projectId),
  156        _projectId,
  157        JBOperations.SET_SPLITS,
  158        address(directory.controllerOf(_projectId)) == msg.sender
  159      )
  160    {

contracts/JBTokenStore.sol:
  391    function claimFor(
            ...
  395:   ) external override requirePermission(_holder, _projectId, JBOperations.CLAIM) {

  432    function transferFrom(
  433      address _holder,
  434      uint256 _projectId,
  435      address _recipient,
  436      uint256 _amount
  437:   ) external override requirePermission(_holder, _projectId, JBOperations.TRANSFER) {

  468    function shouldRequireClaimingFor(uint256 _projectId, bool _flag)
  469      external
  470      override
  471:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.REQUIRE_CLAIM)
  472    {

contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:
  385    function redeemTokensOf(
            ...
  394    )
  395      external
  396      virtual
  397      override
  398:     requirePermission(_holder, _projectId, JBOperations.REDEEM)
  399      returns (uint256 reclaimAmount)
  400    {

  470    function useAllowanceOf(
            ...
  478    )
  479      external
  480      virtual
  481      override
  482:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.USE_ALLOWANCE)
  483      returns (uint256 netDistributedAmount)
  484    {

  502    function migrate(uint256 _projectId, IJBPaymentTerminal _to)
  503      external
  504      virtual
  505      override
  506:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.MIGRATE_TERMINAL)
  507      returns (uint256 balance)
  508    {

  573    function processFees(uint256 _projectId)
  574      external
  575      virtual
  576      override
  577:     requirePermissionAllowingOverride(
  578        projects.ownerOf(_projectId),
  579        _projectId,
  580        JBOperations.PROCESS_FEES,
  581        msg.sender == owner()
  582      )
  583    {