sherlock-admin2 commented 8 months ago

zzykxx

medium

The protocol is not compatible with collections of NFTs with non-sequential IDs or sequential IDs that don't start from 0

Summary

The protocol is not compatible with collections of NFTs with non-sequential IDs or sequential IDs that don't start from 0.

Vulnerability Detail

The protocol allows to mint an auctionable NFT with a custom ID via mintToken() but the auction system assumes that the IDs of the auctioned NFTs are sequential and starting from 0.

This can cause issues in the following functions:

_isAuctionPeriod(), called to know if an NFT with ID tokenId can be auctioned, always returns false if the tokenId is lower than maxTokenCount. This means that NFTs minted with an ID bigger than the amount of NFTs currently available in the collection cannot be auctioned.
_isReadyForStranfer() performs the same check as _isAuctionPeriod().
_auctionStartTime() can set the auction start time to initialPeriodStartTimeOffset * tokenId, which means an auction for an NFT with a very high ID can potentially be far in the future.

Impact

It might be impossible to auction some NFTs.

Code Snippet

Tool used

Manual Review

Recommendation

Adjust the protocol in such a way that NFTs with non-sequential IDs or IDs not starting from 0 can be used.

gravenp commented 8 months ago

Believe this to be out of scope and a design decision. The auction contracts assume that tokenIds are indeed either sequential or "meaningfully non-sequential" to time the start of auctions. The NFT wrapping contract in the repo supports sequential tokenIds.

0xMR0 commented 8 months ago

Escalate

This seems to be intended design and should be invalid. The above comment by @gravenp should be taken for further considerations.

sherlock-admin2 commented 8 months ago

Escalate

This seems to be intended design and should be invalid. The above comment by @gravenp should be taken for further considerations.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Hash01011122 commented 8 months ago

Do you want to add anything @zzykxx ??

zzykxx commented 8 months ago

Artists are allowed to mint NFTs with non-sequential IDs or IDs that don't start from 0 via mintToken(), which breaks core functionality of the protocol (auctions). As far as I know it was not mentioned anywhere that this is a design choice.

In my opinion this has been judged correctly.

Hash01011122 commented 8 months ago

In my assessment, and as highlighted by @zzykxx in the report, it seems to be a legitimate medium-level issue that NFTs with an ID beginning at 0 disrupt the essential operations of the protocol.

realfugazzi commented 8 months ago

As per the docs, the protocol is wrapping the NFTs in their own collection, see https://github.com/sherlock-audit/2024-02-radicalxchange?tab=readme-ov-file#q-which-erc721-tokens-do-you-expect-will-interact-with-the-smart-contracts

Even so, the highlighted mintToken() function corresponds to a trusted role.

Hash01011122 commented 8 months ago

The report highlights a potential DoS (Denial of Service) issue if an NFT ID does not start from 0, causing transactions to revert within isAuctionPeriod(), but mintToken() function, which is under the control of a trusted artist, categorizing it as an admin error. This can be considered as a low or informational issue. Sponsors confirm this is a deliberate design choice, though it would be great if sponsors rectify this to prevent any negative repercussions. Your input is requested, @zzykxx, to add further insights or considerations to this analysis.

zzykxx commented 8 months ago

I stand by my position that this was not mentioned anywhere and as such it should stay as a valid issue. mintToken() doesn't have to be used maliciously for this to happen, it's simply an allowed input that will prevent the protocol from working.

realfugazzi commented 8 months ago

I stand by my position that this was not mentioned anywhere and as such it should stay as a valid issue. mintToken() doesn't have to be used maliciously for this to happen, it's simply an allowed input that will prevent the protocol from working.

It's how the protocol works and it's mentioned in the FAQ of the contest, link is two comments above.

zzykxx commented 8 months ago

I will leave my last comments for the judges to make a decision on this issue:

It was not mentioned that the protocol is not supposed to work with NFTs with non-sequential IDs. If anybody can prove otherwise I 100% agree this issue is out of scope (I might have missed it).
The NFTs in the protocol are minted via mintToken() which allows to pass an arbitrary ID as input. This function can only be called by the initialBidder and there is no need for malicious intention to mint an NFT with a random ID, it can be a legitimate choice.
Not that it matters in this case but since @realfugazzi mentioned it the README states that the owner is trusted, but the initialBidder is not necessarily the owner.

