Juicebox contest - Aymen0909's results

Lines of code

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

Vulnerability details

Impact

When trying to calculate the total redemption weight named weight using the totalRedemptionWeight function the wrong tierId is passed to the function _numberOfReservedTokensOutstandingFor which is responsible of determining the number of remaining reserved tokens, this will cause the _numberOfReservedTokensOutstandingFor function to return a wrong value for the remaining reserved tokens and thus the total redemption weight calculated will be wrong.

Proof of Concept

The issue occurs in the code below :

function totalRedemptionWeight(address _nft) public view override returns (uint256 weight) {
    // Keep a reference to the greatest tier ID.
    uint256 _maxTierId = maxTierIdOf[_nft];

    // Keep a reference to the tier being iterated on.
    JBStored721Tier memory _storedTier;

    // Add each token's tier's contribution floor to the weight.
    for (uint256 _i; _i < _maxTierId; ) {
      // Keep a reference to the stored tier.
      _storedTier = _storedTierOf[_nft][_i + 1];

      // Add the tier's contribution floor multiplied by the quantity minted.
      weight +=
        (_storedTier.contributionFloor *
          (_storedTier.initialQuantity - _storedTier.remainingQuantity)) +
        _numberOfReservedTokensOutstandingFor(_nft, _i, _storedTier);
        
        /* 
        @audit In the line above :
        The _storedTier of id (i+1) is given to the _numberOfReservedTokensOutstandingFor function 
        But the tierId passed is equal to (i)
        */

      unchecked {
        ++_i;
      }
    }
  }

As you can see when trying to add to the weight variable inside the for loop the _numberOfReservedTokensOutstandingFor function is called to get the reserved tokens outstanding, this function expect the following arguments :

• _nft : The NFT to get reserved tokens outstanding.
• _tierId : The ID of the tier to get a number of reserved tokens outstanding.
• _storedTier : The tier to get a number of reserved tokens outstanding.

This means that the _numberOfReservedTokensOutstandingFor expect to get the _tierId correspanding to the given _storedTier, But in the totalRedemptionWeight function the _storedTier given to the _numberOfReservedTokensOutstandingFor function is of tierId = i+1 and the actual tier id is _tierId = i

_storedTier = _storedTierOf[_nft][_i + 1];

...

_numberOfReservedTokensOutstandingFor(_nft, _i, _storedTier);

This will cause the _numberOfReservedTokensOutstandingFor function to get the _reserveTokensMinted for the _tierId = i :

 uint256 _reserveTokensMinted = numberOfReservesMintedFor[_nft][_tierId];

And use it to calculate the _numberOfNonReservesMinted for the tier with _tierId = i+1

uint256 _numberOfNonReservesMinted = _storedTier.initialQuantity -
  _storedTier.remainingQuantity -
  _reserveTokensMinted;

This will give a wrong numberOfReservedTokensOutstanding and thus will give a wrong total redemption weight.

Tools Used

Manual review

Recommended Mitigation Steps

Inside the totalRedemptionWeight function correct the arguments passed to the _numberOfReservedTokensOutstandingFor function, the following changes must be made :

function totalRedemptionWeight(address _nft) public view override returns (uint256 weight) {
    // Keep a reference to the greatest tier ID.
    uint256 _maxTierId = maxTierIdOf[_nft];

    // Keep a reference to the tier being iterated on.
    JBStored721Tier memory _storedTier;

    // Add each token's tier's contribution floor to the weight.
    for (uint256 _i; _i < _maxTierId; ) {
      // Keep a reference to the stored tier.
      _storedTier = _storedTierOf[_nft][_i + 1];

      // Add the tier's contribution floor multiplied by the quantity minted.
      weight +=
        (_storedTier.contributionFloor *
          (_storedTier.initialQuantity - _storedTier.remainingQuantity)) +
        _numberOfReservedTokensOutstandingFor(_nft, _i + 1, _storedTier);
        
        /* 
        @audit Give _storedTier of id = i+1 with tierId = i+1
        */

      unchecked {
        ++_i;
      }
    }
  }

QA Report

Summary

Findings

1- Immutable state variables lack zero address checks :

