Juicebox V2 contest - Sm4rty's results

Lines of code

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

Vulnerability details

Impact

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.

Proof of Concept

JBERC20PaymentTerminal.sol:87 JBERC20PaymentTerminal.sol:88

contracts/JBERC20PaymentTerminal.sol:87:      ? IERC20(token).transfer(_to, _amount)

contracts/JBERC20PaymentTerminal.sol:88:      : IERC20(token).transferFrom(_from, _to, _amount);

Tools Used

VsCode

Reference:

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

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

1. _sendValueWithFallbackWithdraw() instead of sendValue()

Contract should make use of _sendValueWithFallbackWithdraw() instead of sendValue(). This will ensure invalid transfers are correctly handled.Generally the use of sendValueWithFallbackWithdraw is when we are sending ETH to an address other than the msg.sender

Instances:

JBETHPaymentTerminal.sol:78

contracts/JBETHPaymentTerminal.sol:78:    Address.sendValue(_to, _amount);

Recommendations:

Contract should make use of _sendValueWithFallbackWithdraw() instead of sendValue(). This will ensure invalid transfers are correctly handled.

---

---

2. USE SAFEAPPROVE INSTEAD OF APPROVE

This is probably an oversight since SafeERC20 was imported and safeTransfer() was used for ERC20 token transfers. Nevertheless, note that approve() will fail for certain token implementations that do not return a boolean value (). Hence it is recommend to use safeApprove().

Instances:

contracts/JBERC20PaymentTerminal.sol:99

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

Recommended Mitigation Steps

Update to IERC20(token).safeapprove(_to, _amount); in the function.

---

---

3. Unbounded loop on array can lead to DoS

As this array can grow quite large, the transaction’s gas cost could exceed the block gas limit and make it impossible to call this function at all (see @audit):

Instances:

JBDirectory.sol:355-372 JBDirectory.sol:126-141

File: JBDirectory.sol
355:     function _addTerminalIfNeeded(uint256 _projectId, IJBPaymentTerminal _terminal) private {
...
...
369:     _terminalsOf[_projectId].push(_terminal); //@audit low: a push exist but there's no pop in the solution.
...
...
372:     }

...

126:     function primaryTerminalOf(uint256 _projectId, address _token)
...
...
139:         for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {//@audit low: poolInfo is unbounded
140:             IJBPaymentTerminal _terminal = _terminalsOf[_projectId][_i];

Recommendations

Consider introducing a reasonable upper limit based on block gas limits and/or adding a remove method to remove elements in the array.

---

---

1. Variables: No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

We can use uint number; instead of uint number = 0;

Instance Includes:

contracts/JBProjects.sol:40 contracts/JBSplitsStore.sol:227

contracts/JBSplitsStore.sol:209

contracts/JBProjects.sol:40:  uint256 public override count = 0;
contracts/JBSplitsStore.sol:227:    uint256 _percentTotal = 0;

contracts/JBSplitsStore.sol:209:      bool _includesLocked = false;

Recommendation:

I suggest removing explicit initializations for default values.

---

---

2. ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too)

Saves 6 gas PER LOOP

Instances:

contracts/JBDirectory.sol:139: contracts/JBDirectory.sol:167: contracts/JBDirectory.sol:275: contracts/JBDirectory.sol:276: contracts/JBOperatorStore.sol:85: contracts/JBOperatorStore.sol:135: contracts/JBOperatorStore.sol:165: contracts/JBController.sol:913: contracts/JBController.sol:1014: contracts/JBSplitsStore.sol:204: contracts/JBSplitsStore.sol:211: contracts/JBSplitsStore.sol:229: contracts/JBSplitsStore.sol:304:

contracts/JBDirectory.sol:139:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
contracts/JBDirectory.sol:167:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
contracts/JBDirectory.sol:275:      for (uint256 _i; _i < _terminals.length; _i++)
contracts/JBDirectory.sol:276:        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
contracts/JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
contracts/JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
contracts/JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
contracts/JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
contracts/JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {
contracts/JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
contracts/JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
contracts/JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) {
contracts/JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {
contracts/JBSplitsStore.sol:304:    for (uint256 _i = 0; _i < _splitCount; _i++) {

Reference:

https://code4rena.com/reports/2022-05-cally#g-19-i-costs-less-gas-than-i-especially-when-its-used-in-for-loops---ii---too

---

---

3. ++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- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP

Instances:

contracts/JBDirectory.sol:139: contracts/JBDirectory.sol:167: contracts/JBDirectory.sol:275: contracts/JBDirectory.sol:276: contracts/JBOperatorStore.sol:85: contracts/JBOperatorStore.sol:135: contracts/JBOperatorStore.sol:165: contracts/JBController.sol:913: contracts/JBController.sol:1014: contracts/JBSplitsStore.sol:204: contracts/JBSplitsStore.sol:211: contracts/JBSplitsStore.sol:229: contracts/JBSplitsStore.sol:304:

contracts/JBDirectory.sol:139:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
contracts/JBDirectory.sol:167:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
contracts/JBDirectory.sol:275:      for (uint256 _i; _i < _terminals.length; _i++)
contracts/JBDirectory.sol:276:        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
contracts/JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
contracts/JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
contracts/JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
contracts/JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
contracts/JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {
contracts/JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
contracts/JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
contracts/JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) {
contracts/JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {
contracts/JBSplitsStore.sol:304:    for (uint256 _i = 0; _i < _splitCount; _i++) {

Reference:

https://code4rena.com/reports/2022-05-cally#g-11-ii-should-be-uncheckediuncheckedi-when-it-is-not-possible-for-them-to-overflow-as-is-the-case-when-used-in-for--and-while-loops

---

---

4. 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:

contracts/JBDirectory.sol:139: contracts/JBDirectory.sol:167: contracts/JBDirectory.sol:275: contracts/JBDirectory.sol:276: contracts/JBOperatorStore.sol:85: contracts/JBOperatorStore.sol:135: contracts/JBOperatorStore.sol:165: contracts/JBController.sol:913: contracts/JBController.sol:1014: contracts/JBSplitsStore.sol:204: contracts/JBSplitsStore.sol:211: contracts/JBSplitsStore.sol:229:

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

Remediation:

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

---

---