realfugazzi commented 8 months ago

1. It was not mentioned that the protocol is not supposed to work with NFTs with non-sequential IDs. If anybody can prove otherwise I 100% agree this issue is out of scope (I might have missed it).

I'll just copy what you keep ignoring, the NFT collection IS THE StewardLicense

Q: Which ERC721 tokens do you expect will interact with the smart contracts? Any (via the wrapping functionality); A "PCOArt-modified" ERC721 token is created in minting and ownership (Stewardship) is controlled by the auction/PCO logic.

zzykxx commented 7 months ago

1. It was not mentioned that the protocol is not supposed to work with NFTs with non-sequential IDs. If anybody can prove otherwise I 100% agree this issue is out of scope (I might have missed it).
I'll just copy what you keep ignoring, the NFT collection IS THE StewardLicense

Q: Which ERC721 tokens do you expect will interact with the smart contracts? Any (via the wrapping functionality); A "PCOArt-modified" ERC721 token is created in minting and ownership (Stewardship) is controlled by the auction/PCO logic.

This does not disprove point 1 you quoted. The wrapper is WrappedERC721StewardLicenseFacet. The initializer takes as an input a parameter shouldMint, when this is set to false wrapped NFTs are not minted immediately but can be minted later via mintToken(), which allows passing as input an arbitrary ID.

realfugazzi commented 7 months ago

1. It was not mentioned that the protocol is not supposed to work with NFTs with non-sequential IDs. If anybody can prove otherwise I 100% agree this issue is out of scope (I might have missed it).
I'll just copy what you keep ignoring, the NFT collection IS THE StewardLicense

Q: Which ERC721 tokens do you expect will interact with the smart contracts? Any (via the wrapping functionality); A "PCOArt-modified" ERC721 token is created in minting and ownership (Stewardship) is controlled by the auction/PCO logic.
This does not disprove point 1 you quoted. The wrapper is WrappedERC721StewardLicenseFacet. The initializer takes as an input a parameter shouldMint, when this is set to false wrapped NFTs are not minted immediately but can be minted later via mintToken(), which allows passing as input an arbitrary ID.

The scenario you just mentioned represents a mistake on the use of the protocol, note the wrapped holds the pair collection/tokenId. Read again the summary of your issue, "The protocol is not compatible with collections of NFTs with non-sequential IDs or sequential IDs that don't start from 0". Would be best to chime in the sponsor for a definitive answer.

zzykxx commented 7 months ago

The scenario you just mentioned represents a mistake on the use of the protocol.

Let me get this straight:

Your first claim was As per the docs, the protocol is wrapping the NFTs in their own collection. I showed that wrapping NFTs with non-sequential tokens ID is possible.
Your second claim was Even so, the highlighted mintToken() function corresponds to a trusted role. I showed this is incorrect because the initialBidder is the role that can call mintToken(), which is not necessarily the owner and therefore not trusted.
Your third claim is The scenario you just mentioned represents a mistake on the use of the protocol. How is it a mistake to mint NFTs with non-sequential IDs when the code itself allows it and it's not mentioned anywhere that this shouldn't be the case?

As per the sponsor comment:

The sponsor literally stated The auction contracts assume that tokenIds are indeed either sequential or "meaningfully non-sequential" to time the start of auctions.
The sponsor also added The NFT wrapping contract in the repo supports sequential tokenIds. which is correct. But "supporting" something is not the same as "enforcing" it, in fact as I showed before it's possible to mint NFTs with non-sequential token IDs via the wrapping contract.

To sum everything up:

The protocol allows to mint NFTs with non-sequential token IDs, even via the wrapping contract.
The auction process breaks when this happens.
The auction is the core process of the in-scope code.
It's not state anywhere that users should not mint NFTs with non-sequential IDs or that it's known that this would break the protocol.

Because the core functionality of the protocol breaks when NFTs with non-sequential token IDs are minted this is a medium severity issue according to sherlock rules.

