Juicebox V2 contest - Rohan16's results

1. Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Instances

//Links to githubfile https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L87 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L88

//actual codes which shows the use.

juice-contracts-v2-code4rena/contracts/JBERC20PaymentTerminal.sol:87:      ? IERC20(token).transfer(_to, _amount)
juice-contracts-v2-code4rena/contracts/JBERC20PaymentTerminal.sol:88:      : IERC20(token).transferFrom(_from, _to, _amount);

Reference:

This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol.

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

2.Approve() is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance().Whenever possible, use {safeIncreaseAllowance} and {safeDecreaseAllowance} instead

Instances

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

//actual codes which shows the use

juice-contracts-v2-code4rena/contracts/JBERC20PaymentTerminal.sol:99:    IERC20(token).approve(_to, _amount);

References

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bfff03c0d2a59bcd8e2ead1da9aed9edf0080d05/contracts/token/ERC20/utils/SafeERC20.sol#L38-L45

Recommended

Whenever possible, use {safeIncreaseAllowance} and {safeDecreaseAllowance} instead.

1.An array’s length should be cached to save gas in for-loops

An array’s length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Instances

/// Links to githubfiles

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L913 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L1014 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBDirectory.sol#L139 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBDirectory.sol#L167 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBDirectory.sol#L275 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBDirectory.sol#L276 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/JBOperatorStore.sol#L135 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBOperatorStore.sol#L165 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L165 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L204 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L211 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L229

/// actual codes 
juice-contracts-v2-code4rena/contracts/JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++)
juice-contracts-v2-code4rena/contracts/JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++)
juice-contracts-v2-code4rena/contracts/JBDirectory.sol:139:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
juice-contracts-v2-code4rena/contracts/JBDirectory.sol:167:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
juice-contracts-v2-code4rena/contracts/JBDirectory.sol:275:      for (uint256 _i; _i < _terminals.length; _i++)
juice-contracts-v2-code4rena/contracts/JBDirectory.sol:276:        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
juice-contracts-v2-code4rena/contracts/JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++)
juice-contracts-v2-code4rena/contracts/JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++)
juice-contracts-v2-code4rena/contracts/JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++)
juice-contracts-v2-code4rena/contracts/JBSplitsStore.sol:165:    for (uint256 _i = 0; _i < _groupedSplitsLength; )
juice-contracts-v2-code4rena/contracts/JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++)
juice-contracts-v2-code4rena/contracts/JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) 
juice-contracts-v2-code4rena/contracts/JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) 

Recommendation

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead of .length