cergyk - Incorrect bitmask used in _swap will lead to ids in agents mapping to be corrupted about 2023-10-looksrare-judging

[0xhiroshi]

https://github.com/LooksRare/contracts-infiltration/pull/150

[mstpr]

Escalate

This issue is invalid. Agent ID is either 0 or already set a value. If the agent ID is 0 the OR here does the correct thing by setting the ID of the uninitialized agent. If the agent ID is already set then the or here does not have any impact.

Only use case
1- Say the agent ID in storage is 0 and we need to set it to 200

0 OR 200 = 200

Unnecessary assembly operation
2- Say the agent ID in storage is 60 and we need to set it to 60
60 or 60 = 60

this is unnecessary because lastAgentId is always equal to the last 16 bits of the agent ID. Doing an OR operation is unnecessary for the agent that already has initialized ID. As stated above, it only useful when the agentId is 0.

Regarding to this part:
lastAgentCurrentValue := and(lastAgentCurrentValue, not(AGENT__STATUS_OFFSET))
this part is completely unnecessary because we are 100% sure that the lastAgents status is indeed ACTIVE which is 00000000

you can comment this line in the code and run tests, nothing will change.

[nevillehuang]

Agreed, suggestion to make this issue invalid since gas findings are not accepted based on sherlock rules.

[sherlock-admin2]

Escalate

This issue is invalid. Agent ID is either 0 or already set a value. If the agent ID is 0 the OR here does the correct thing by setting the ID of the uninitialized agent. If the agent ID is already set then the or here does not have any impact.

Only use case
1- Say the agent ID in storage is 0 and we need to set it to 200

0 OR 200 = 200

Unnecessary assembly operation
2- Say the agent ID in storage is 60 and we need to set it to 60
60 or 60 = 60

this is unnecessary because lastAgentId is always equal to the last 16 bits of the agent ID. Doing an OR operation is unnecessary for the agent that already has initialized ID. As stated above, it only useful when the agentId is 0.

Regarding to this part:
lastAgentCurrentValue := and(lastAgentCurrentValue, not(AGENT__STATUS_OFFSET))
this part is completely unnecessary because we are 100% sure that the lastAgents status is indeed ACTIVE which is 00000000

you can comment this line in the code and run tests, nothing will change.

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.

[0xhiroshi]

https://github.com/LooksRare/contracts-infiltration/pull/175

[nevillehuang]

Agree with escalation, seems like issue is invalid. Any thoughts @0xhiroshi given u implemented a fix?

[0xhiroshi]

Agree with escalation, seems like issue is invalid. Any thoughts @0xhiroshi given u implemented a fix?

It is still an issue, but the severity should be lowered as it only wastes gas instead of breaking the game

[Evert0x]

Will accept escalation and make invalid as submitter agreed to low/invalid in DMs.

[Czar102]

Result:
Low
Unique

Submitter agreed to low/invalid in DMs.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• mstpr: accepted

[SergeKireev]

Fix LGTM