ABI: Event selectors do not distinguish indexed arguments

[frangio]

Event selectors are calculated as the hash of the event name and the canonical types of its arguments:

topics[0]: keccak(EVENT_NAME+"("+EVENT_ARGS.map(canonical_type_of).join(",")+")") (canonical_type_of is a function that simply returns the canonical type of a given argument, e.g. for uint indexed foo, it would return uint256).

(Source)

This means that the "indexedness" of its arguments is not reflected in the selector. Put another way, each event selector maps to many ways of decoding a log, and in general there is no way to know which is the correct one. (This is true regardless of this issue because of hash collisions, but that's a separate thing, and I believe this is more serious.)

For example, the logs corresponding to Foo(address a, address indexed b) would be indistinguishable from Foo(address indexed a, address b). I think this incompatibility should be visible in the event selector.

A related problem is that, as of Solidity 0.4.24, redefining an event of a parent contract with different indexedness will not results in errors or warnings.

If we agree that this is a problem with the ABI, we may want to tackle it for 0.5.0, since the fix will probably be backwards-incompatible.

[begelundmuller]

Relating to the related problem, see https://etherscan.io/address/0xff1f9e2e0dcbaaa85d9320bca4d82d619eec531e#code. It redefines Transfer without indexed parameters. The Transfer event emitted contains no indexed data, however, the ABI says two of the fields are indexed. Clearly a bug.

[axic]

As a note, in the language they are indeed considered clashing:

contract C {
  event Approve(address a);
  event Approve(address a, address b);
  event Approve(address indexed a);
}

eventsel.sol:2:3: Error: Event with same name and arguments defined twice.
  event Approve(address a);
  ^-----------------------^
eventsel.sol:4:3: Other declaration is here:
  event Approve(address indexed a);
  ^-------------------------------^

The naive change is to include indexed as part of the selector hash. This would mean that any event not containing indexed arguments would be unaffected, but those which do, would break fully.

[frangio]

@axic That's true but shouldn't the same happen with derived contracts? This compiles without warnings:

contract C {
    event Approve(address a, address b);
}
contract D is C {
    event Approve(address indexed a, address b);
}

[axic]

I think that is a distinct bug. In a lot of cases indexed was not handled correctly, probably this has the same root. Can you create an issue for it?

The proper review how inheritance should work will be done during the "inheritance review" after 0.5.0.

[Marenz]

The naive change is to include indexed as part of the selector hash. This would mean that any event not containing indexed arguments would be unaffected, but those which do, would break fully.

Seems easy enough for me. Is this the way we want to solve this issue?

[axic]

The safe solution to this would be #11819.

[ryanio]

ran into this error testing ERC721 token contracts in foundry with two different Transfer events, the second has the last parameter as indexed, I cannot test for both without compilation errors

// one contract emits:
event Transfer(address indexed from, address indexed to, uint256 value);

// the other emits:
event Transfer(address indexed from, address indexed to, uint256 indexed id);