# Potential unbounded loops in `JBTiered721DelegateStore`

[code423n4]

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()

[drgorillamd]

Disagree with:

Over time maxTierIdOf for a nft address gets large due to several increments

There is no several increments outside of adding new tiers by the project owner (this is similar to adding new token in an erc1155 - there is no such check in, for instance, OZ https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/ERC1155.sol), this is a project owner choice, not a faulty logic

[trust1995]

Agree that this is a design choice and that no danger to any user funds or usability makes it a QA recommendation issue.

[Picodes]

I don't think https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/ERC1155.sol uses unbounded for loop ?

[Picodes]

User funds could be at stake as redeemParams would revert because of the for loop in totalRedemptionWeight. A limit value would be indeed a good safeguard.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Simon-Busch marked the issue as duplicate of #64