Open c4-bot-1 opened 8 months ago
raymondfam marked the issue as insufficient quality report
raymondfam marked the issue as duplicate of #314
HickupHH3 changed the severity to QA (Quality Assurance)
Hey, @HickupHH3!
This is an EIP compatible problem, and in many previous code4rena contests this type of issue was marked as a valid medium difficulty problem (example).
I've spotted a compliance issue in the GameItems.sol
contract, which doesn't fully align with the ERC-1155 standard. The main problem is with the setTokenURI()
function, used to update the metadata URI for game items. According to the ERC-1155 standard, any change to a token's URI must trigger a URI
event to notify external observers. However, this contract fails to emit such an event upon updating a token's URI.
This omission could break the contract's integration with ERC-1155-compliant platforms, such as digital marketplaces or wallets, where accurate and up-to-date token metadata is critical. Without the "URI" event, these platforms may not know about recent updates, resulting in outdated or incorrect information being displayed.
Also Important: The GameItems contract's failure to emit the URI event upon updating the URI for token ID in the GameItems.sol#setTokenURI() function results in non-compliance with the ERC-1155 standard. This oversight can lead to issues with ERC-1155 compatible marketplaces, wallets, and other platforms that rely on the URI event to track changes to token metadata. Without this event, external applications will not be aware of updates to the token's metadata, leading to outdated or incorrect information being displayed to users.
upon consulting with another judge, because the EIP states that it is a MUST
:
event URI MUST emit when the URI is updated for a token ID.
and https://github.com/code-423n4/org/issues/130
Agree only in cases where the EIP uses SHOULD, but MUST is considered a requirement, and failure to implement it should be considered valid. Interoperability issues between contracts are a security concern.
medium severity is justified.
This previously downgraded issue has been upgraded by HickupHH3
HickupHH3 marked the issue as selected for report
@HickupHH3. Since when event emission is valid medium? This is the first time in the history of C4 that event emission became a valid medium because of not following ERC, can you point to me an event emission that is a valid medium? This won't even remotely break any integrators. I implore you NOT to set a precedent for this if not this will just become a race to the bottom.
Hi @Haxatron I don't appreciate the butting in and mentioning things already addressed in my responses in an attempt to kill off the competition. Either bring new info to the table or let the judge make up their mind.
Let me bring up another same exact finding that was judged QA - https://github.com/code-423n4/2023-10-ens-findings/issues/270
Hey @HickupHH3, My QA report L-09 is a duplicate of this one. Can you please take a look!
Raised this issue in the judging group, the larger majority agreed with this
The event impact is superseded by EIP compliance
That said, it's an Event and it's for the URI
I believe the most appropriate severity is informational as that's the maximum impact (even if the contract in scope was a bridge)
I believe the side effect of ruling Med would be creating more confusion and exceptions vs a consistent, Event + Informational Param = QA
In general, event-related non-ERC compliance needs further clarification, which I hope we will get in the future. I agree in not wanting to set a precedent for this; I've always been judging event emission related findings as QA as well + same finding in the past was QA.
HickupHH3 changed the severity to QA (Quality Assurance)
My reposne:
Let's look at the Code4rena Severity Categorization:
Event-related impacts A bug whose consequence is faulty emission of event(s) shall be graded in line with its broader-level impact:
- For events which are used for additional on-chain processes such as bridging, inclusion proofs etc., issue will be graded based on impacted functionality.
- Bugs that cause non-compliance with EIPs shall be graded based on EIP ruling guidelines
- Failure to demonstrate the broader level impacts above shall cap the severity to Low. For clarification, bugs leading to readability or accessibility of data, or issues of display in frontends, are capped to Low.
As we saw in c4 severity categorization writes: Bugs that cause non-compliance with EIPs shall be graded based on EIP ruling guidelines
and in the EIP1155 standard documentation writes the following:
event URI MUST emit when the URI is updated for a token ID.
the word MUST
is used here and as we can see in RFC 2119 (Key words for use in RFCs to Indicate Requirement Levels writes) writes the following:
MUST This word, or the terms "REQUIRED" or "SHALL", mean that the definition is an absolute requirement of the specification.
Again, as we saw in c4 severity categorization writes: For events which are used for additional on-chain processes such as bridging, inclusion proofs etc., issue will be graded based on impacted functionality.
and I already explain that the omission of event emitting break the contract's integration with ERC-1155-compliant platforms, such as digital marketplaces or wallets, where accurate and up-to-date token metadata is critical. This also break the contract's integration with off-chain platforms like NFT marketplaces or block explorers may show stale metadata for the NFTs, and token holders can use this stale data to their advantage. Without the "URI" event, these platforms may not know about recent updates, resulting in outdated or incorrect information being displayed.
So, the GameItems
contract failure to emit the "URI" event upon updating the URI for tokenID in the GameItems.sol#setTokenURI() function results in non-compliance with the ERC-1155 standard. This oversight can lead to issues with ERC-1155 compatible marketplaces, wallets, and other platforms that rely on the URI event to track changes to token metadata. Without this event, external applications will not be aware of updates to the token's metadata, leading to outdated or incorrect information being displayed to users.
@HickupHH3
Let me bring up another finding that was about event omission and how this can break the contract's integration with ERC-1155-compliant platforms -> https://github.com/code-423n4/2023-10-party-findings/issues/340
As we can see, this issue has been judged as a valid medium severity issue!
Let me bring up another finding that was about event omission and how this can break the contract's integration with ERC-1155-compliant platforms -> code-423n4/2023-10-party-findings#340
As we can see, this issue has been judged as a valid medium severity issue!
In the contest documentation of the PartyDAO contest, the sponsor explicitly states that the contracts must comply with the EIP. There isn't any such case in this contest's README.
Let me bring up another finding that was about event omission and how this can break the contract's integration with ERC-1155-compliant platforms -> code-423n4/2023-10-party-findings#340 As we can see, this issue has been judged as a valid medium severity issue!
In the contest documentation of the PartyDAO contest, the sponsor explicitly states that the contracts must comply with the EIP. There isn't any such case in this contest's README.
Are you serious? Your comment is 100% irrelevant!
HickupHH3 marked the issue as not selected for report
My reposne:
Let's look at the Code4rena Severity Categorization:
Event-related impacts
A bug whose consequence is faulty emission of event(s) shall be graded in line with its broader-level impact:
For events which are used for additional on-chain processes such as bridging, inclusion proofs etc., issue will be graded based on impacted functionality.
Bugs that cause non-compliance with EIPs shall be graded based on EIP ruling guidelines
Failure to demonstrate the broader level impacts above shall cap the severity to Low. For clarification, bugs leading to readability or accessibility of data, or issues of display in frontends, are capped to Low.
As we saw in c4 severity categorization writes:
Bugs that cause non-compliance with EIPs shall be graded based on EIP ruling guidelines
and in the EIP1155 standard documentation writes the following:event URI MUST emit when the URI is updated for a token ID.
the word
MUST
is used here and as we can see in RFC 2119 (Key words for use in RFCs to Indicate Requirement Levels writes) writes the following:MUST This word, or the terms "REQUIRED" or "SHALL", mean that the definition is an absolute requirement of the specification.
(absolute requirement)
- Again, as we saw in c4 severity categorization writes:
For events which are used for additional on-chain processes such as bridging, inclusion proofs etc., issue will be graded based on impacted functionality.
and I already explain that the omission of event emitting break the contract's integration with ERC-1155-compliant platforms, such as digital marketplaces or wallets, where accurate and up-to-date token metadata is critical. This also break the contract's integration with off-chain platforms like NFT marketplaces or block explorers may show stale metadata for the NFTs, and token holders can use this stale data to their advantage. Without the "URI" event, these platforms may not know about recent updates, resulting in outdated or incorrect information being displayed.
So, the
GameItems
contract failure to emit the "URI" event upon updating the URI for tokenID in the GameItems.sol#setTokenURI() function results in non-compliance with the ERC-1155 standard. This oversight can lead to issues with ERC-1155 compatible marketplaces, wallets, and other platforms that rely on the URI event to track changes to token metadata. Without this event, external applications will not be aware of updates to the token's metadata, leading to outdated or incorrect information being displayed to users.@HickupHH3
@HickupHH3 what is your answer to this
I stand by what I said: https://github.com/code-423n4/2024-02-ai-arena-findings/issues/1515#issuecomment-2014118950
Final decision, not changing. You may request an appeal committee to dispute further or open up an issue at https://github.com/code-423n4/org/ for further severity standardisation on events + EIP compliance.
I stand by what I said: #1515 (comment)
Final decision, not changing. You may request an appeal committee to dispute further or open up an issue at https://github.com/code-423n4/org/ for further severity standardisation on events + EIP compliance.
1127's been marked invalid already, as people have pointed out that it's a bot finding
I understand you @HickupHH3, but you are not following Code4rena's severity categorization rules.
Btw, the core issue/problem of #1127 report at the end is valid (included in known issues after the boat race)
@HickupHH3
upon consulting with another judge, because the EIP states that it is a
MUST
:event URI MUST emit when the URI is updated for a token ID.
Agree only in cases where the EIP uses SHOULD, but MUST is considered a requirement, and failure to implement it should be considered valid. Interoperability issues between contracts are a security concern.
medium severity is justified.
This is an EIP compatible problem, and in many previous code4rena contests this type of issue was marked as a valid medium difficulty problem (example).
I've spotted a compliance issue in the GameItems.sol
contract, which doesn't fully align with the ERC-1155 standard. The main problem is with the setTokenURI()
function, used to update the metadata URI for game items. According to the ERC-1155 standard, any change to a token's URI must trigger a URI
event to notify external observers. However, this contract fails to emit such an event upon updating a token's URI.
This omission could break the contract's integration with ERC-1155-compliant platforms, such as digital marketplaces or wallets, where accurate and up-to-date token metadata is critical. Without the "URI" event, these platforms may not know about recent updates, resulting in outdated or incorrect information being displayed.
Also Important: The GameItems contract's failure to emit the URI event upon updating the URI for token ID in the GameItems.sol#setTokenURI() function results in non-compliance with the ERC-1155 standard. This oversight can lead to issues with ERC-1155 compatible marketplaces, wallets, and other platforms that rely on the URI event to track changes to token metadata. Without this event, external applications will not be aware of updates to the token's metadata, leading to outdated or incorrect information being displayed to users.
3. Also, let me remind you the Code4rena Severity Categorization Rules
Let's look at the Code4rena Severity Categorization:
Event-related impacts A bug whose consequence is faulty emission of event(s) shall be graded in line with its broader-level impact:
- For events which are used for additional on-chain processes such as bridging, inclusion proofs etc., issue will be graded based on impacted functionality.
- Bugs that cause non-compliance with EIPs shall be graded based on EIP ruling guidelines
- Failure to demonstrate the broader level impacts above shall cap the severity to Low. For clarification, bugs leading to readability or accessibility of data, or issues of display in frontends, are capped to Low.
As we saw in c4 severity categorization writes: Bugs that cause non-compliance with EIPs shall be graded based on EIP ruling guidelines
and in the EIP1155 standard documentation writes the following:
event URI MUST emit when the URI is updated for a token ID.
the word MUST
is used here and as we can see in RFC 2119 (Key words for use in RFCs to Indicate Requirement Levels writes) writes the following:
MUST This word, or the terms "REQUIRED" or "SHALL", mean that the definition is an absolute requirement of the specification.
Again, as we saw in c4 severity categorization writes: For events which are used for additional on-chain processes such as bridging, inclusion proofs etc., issue will be graded based on impacted functionality.
and I already explain that the omission of event emitting break the contract's integration with ERC-1155-compliant platforms, such as digital marketplaces or wallets, where accurate and up-to-date token metadata is critical. This also break the contract's integration with off-chain platforms like NFT marketplaces or block explorers may show stale metadata for the NFTs, and token holders can use this stale data to their advantage. Without the "URI" event, these platforms may not know about recent updates, resulting in outdated or incorrect information being displayed.
So, the GameItems
contract failure to emit the "URI" event upon updating the URI for tokenID in the GameItems.sol#setTokenURI() function results in non-compliance with the ERC-1155 standard. This oversight can lead to issues with ERC-1155 compatible marketplaces, wallets, and other platforms that rely on the URI event to track changes to token metadata. Without this event, external applications will not be aware of updates to the token's metadata, leading to outdated or incorrect information being displayed to users.
Cheers!
Acknowledged on points raised. There's not much else for me to add at this point, I stand firmly on the final decision I've made.
Acknowledged on points raised. There's not much else for me to add at this point, I stand firmly on the final decision I've made.
I understand you @HickupHH3, but you are not following Code4rena's severity categorization rules. 😌
The rules are meant to be followed, but that clearly doesn't apply here. Let's take a look at the big picture for code4rena contests and see how things play out after the rules aren't followed.
Have a good one!
Lines of code
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L194-L197
Vulnerability details
Impact
The
GameItems.sol
contract isERC1155
token and represents a collection of game items used in AI Arena. The contract implementssetTokenURI()
function that sets thetokenURI
for agame item
.However, this function make the
GameItems.sol
contract not fully compilable withEIP1155
, because in theEIP1155
standard documentation writes the following:But as we can see in the
GameItems.sol#setTokenURI()
function, there isn't emitting ofURI event
The
GameItems
contract's failure to emit theURI event
upon updating the URI for token ID in theGameItems.sol#setTokenURI()
function results in non-compliance with theERC-1155
standard. This oversight can lead to issues with ERC-1155 compatible marketplaces, wallets, and other platforms that rely on theURI
event to track changes to token metadata. Without this event, external applicationswill not be aware of updates to the token's metadata, leading to outdated or incorrect information being displayed to users
.Proof of Concept
The
setTokenURI()
function in theGameItems.sol
contract is intended to update the URI for a token ID, but it does not emit theURI
event as required by theEIP1155
standard. This event is crucial for notifying external observers of changes to a token's metadata URI.Tools Used
Manual Review
Recommended Mitigation Steps
To ensure full compliance with the ERC-1155 standard and improve the contract's interoperability with ERC-1155 compatible platforms, the
GameItems
contract should be modified to emit theURI
event whenever thesetTokenURI()
function is called and successfully updates a token's URI.Side Note:
URI
event is implemented in IERC1155 interface as following:Assessed type
Context