0 byte/payable selector bug

[startswithaj]

There is a bug where selector tracking is broken when adding and then removing the payable selector 0x00000000. For this issue to occur the 0x00000000 selector has to be added to a new row, and then removed.

Only effects facet-> selector tracking and does not/cannot effect selector -> facet.

Please see SolidStateDiamond.behavior.ts for POC. And DiamondWritableInternal.sol for a potential fix.

// The diamond example comes with 8 function selectors
// [cut, loupe, loupe, loupe, loupe, erc165, transferOwnership, owner]
// This bug manifests if you add the payableSelector(0x00000000) to the 1st slot in a fresh row and remove it
// We Add 8 more random selectors to the diamond, then add the payable selector to the 1st slot in a fresh row
// [cut, loupe, loupe, loupe, loupe, erc165, transferOwnership, owner]
// [rand1, rand2, rand3, rand4, rand5, rand6, rand7, rand8]
// [payableSelector]
// We then remove the payable selector from the diamond, this causes malfunction in the slot tracking, 
// And `rand8` is lost from the selector tracking

If an already deployed Diamond suffers from this issue, whereby a selector is missing from a facet mapping, you must Remove the selector and Add it again. Replace on the same facet will not fix the mapping.

If you want to check if a Diamond Suffers from this bug, you get a list of the selectors you expect to be on each facet and compare them to the output of diamond.facets(). You can use the event history of a diamond to calculate which selectors should be exposed on each facet.

This does not affect the onchain behaviour of a diamond.

closes #60

[ItsNickBarry]

Linking #60 because I believe it's related.

[ItsNickBarry]

Invert the logic and revert the order of the if/else blocks:

Suggested change

	if (
	selectorSlot != 0 ||
	(selectorInSlotIndex != 0 && selector == 0)
	) {
	selectorInSlotIndex--;
	} else {
	selectorSlotCount--;
	selectorSlot = l.selectorSlots[selectorSlotCount];
	selectorInSlotIndex = 7;
	if (
	selectorSlot == 0 &&
	(selectorInSlotIndex == 0 || selector != 0)
	) {
	selectorSlotCount--;
	selectorSlot = l.selectorSlots[selectorSlotCount];
	selectorInSlotIndex = 7;
	} else {
	selectorInSlotIndex--;

Makes the diff easier to reason about, and == is fundamentally easier to reason about than !=.

[ItsNickBarry]

Also I'm rather confident that the selector comparison is redundant. And I'm somewhat confident that selectorSlot comparison is also redundant. Isn't the selectorInSlotIndex comparison all we need? Tests pass if I remove the others.

Like this:

if (selectorInSlotIndex == 0) {
    selectorSlotCount--;
    selectorSlot = l.selectorSlots[selectorSlotCount];
    selectorInSlotIndex = 7;
} else {
    selectorInSlotIndex--;
}

[startswithaj]

Yeah, I think you are definitely right about this. Updating.

[ItsNickBarry]

I made the above changes after merging the diamond-cleanup branch and pushed them here.

[ItsNickBarry]

@startswithaj I changed the base of this PR to diamond-cleanup, and merged those changes here.

[startswithaj]

Should I merge this?

[ItsNickBarry]

@startswithaj Okay to merge as long as you're okay with the new changes I've pushed. Let's also wait 12 hours so that @NouDaimon can analyse the situation.

[NouDaimon]

Fix looks good to me.