Juicebox contest - 0x1f8b's results

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721Delegate.sol#L218

Vulnerability details

Impact

The initialize method of the JBTiered721Delegate contract has as a flag that the _store argument is different from address(0), however, it can be initialized by anyone with this value to allow the project to continue with its usual initialization, the attacker could have interfered and modified the corresponding values to carry out an attack.

Proof of Concept

Looking at the method below, we highlight in green the parts that need to be initialized to prevent a call to store=address(0) from failing.

  function initialize(
    uint256 _projectId,
    IJBDirectory _directory,
    string memory _name,
    string memory _symbol,
    IJBFundingCycleStore _fundingCycleStore,
    string memory _baseUri,
    IJBTokenUriResolver _tokenUriResolver,
    string memory _contractUri,
    JB721PricingParams memory _pricing,
    IJBTiered721DelegateStore _store,
    JBTiered721Flags memory _flags
  ) public override {
    // Make the original un-initializable.
    require(address(this) != codeOrigin);
    // Stop re-initialization.
    require(address(store) == address(0));

    // Initialize the sub class.
    JB721Delegate._initialize(_projectId, _directory, _name, _symbol);

    fundingCycleStore = _fundingCycleStore;
    store = _store;
    pricingCurrency = _pricing.currency;
    pricingDecimals = _pricing.decimals;
    prices = _pricing.prices;

    // Store the base URI if provided.
+   if (bytes(_baseUri).length != 0) _store.recordSetBaseUri(_baseUri);

    // Set the contract URI if provided.
+   if (bytes(_contractUri).length != 0) _store.recordSetContractUri(_contractUri);

    // Set the token URI resolver if provided.
+   if (_tokenUriResolver != IJBTokenUriResolver(address(0)))
      _store.recordSetTokenUriResolver(_tokenUriResolver);

    // Record adding the provided tiers.
+   if (_pricing.tiers.length > 0) _store.recordAddTiers(_pricing.tiers);

    // Set the flags if needed.
    if (
+     _flags.lockReservedTokenChanges ||
+     _flags.lockVotingUnitChanges ||
+     _flags.lockManualMintingChanges ||
+     _flags.pausable
    ) _store.recordFlags(_flags);

    // Transfer ownership to the initializer.
    _transferOwnership(msg.sender);
  }

So if the attacker initializes the contract as follows:

• _baseUri = ""
• _contractUri = ""
• _tokenUriResolver = address(0)
• _pricing.tiers = []
• _flags = all false

The contract will be initialized and transfered the ownership to msg.sender.

After that, the owner can call didPay with the the fake data provided in JBTiered721Delegate.sol:221 and increase creditsOf of anyone JBTiered721Delegate.sol:587 without touching any store call.

• The attacker can transfer the ownership to the contract, and the project will be able to initialize the contract again without notice.

Recommended Mitigation Steps

• Ensure that the store address is not empty.

• Low
  • 1. Outdated packages
  • 2. Use string.concat instead of abi.encodePacked
  • 3. Integer overflow by unsafe casting
  • 4. Lack of checks supportsInterface
  • 5. Lack of initialize protections

Low

1. Outdated packages

Some used packages are out of date, it is good practice to use the latest version of these packages:

"@openzeppelin/contracts": "^4.6.0"

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/releases

2. Use string.concat instead of abi.encodePacked

Tthere is a discussion about removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Affected source code:

• JBIpfsDecoder.sol:28
• JBIpfsDecoder.sol:34

3. Integer overflow by unsafe casting

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types.

Recommendation:

Use a safeCast from Open Zeppelin.

  function tierIdOfToken(uint256 _tokenId) public pure override returns (uint256) {
    // The tier ID is in the first 16 bits.
    return uint256(uint16(_tokenId));
  }

Affected source code:

• JBTiered721DelegateStore.sol:587
• JBTiered721DelegateStore.sol:689
• JBTiered721DelegateStore.sol:690
• JBTiered721DelegateStore.sol:691
• JBTiered721DelegateStore.sol:692
• JBTiered721DelegateStore.sol:693
• JBTiered721DelegateStore.sol:694
• JBIpfsDecoder.sol:50

4. Lack of checks supportsInterface

The EIP-165 standard helps detect that a smart contract implements the expected logic, prevents human error when configuring smart contract bindings, so it is recommended to check that the received argument is a contract and supports the expected interface.