Constructors should check the values written in an immutable state variables(address, uint, int) is not the zero value (address(0) or 0)

Impact - Low Risk

Proof of Concept

Instances include:

File: contracts/JBTiered721DelegateDeployer.sol Line 51

globalGovernance = _globalGovernance;

File: contracts/JBTiered721DelegateDeployer.sol Line 52

tieredGovernance = _tieredGovernance;

File: contracts/JBTiered721DelegateDeployer.sol Line 53

noGovernance = _noGovernance;

File: contracts/JBTiered721DelegateProjectDeployer.sol Line 52

controller = _controller;

File: contracts/JBTiered721DelegateProjectDeployer.sol Line 53

delegateDeployer = _delegateDeployer;

Mitigation

Add non-zero address check in the constructors for the instances aforementioned.

2- setDefaultReservedTokenBeneficiary missing zero address check for new default beneficiary :

The setDefaultReservedTokenBeneficiary function inside the JBTiered721DelegateStore contract is missing a zero address check for the new default beneficiary, this could lead to having defaultReservedTokenBeneficiaryOf == address(0) for a given collection which could be a problem if the _reservedTokenBeneficiaryOf is also address(0) because there will be no beneficiary for this collection.

Risk : Low

Proof of Concept

Instance include:

File: contracts/JBTiered721Delegate.sol Line 375

function setDefaultReservedTokenBeneficiary(address _beneficiary) external override onlyOwner {
    // Set the beneficiary.
    store.recordSetDefaultReservedTokenBeneficiary(_beneficiary);

    emit SetDefaultReservedTokenBeneficiary(_beneficiary, msg.sender);
  }

Mitigation

Add a non-zero address check in the setDefaultReservedTokenBeneficiary function

3- Related data should be grouped in a struct :

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: contracts/JBTiered721DelegateStore.sol

119      mapping(address => uint256) public override maxTierIdOf;
129      mapping(address => mapping(address => mapping(uint256 => uint256))) public override tierBalanceOf;
138      mapping(address => mapping(uint256 => uint256)) public override numberOfReservesMintedFor;
147      mapping(address => mapping(uint256 => uint256)) public override numberOfBurnedFor;
155      mapping(address => address) public override defaultReservedTokenBeneficiaryOf;
164      mapping(address => mapping(uint256 => address)) public override firstOwnerOf;
172      mapping(address => string) public override baseUriOf;  
180      mapping(address => IJBTokenUriResolver) public override tokenUriResolverOf;  
188      mapping(address => string) public override contractUriOf; 
197      mapping(address => mapping(uint256 => bytes32)) public override encodedIPFSUriOf;    

Mitigation

Group the related data in a struct and use one mapping:

struct Collection {
    uint256 maxTierIdOf;
    address defaultReservedTokenBeneficiaryOf;
    string baseUriOf;
    string contractUriOf;
    IJBTokenUriResolver tokenUriResolverOf;
    mapping(address => mapping(uint256 => uint256)) tierBalanceOf;
    mapping(uint256 => uint256) numberOfReservesMintedFor;
    mapping(uint256 => uint256) numberOfBurnedFor;
    mapping(uint256 => address) firstOwnerOf;
    mapping(uint256 => bytes32) encodedIPFSUriOf;
}

And it would be used as a state variable :

mapping(address => Collection) public nftCollections;

Gas Optimizations

Findings

Findings

1- State variables only set in the constructor should be declared immutable :

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

There are 4 instances of this issue:

File: contracts/JBTiered721Delegate.sol Line 48

address public override codeOrigin;

2- Multiple address mappings can be combined into a single mapping of an address to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 10 instances of this issue :

File: contracts/JBTiered721DelegateStore.sol

119      mapping(address => uint256) public override maxTierIdOf;
129      mapping(address => mapping(address => mapping(uint256 => uint256))) public override tierBalanceOf;
138      mapping(address => mapping(uint256 => uint256)) public override numberOfReservesMintedFor;
147      mapping(address => mapping(uint256 => uint256)) public override numberOfBurnedFor;
155      mapping(address => address) public override defaultReservedTokenBeneficiaryOf;
164      mapping(address => mapping(uint256 => address)) public override firstOwnerOf;
172      mapping(address => string) public override baseUriOf;  
180      mapping(address => IJBTokenUriResolver) public override tokenUriResolverOf;  
188      mapping(address => string) public override contractUriOf; 
197      mapping(address => mapping(uint256 => bytes32)) public override encodedIPFSUriOf;    