If the judges need more clarifications please tag me.

realfugazzi commented 7 months ago

That is definitely not core functionality, and mintToken is provided as a way to realize the mint, not to escape the boundaries of maxTokenCount. That is why the sponsor disputed the issue.

I believe you are adjusting your arguments as the discussion progresses, your issues refers to arbitrary collections of NFTs that have non-sequential token ids, but that's not how the protocol is used. You then moved your arguments towards the license NFT.

@Hash01011122 @Czar102 any chance we can get the sponsor input?

St4rgarden commented 7 months ago

I stand by my position that this was not mentioned anywhere and as such it should stay as a valid issue. mintToken() doesn't have to be used maliciously for this to happen, it's simply an allowed input that will prevent the protocol from working.

I am personally inclined to agree that there should not be any allowed input that essentially "bricks" any of the functionality of the protocol. It is arguably a trusted role - and it's arguably misuse of the contracts... but I don't see any reason to allow misuse.

Anything that should not happen in a smart contract system should not be allowed. It's just different than a typical software system because we're dealing with real value and a system that can't be easily updated or patched.

Hash01011122 commented 7 months ago

I am inclined towards categorizing it as low issue as @St4rgarden has confirmed that it's trusted role. @zzykxx Do you want to add anything here? Please correct me if you feel I am wrong

zzykxx commented 7 months ago

I am inclined towards categorizing it as low issue as @St4rgarden has confirmed that it's trusted role. @zzykxx Do you want to add anything here? Please correct me if you feel I am wrong

I'm not sure why the fact that initialBidder is trusted or not even matters in this scenario. There's no malicious intention in minting NFTs with a non-sequential IDs.

Edit: @St4rgarden can you be more clear on what you mean by and it's arguably misuse of the contracts? In my view it's misuse only if it's clear that the contracts should not be used with NFTs with non-sequential IDs, which is not the case because the code allows it and it was not specified anywhere during the time of the audit.

realfugazzi commented 7 months ago

Indeed it's a misuse of the function. The idea of mintToken() there is to provide a way to the initial bidder to realize the mint given the license admits an option to skip minting during initialization (shouldMint).

The documentation could be better but it's there:

/**
 * @notice Initial bidder can mint token if it doesn't exist
 */

In any case this is at most a missing validation in a contract out of scope (require(tokenId < maxTokenCount)).

Hash01011122 commented 7 months ago

Would like to know whether the Watson's were aware of this at the time of audit or was anything mentioned in documentation or codebase with at most clarity about the trusted role who will be minting tokens and reasons on why non-sequential ID will not be used as posed by @zzykxx. @St4rgarden @gravenp @0xMR0 @realfugazzi, if not this issue will remain as it is.

realfugazzi commented 7 months ago

Would like to know whether the Watson's were aware of this at the time of audit or was anything mentioned in documentation or codebase with at most clarity about the trusted role who will be minting tokens and reasons on why non-sequential ID will not be used as posed by @zzykxx. @St4rgarden @gravenp @0xMR0 @realfugazzi, if not this issue will remain as it is.