Reference:

• https://eips.ethereum.org/EIPS/eip-165

Affected source code:

• JB721Delegate.sol:212
• JBTiered721DelegateDeployer.sol:51-53
• JBTiered721DelegateProjectDeployer.sol:52-53

5. Lack of initialize protections

The following contracts are updateable, and follow the update pattern, these contracts contain an initialize method to set the contract once, but anyone can call this method, this will spend project fuel if someone tries to initialize it before the project.

It is recommended to check the sender when initializing the contract.

Affected source code:

• JBTiered721Delegate.sol:202-214

• Gas
  • 1. Don't use the length of an array for loops condition
  • 2. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 1 = 13
  • 3. Use string.concat instead of abi.encodePacked
  • 4. Use calldata instead of memory
  • 5. ++i costs less gas compared to i++ or i += 1
  • 6. Use Custom Errors instead of Revert Strings to save Gas
    • Total gas saved: 9 * 2 = 18
  • 7. delete optimization
    • Total gas saved: 5 * 4 = 20
  • 8. There's no need to set default values for variables
    • Total gas saved: 8 * 1 = 9
  • 9. Remove natspec complaints
  • 10. Use the unchecked keyword

Gas

1. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• JBIpfsDecoder.sol:49
• JBIpfsDecoder.sol:76
• JBIpfsDecoder.sol:84

2. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
uint private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• JBTiered721DelegateStore.sol:827

Total gas saved: 13 * 1 = 13

3. Use string.concat instead of abi.encodePacked

abi.encodePacked has a cost difference with respect to string.concat depending on the types it uses. In the case of the reference shown below you can see that the type address is affected, so I have decided to do a test on it.

Also there is a discussion about removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Affected source code:

• JBIpfsDecoder.sol:28
• JBIpfsDecoder.sol:34

4. Use calldata instead of memory

Some methods are declared as external but the arguments are defined as memory instead of as calldata.

By marking the function as external it is possible to use calldata in the arguments shown below and save significant gas.

Recommended change:

- function mintReservesFor(JBTiered721MintReservesForTiersData[] memory _mintReservesForTiersData)
+ function mintReservesFor(JBTiered721MintReservesForTiersData[] calldata _mintReservesForTiersData)
    external
    override
  {
  ...
  }

Affected source code:

• JBTiered721Delegate.sol:264-282
• JBTiered721Delegate.sol:290-309
• JBTiered721Delegate.sol:386-391
• JBTiered721DelegateStore.sol:628-794
• JBTiered721DelegateStore.sol:1091-1114

5. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Keep in mind that this change can only be made when we are not interested in the value returned by the operation, since the result is different, you only have to apply it when you only want to increase a counter.

Affected source code:

• JBTiered721DelegateStore.sol:1106
• JBTiered721DelegateStore.sol:1108
• JBIpfsDecoder.sol:59
• JBIpfsDecoder.sol:68
• JBIpfsDecoder.sol:76
• JBIpfsDecoder.sol:84

6. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testRevert(bool path) public view {
 require(path, "test error");
}
}

contract TesterB {
error MyError(string msg);
function testError(bool path) public view {
 if(path) revert MyError("test error");
}
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611
TesterB.testError:  21602     

Affected source code:

• JBTiered721Delegate.sol:216
• JBTiered721Delegate.sol:218

Total gas saved: 9 * 2 = 18

7. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

Affected source code:

• JBTiered721Delegate.sol:588
• JBTiered721Delegate.sol:589
• JBTiered721DelegateStore.sol:772
• JBTiered721DelegateStore.sol:1198

Total gas saved: 5 * 4 = 20

8. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384

Affected source code:

• JBIpfsDecoder.sol:47

Total gas saved: 8 * 1 = 9

9. Remove natspec complaints

Remove natspec complaints in order to save gas. This is not the best way to avoid some warnings.

Affected source code:

• JB721GlobalGovernance.sol:57
• JB721TieredGovernance.sol:315

10. Use the unchecked keyword

When an underflow or overflow cannot occur, one might conserve gas by using the unchecked keyword to prevent unnecessary arithmetic underflow/overflow tests.

Affected source code:

• JBIpfsDecoder.sol:49
• JBIpfsDecoder.sol:51
• JBIpfsDecoder.sol:68
• JBIpfsDecoder.sol:77