Juicebox contest - Lambda's results

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/89cea0e2a942a9dc9e8d98ae2c5f1b8f4d916438/contracts/JBTiered721DelegateStore.sol#L701

Vulnerability details

Impact

When the reservedTokenBeneficiary of a tier is equal to defaultReservedTokenBeneficiaryOf[msg.sender], it is not explicitly set for this tier. This generally works well because in the function reservedTokenBeneficiaryOf(address _nft, uint256 _tierId), defaultReservedTokenBeneficiaryOf[_nft] is used as a backup when _reservedTokenBeneficiaryOf[_nft][_tierId] is not set. However, it will lead to the wrong beneficiary when defaultReservedTokenBeneficiaryOf[msg.sender] is later changed, as this new beneficiary will be used for the tier, which is not the intended one.

Proof Of Concept

defaultReservedTokenBeneficiaryOf[address(delegate)] is originally set to address(Bob) when the following happens: 1.) A new tier 42 is added with _tierToAdd.reservedTokenBeneficiary = address(Bob). Because this is equal to defaultReservedTokenBeneficiaryOf[address(delegate)], _reservedTokenBeneficiaryOf[msg.sender][_tierId] is not set. 2.) The owner calls setDefaultReservedTokenBeneficiary to change the default beneficiary (i.e., the value defaultReservedTokenBeneficiaryOf[address(delegate)]) to address(Alice). 3.) Now, every call to reservedTokenBeneficiaryOf(address(delegate), 42) will return address(Alice), meaning she will get these reserved tokens. This is of course wrong, the tier was explicitly created with Bob as the beneficiary.

Recommended Mitigation Steps

Also set _reservedTokenBeneficiaryOf[msg.sender][_tierId] when it is equal to the current default beneficiary.

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/89cea0e2a942a9dc9e8d98ae2c5f1b8f4d916438/contracts/JBTiered721DelegateStore.sol#L950

Vulnerability details

Impact

JBTiered721DelegateStore.recordMintBestAvailableTier potentially iterates over all tiers to find the one with the highest contribution floor that is lower than _amount. When there are many tiers, this loop can always run out of gas, which will cause some transactions (the ones that have a high _leftoverAmount within _processPayment) to always revert. The (implicit) limit for the number of tiers is 2^16 - 1, so it is possible that this happens in practice.

Proof Of Concept

Let's say that 1,000 tiers are registered for a project. Small payments without a leftover amount or a small amount will be succesfully processed by _processPayment, because _mintBestAvailableTier is either not called or it is called with a small amount, meaning that recordMintBestAvailableTier will exit the loop early (when it is called with a small amount). However, if a payment with a large leftover amount (let's say greater than the highest contribution floor) is processed, it is necessary to iterate over all tiers, which will use too much gas and cause the processing to revert.

Recommended Mitigation Steps

Use a binary search (which requires some architectural changes) for determining the best available tier. Then, the gas usage grows logarithmically (instead of linear with the current design) with the number of tiers, meaning that it would only be ~16 times higher for 65535 tiers as for 2 tiers.

Lines of code

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

Vulnerability details

Impact

According to EIP-721, tokenURI "Throws if _tokenId is not a valid NFT." This is not the case for JBTiered721Delegate. In such situations, an empty string is returned.

Proof Of Concept

According to EIP-721, a valid way to determine if a token ID exists is:

bool exists = true;
try nft.tokenURI(_i) returns (string memory uri) {
  // Some other code
} catch {
  exists = false;
}

For JBTiered721Delegate, this will always result in the token existing, even if it does not. Depending on the applications, this can have negative consequences and lead to malfunctioning or a loss of funds.

Recommended Mitigation Steps

Throw if the token ID does not exist.