The protocol is entirely based on the License NFT (ref https://github.com/sherlock-audit/2024-02-radicalxchange/blob/main/pco-art/contracts/license/StewardLicenseInternal.sol) which uses sequential ids determined by a maxTokenCount.
The protocol offers the possibility of delaying the mint of these tokens (shouldMint = false).
When used this way, the protocol allows the initial bidder to REALIZE the minting of these tokens through mintToken().
The protocol doesn't operate on arbitrary NFT collections as the watson states in the issue.
Referring to mintToken() as a way to break core functionality of the auction is completely false and inaccurate, since the auction will work just fine with the token ids originally defined for the range.
Using mintToken() with an out-of-range id is a complete misuse of the function and at most a missing check in a contract out of scope.

Brivan-26 commented 7 months ago

Indeed it's a misuse of the function. The idea of mintToken() there is to provide a way to the initial bidder to realize the mint given the license admits an option to skip minting during initialization (shouldMint).

The documentation could be better but it's there:
/**
 * @notice Initial bidder can mint token if it doesn't exist
 */
In any case this is at most a missing validation in a contract out of scope (require(tokenId < maxTokenCount)).

I was following up the discussion. This quoted reply is enough to invalidate the issue as it demonstrates that having a non-sequential ID is not the normal behavior. Minting non-sequential ID is used to recover a token that doesn't exist

Hash01011122 commented 7 months ago

Do you want to add anything here @zzykxx??

zzykxx commented 7 months ago

I've already answered @realfugazzi points previously.

Knowing that:

NFTs can have non-sequential token ID (ERC721 doesn't enforce this) and the wrapper contract allows to mint such NFTs
The auction can break if such NFTs are used

The validity of this issue stands on a simple question:

Was it stated somewhere that the use of NFTs with non-sequential IDs was not intended? If the answer is yes then the issue is invalid, if the answer is no then the issue is valid.

Me and @realfugazzi are stating the same points over and over again, we simply don't agree. He is claiming that the protocol is not intended to be used with non-sequential token IDs and I'm claiming that the protocol allows to do so, so it's technically intended unless it's stated somewhere that it's not.

For this reason from now on I will only answer concerns/questions raised by the head of judging.

realfugazzi commented 7 months ago

Was it stated somewhere that the use of NFTs with non-sequential IDs was not intended? If the answer is yes then the issue is invalid, if the answer is no then the issue is valid.

This is nonsense imho. Your side of the story implies that the owners of the protocol designed an auction system for their own protocol in total dissonance with how their INTERNAL NFT works, instead of accepting a misuse of just one function.

I'll reiterate this point also because you keep repeating the same misleading argument, I'd also recommended judges to go over the original report again.

The protocol doesn't operate on arbitrary NFT collections as the watson states in the issue.

Evert0x commented 7 months ago

This does not disprove point 1 you quoted. The wrapper is WrappedERC721StewardLicenseFacet. The initializer takes as an input a parameter shouldMint, when this is set to false wrapped NFTs are not minted immediately but can be minted later via mintToken(), which allows passing as input an arbitrary ID.

Correct me if I'm wrong, but the StewardLicenseBase contract where the mintToken() exists, is not in scope right?

zzykxx commented 7 months ago

Correct me if I'm wrong, but the StewardLicenseBase contract where the mintToken() exists, is not in scope right?

You are correct. All the NFTs that can be auctioned in the contracts are minted via the Steward license wrapper, which is out-of-scope.

Evert0x commented 7 months ago

Planning to accept the escalation and invalidate the report as the issue exists in an out of scope contract.

andrey-kuprianov commented 7 months ago

Correct me if I'm wrong, but the StewardLicenseBase contract where the mintToken() exists, is not in scope right?

You are correct. All the NFTs that can be auctioned in the contracts are minted via the Steward license wrapper, which is out-of-scope.

@Evert0x I was following along... one thing I would like to point out: exactly the same function mintToken() from StewardLicenseBase is part of the root cause of #33. I believe for consistency of judging both of the present and the other issue should be either valid, or not. If #33 is valid, so this one should be valid as well, or vice versa. But not one valid, one invalid...

realfugazzi commented 7 months ago

Correct me if I'm wrong, but the StewardLicenseBase contract where the mintToken() exists, is not in scope right?

You are correct. All the NFTs that can be auctioned in the contracts are minted via the Steward license wrapper, which is out-of-scope.

@Evert0x I was following along... one thing I would like to point out: exactly the same function mintToken() from StewardLicenseBase is part of the root cause of #33. I believe for consistency of judging both of the present and the other issue should be either valid, or not. If #33 is valid, so this one should be valid as well, or vice versa. But not one valid, one invalid...

These are really different issues. 33 is about an edge case in the auction flow.

sammy-tm commented 7 months ago

@Evert0x I have analyzed Issue #33 carefully and would like to share my opinion on the same :

The attack described is most certainly valid (I reproduced the scenario in hardhat and tested it : https://p.ip.fi/aBlw). However, it's only possible because of two vulnerabilities present in StewardLicenseBase.sol .

Notably :

The wrong auction start time being checked in mintToken() :

function mintToken(address to, uint256 tokenId) external {
    require(
        msg.sender ==
            IPeriodicAuctionReadable(address(this)).initialBidder(),
        'StewardLicenseFacet: only initial bidder can mint token'
    );
    //slither-disable-next-line timestamp
    require(
        block.timestamp <
        @>    IPeriodicAuctionReadable(address(this))
                .initialPeriodStartTime(),
        'StewardLicenseFacet: cannot mint after initial period start time'
    );
    require(!_exists(tokenId), 'StewardLicenseFacet: Token already exists');

    _triggerTransfer(address(0), to, tokenId);
}

(it should call IPeriodicAuctionReadable(address(this)).auctionStartTime() which would return the correct auction start time)

The ability of the ADD_TOKEN_TO_COLLECTION_ROLE to set the tokenInitialPeriodStartTime[] before the l.initialPeriodStartTime.

Fixing either of these would mitigate the attack. The recommendation provided by the Watson is : "Don't allow tokenInitialPeriodStartTime[] to be set at a timestamp before l.initialPeriodStartTime."

However, it is important to note that both these vulnerabilities are in functions present in StewardLicenseBase.sol, which was out of scope for this contest. Therefore I feel that it has been incorrectly judged according to the sherlock rules, and this is unfair to other participants who chose to ignore this contract during the contest.

andrey-kuprianov commented 7 months ago

Hey, don't get me wrong. I am truly impartial here: I am not arguing for or against any finding. Personally, I really like 33: I think it's a truly ingenious finding, which should be rewarded; and the fact that StewardLicenseBase.sol is out of scope is simply a mistake of scoping.

I am only saying that from the judging perspective it is an impossible situation to apply certain rules (validity based on in/out-of-scope) differently to two findings in the same ruling.

andrey-kuprianov commented 7 months ago

@Hash01011122, @Evert0x, I dived a bit more into the implementation, and here are a few facts (and facts only):

1. EnglishPeriodicAuctionInternal, EnglishPeriodicAuctionFacet, and StewardLicenseBase are parts of one and the same contract:

See how everything is deployed via the Diamond Standard, ERC-2535, in function EnglishPeriodicAuction.ts::getInstance(). In particular, here is the function initialPeriodStartTime() (from EnglishPeriodicAuctionFacet), and here is the function mintToken() (from StewardLicenseBase).

See also how mintToken() calls initialPeriodStartTime():

      require(
          block.timestamp <
              IPeriodicAuctionReadable(address(this))
                  .initialPeriodStartTime(),
          'StewardLicenseFacet: cannot mint after initial period start time'
      );

2. Here are the Sherlock scoping rules: https://docs.sherlock.xyz/audits/judging/judging#iii.-sherlocks-standards:

sammy-tm commented 7 months ago

I would like to add some points here :

Usage of diamond proxy does not automatically mean that all the contracts involved are in scope, considering this would add way more contracts in scope and push the SLOC way above the aforementioned number.
The contract scope rules are not in favor of considering the contract in scope: It is neither a library nor a parent contract.
ADD_TO_COLLECTION_ROLE is an admin role and trusted.

zzykxx commented 7 months ago

@Evert0x What are the criteria for determining if an issue is in scope or out of scope when dealing with external integrations?

Let's suppose a protocol integrates chainlink oracles and the protocol doesn't check for stale prices, is the issue in scope because it's the responsibility of the protocol to check for stale prices or is the issue out of scope because chainlink shouldn't return stale prices in the first place?
Let's suppose a protocol integrates with curve and a read-only reentrancy attack is possible. Is the issue in scope because it's the protocol's responsibility to prevent attacks related to the curve integration or is the issue out of scope because curve shouldn't allow read-only reentrancies in the first place?

On the same line of thought:

About this issue: Is it in-scope because the in-scope contracts don't take into consideration that NFTs with arbitrary IDs can be minted or is the issue out of scope because NFTs with arbitrary IDs should not be minted in the first place? If NFTs with arbitrary IDs should not be minted in the first place, why should that be the case?
About #33: Is it in-scope because the protocol doesn't take into consideration conditions that are possible due to the external integration or is the issue out of scope because those conditions should not be possible in the first place? If those conditions should not be possible in the first place, can you point out what damage would be caused if the in-scope code didn't exist?

In both scenarios the vulnerabilities only exist because the in-scope code exists.

@sammy-tm Can you point out where it's written that ADD_TOKEN_TO_COLLECTION_ROLE is a trusted role?

sammy-tm commented 7 months ago

@zzykxx First of all, the ADD_TOKEN_TO_COLLECTION_ROLE was not in scope for this contest; therefore, whether or not it is trusted was not clearly mentioned in the Q/A or the Readme.

Since your attack involves an out-of-scope contract that requires arbitrary input by this particular role, the clarification about this was provided by the sponsor here: https://github.com/sherlock-audit/2024-02-radicalxchange-judging/issues/33#issuecomment-2053738314 and there is no clarification about this elsewhere as this piece of information was irrelevant for this particular contest's scope.

realfugazzi commented 7 months ago

* About this issue: Is it in-scope because the in-scope contracts don't take into consideration that NFTs with arbitrary IDs can be minted or is the issue out of scope because NFTs with arbitrary IDs should not be minted in the first place? If NFTs with arbitrary IDs should not be minted in the first place, why should that be the case?

* About [zzykxx - Currently auctioned NFTs can be transferred to a different address in a specific edge case #33](https://github.com/sherlock-audit/2024-02-radicalxchange-judging/issues/33): Is it in-scope because the protocol doesn't take into consideration conditions that are possible due to the external integration or is the issue out of scope because those conditions should not be possible in the first place? If those conditions should not be possible in the first place, can you point out what damage would be caused if the in-scope code didn't exist?

There's a key difference between the two issues.

In #16 the watson completely missed the point of how the protocol works, and continues arguing about collections of non-sequential ids, ignoring how the protocol works and insisting about missing documentation (hint: it's in the same code) or other vague arguments. This is an invalid issue, right from the title.

33 is different because the wrong assumption is in the auction flow. Although @sammy-tm 's point is completely valid, the core of the issue lies in `mintToken()`. In this case the edge case is valid, but ultimately it should be judged according to the rules and other precedents.

Hash01011122 commented 7 months ago

Reassessing the arguments for clearer judgment:

Issue #16 should be categorized as low or informational. The mintTokens() function is managed by a trusted role, as noted earlier by @St4rgarden. Another consideration is that the functions highlighted in this issue are out of scope, further supporting the classification as low severity at most.
Issue #33 represents a distinct edge case, identifying a logical flaw that could lead to significant discrepancies in the core functionality of the protocol. Such issues are typically rated as high severity. However, given the unique circumstances of this issue, as @sammy-tm is trying to point, the head of Judging has deemed it borderline between medium and high, ultimately assigning it a medium severity rating.

Do you want to add anything @zzykxx??

zzykxx commented 7 months ago

@Hash01011122

Wether initialBidder is trusted or not is irrelevant in the context of #16, because minting an NFT with an arbitrary ID is not a malicious action.
The fact that mintToken() is not in-scope is also irrelevant, that function is used to mint NFTs that will be auctioned, and is reponsible for setting a paramater (the NFT ID) that is used by the in-scope code.

By that reasoning #31 should also be low severity, the fee is setted by functions that are out-of-scope which are managed by an admin role. In both cases the protocol breaks because of a parameter set by an admin in an out-of-scope function.

Both setting a fee of 0 and minting an NFT with an arbitrary ID are not malicious actions, yet the in-scope code breaks when that happens.

sammy-tm commented 7 months ago

@zzykxx You are missing a key difference here. The setting of 0 fees is an expected input within the protocol (according to the docs), and the in-scope code does not comply with this assumption; the fix/mitigation also exists within the scope.

With that and my previous engagements, I will leave the decision about the validity of these issues with the head judge.

zzykxx commented 7 months ago

@sammy-tm I agree with you. I've been asking for evidence that minting an NFT with an arbitrary ID is not an expected input by the protocol since the beginning of the discussion (3 weeks ago). I quote myself for reference:

It was not mentioned that the protocol is not supposed to work with NFTs with non-sequential IDs. If anybody can prove otherwise I 100% agree this issue is out of scope (I might have missed it).

In this case, the in-scope code assumes these type of NFTs cannot be auctioned, but they can. The wrong assumption is in the way the in-scope code is designed (as it is for #31) and not in the out-of-scope code.

This being said if this issue is invalidated only (remark on "only") because of the reasons provided by @Hash01011122 in his last message, the same reasoning should be applied to #31.

Edit: Same goes for @Evert0x reasoning on classifying this as out-of-scope because an out-of-scope function is involved. Because this is true for #31, #16 and #33, if that reasoning is the only one used to classify this as out-of-scope then #31 and #33 should be classified as out-of-scope as well.

sammy-tm commented 7 months ago

I'm unsure if using #31 to argue for this particular issue is sensible. The core of this issue is within the bounds of the mintToken() function; the QA clearly states that the NFTs are wrapped in PCOArt-modified ERC721 token, which doesn't necessarily support your argument, as it would require the initialBidder to key in an arbitrary input, not sure if you can call it "expected".

Again, I trust the head judge here to make the correct decision about the validity.

zzykxx commented 7 months ago

Seems like you are now switching your argument from the issue being out-of-scope to the issue being invalid because minting an NFT with arbitrary ID is not an expected input.

Can anybody explain why an artist deciding to auction and mint an NFT with ID "100" to celebrate selling 100 artworks (or whatever other reason) is not an expected input?

sammy-tm commented 7 months ago

I never argued against/for the validity of the current issue. I only pointed out the key difference between #31 and this issue. The out-of-scope argument was for issue #33

realfugazzi commented 7 months ago

Can anybody explain why an artist deciding to auction and mint an NFT with ID "100" to celebrate selling 100 artworks (or whatever other reason) is not an expected input?

This is not how the protocol works. You can't simply adjust the whole protocol to your convenience.

0xMR0 commented 7 months ago

This is probably the most discussed issue i have ever seen. This is being discussed from last two weeks.

Friends, Please don't argue and leave the final decision on sherlock head of judging.

Hash01011122 commented 7 months ago

@Evert0x It would be appreciated if you give your final verdict here. Whatever decision you will be making here I will respect it.

Evert0x commented 7 months ago

About this issue (16). I don't judge this as an external integration as the contracts are within the same codebase.

The core issue exists in the contract that's out of scope.

Still planning to accept the escalation and invalidate the report.

About the other issue (33), the described attack path uses the out of scope mintToken() but the core issue resides in EnglishPeriodicAuctionInternal which is in scope. https://github.com/sherlock-audit/2024-02-radicalxchange/blob/main/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol#L499-L506

Evert0x commented 7 months ago

Result: Invalid Unique

sherlock-audit / 2024-02-radicalxchange-judging

zzykxx - The protocol is not compatible with collections of NFTs with non-sequential IDs or sequential IDs that don't start from 0 #16

The protocol is not compatible with collections of NFTs with non-sequential IDs or sequential IDs that don't start from 0

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

33 is different because the wrong assumption is in the auction flow. Although @sammy-tm 's point is completely valid, the core of the issue lies in `mintToken()`. In this case the edge case is valid, but ultimately it should be judged according to the rules and other precedents.

sherlock-audit / 2024-02-radicalxchange-judging

zzykxx - The protocol is not compatible with collections of NFTs with non-sequential IDs or sequential IDs that don't start from 0 #16

The protocol is not compatible with collections of NFTs with non-sequential IDs or sequential IDs that don't start from 0

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

33 is different because the wrong assumption is in the auction flow. Although @sammy-tm 's point is completely valid, the core of the issue lies in mintToken(). In this case the edge case is valid, but ultimately it should be judged according to the rules and other precedents.

33 is different because the wrong assumption is in the auction flow. Although @sammy-tm 's point is completely valid, the core of the issue lies in `mintToken()`. In this case the edge case is valid, but ultimately it should be judged according to the rules and other precedents.