These mappings could be refactored into the following struct and mapping for example :

struct Collection {
    uint256 maxTierIdOf;
    address defaultReservedTokenBeneficiaryOf;
    string baseUriOf;
    string contractUriOf;
    IJBTokenUriResolver tokenUriResolverOf;
    mapping(address => mapping(uint256 => uint256)) tierBalanceOf;
    mapping(uint256 => uint256) numberOfReservesMintedFor;
    mapping(uint256 => uint256) numberOfBurnedFor;
    mapping(uint256 => address) firstOwnerOf;
    mapping(uint256 => bytes32) encodedIPFSUriOf;
}
    
mapping(address => Collection) public nftCollections;

3- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There is 1 instance of this issue:

File: contracts/JBTiered721DelegateStore.sol Line 1077

leftoverAmount = leftoverAmount - _storedTier.contributionFloor;

The above operation cannot underflow due to the check :

if (_storedTier.contributionFloor > leftoverAmount) revert INSUFFICIENT_AMOUNT();

and should be modified as follows :

unchecked {
    leftoverAmount = leftoverAmount - _storedTier.contributionFloor;
}

4- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There is 1 instance of this issue:

File: contracts/JBTiered721DelegateStore.sol Line 827

numberOfReservesMintedFor[msg.sender][_tierId] += _count;

5- Use calldata instead of memory for function parameters type :

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

There are 12 instances of this issue:

File: contracts/JB721TieredGovernance.sol Line 147

function setTierDelegates(JBTiered721SetTierDelegatesData[] memory _setTierDelegatesData)

File: contracts/JBTiered721Delegate.sol Line 264

function mintReservesFor(JBTiered721MintReservesForTiersData[] memory _mintReservesForTiersData)

File: contracts/JBTiered721Delegate.sol Line 290

function mintFor(JBTiered721MintForTiersData[] memory _mintForTiersData)

File: contracts/JBTiered721Delegate.sol Line 490

function mintFor(uint16[] memory _tierIds, address _beneficiary)

File: contracts/JBTiered721Delegate.sol Line 598

function _didBurn(uint256[] memory _tokenIds) internal override

File: contracts/JBTiered721Delegate.sol Line 650-654

function _mintAll(
    uint256 _amount,
    uint16[] memory _mintTierIds,
    address _beneficiary
  )

File: contracts/JBTiered721Delegate.sol Line 695

function _redemptionWeightOf(uint256[] memory _tokenIds)

File: contracts/JBTiered721DelegateStore.sol Line 628

function recordAddTiers(JB721TierParams[] memory _tiersToAdd)

File: contracts/JBTiered721DelegateStore.sol Line 1091

function recordBurn(uint256[] memory _tokenIds)

File: contracts/libraries/JBIpfsDecoder.sol Line 628

function _truncate(uint8[] memory _array, uint8 _length)

File: contracts/libraries/JBIpfsDecoder.sol Line 74

function _reverse(uint8[] memory _input)

File: contracts/libraries/JBIpfsDecoder.sol Line 82

function _toAlphabet(uint8[] memory _indices)

6- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 5 instances of this issue:

File: contracts/libraries/JBIpfsDecoder.sol Line 49

for (uint256 i = 0; i < _source.length; ++i)

File: contracts/libraries/JBIpfsDecoder.sol Line 51

for (uint256 j = 0; j < digitlength; ++j)

File: contracts/libraries/JBIpfsDecoder.sol Line 68

for (uint256 i = 0; i < _length; i++)

File: contracts/libraries/JBIpfsDecoder.sol Line 76

for (uint256 i = 0; i < _input.length; i++)

File: contracts/libraries/JBIpfsDecoder.sol Line 84

for (uint256 i = 0; i < _indices.length; i++)