Juicebox contest - brgltd's results

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateStore.sol#L688-L696

Vulnerability details

Solidity won't perform automatic checks when downcasting and it's possible for some fields to overflow while adding tiers.

Proof of Concept

• Assume that in JBTiered721DelegateStore.recordAddTiers(), one item for _tiersToAdd contains votingUnits bigger than the size of uint16, e.g. 65536.
• The data stored in _storedTierOf will overflow, e.g. if the input is 65536 the value will be 0.

Similar behavior can occur for other fields for a tier.

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateStore.sol#L688-L696

Impact

Slient overflows will affect tier accouting and can cause unexpected behavior in the protocol.

Recommended Mitigation Steps

Make use of a safe-cast library. E.g. OpenZeppelin's SafeCast.

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateStore.sol#L349 https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateStore.sol#L403 https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateStore.sol#L504 https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateStore.sol#L558

Vulnerability details

Impact

Multiple loops in JBTiered721DelegateStore are iterating over maxTierIdOf for a nft address. This value is incremented when calling recordAddTiers(). The contract doesn't seem to have a functionality for decreasing this value.

Proof of Concept

• Over time maxTierIdOf for a nft address gets large due to several increments
• Retrieve data for totalSupply(), votingUnitsOf(), balanceOf(), totalRedemptionWeight() may run out of gas.

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

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

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

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

Recommended Mitigation Steps

Add a limit value when increment maxTierIfOf inside recordAddTiers(). E.g. require(maxTierIdOf[msg.sender] <= CONSTANT_MAX_VALUE).

Additionally consider adding a pagination functionality when retrieving data for totalSupply(), votingUnitsOf(), balanceOf() and totalRedemptionWeight()

[01] Missing address(0) checks for setter function

If a variable gets configured with address zero, failure to immediately reset the value can result in unexpected behavior for the project.

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JB721TieredGovernance.sol#L177-L179

[02] Critical changes should use two-step procedure

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

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

[03] Only emit events if the input parameter changes

Assume the following scenario:

• The owner updates JBTiered721Delegate.setBaseUri() with the existing current baseUri, potentially by accident
• Offchain monitoring will assume there's a new baseUri. This can create confusion for the protocol.

Recommendation

Consider checking if the new parameter is different from the existing parameter for the following functions:

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

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

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

[04] Missing NATSPEC

Consider adding NATSPEC on all functions to improve documentation.

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/libraries/JBIpfsDecoder.sol#L22

[05] Inconsistent use of named return variables

Some functions return named variables, others return explicit values.

Following function return explicit value

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JB721TieredGovernance.sol#L74-L81

Following function return return a named variable

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JB721GlobalGovernance.sol#L32-L40

Consider adopting a consistent approach to return values throughout the codebase by removing all named return variables, explicitly declaring them as local variables, and adding the necessary return statements where appropriate. This would improve both the explicitness and readability of the code, and it may also help reduce regressions during future code refactors.

[01] The increment in for loop post condition can be made unchecked to save gas

It can save 30-40 gas per loop

There are 5 instances of this issue.

File: contracts/libraries/JBIpfsDecoder.sol
49: for (uint256 i = 0; i < _source.length; ++i) {
51: for (uint256 j = 0; j < digitlength; ++j) {
68: for (uint256 i = 0; i < _length; i++) {
76: for (uint256 i = 0; i < _input.length; i++) {
84: for (uint256 i = 0; i < _indices.length; i++) {

[02] x += y costs more gas than x = x + y for state variables

Using the addition operator instead of plus-equals saves 113 gas.

There's 1 instance with this issue.

File: contracts/JBTiered721DelegateStore.sol
827: numberOfReservesMintedFor[msg.sender][_tierId] += _count;

[03] Cache state variable reads

Caching of a state variable replace each Gwarmaccess (100 gas) with a cheaper stack read.

For example, on the following instance, projectId should be cached.

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/abstract/JB721Delegate.sol#L232-L233