[BTA-01M] Incorrect Emittance of Event

[galimba]

BTA-01M: Incorrect Emittance of Event

Type	Severity	Location
Logical Fault		BaseTxAuthDataVerifier.sol:L147-L154

Description:

The NexeraIDSignatureVerified event will be emitted even if the signature verification eventually fails.

Impact:

As the impact of this exhibit is off-chain, its severity cannot be quantified above informational.

Example:

emit NexeraIDSignatureVerified(
    block.chainid,
    userNonce,
    blockExpiration,
    address(this),
    userAddress,
    argsWithSelector
);

/// Verify Signature
if (
    !SignatureChecker.isValidSignatureNow(
        _signerAddress,
        ethSignedMessageHash,
        _signature
    )
) {
    revert InvalidSignature();
}

return true;

Recommendation:

We advise it to be emitted after the ensuing if block to prevent off-chain software from triggering when signature validation has failed.

[joelamouche]

@omniscia-core We were thinking the same thing but then slither warned us that due to external isValidSignatureNow call, having the event after would be a reentrancy risk. See https://github.com/NexeraProtocol/NexeraIDSigGatingContracts/issues/61 Knowing this, do you still think the above risk is higher than the reentrancy risk?

[pash7ka]

To add more context here: The isValidSignatureNow() may (and probably will in our workflow) call an external contract. During execution of that contract it may so happen, that another gated function of our contract is called requiring to verify one more signature. So what is the correct order of nonces in this scenario? It seems logical, that the signature in the outer (initial) call should have first nonce, and the one which is verified during that verification - should be next. And since we increase nonce before signature verificalion this is the only order that will work.

We think, that order of emmited events should match the order of nonces, otherwise some backend which uses events to calculate correct next nonce may fail if it does not expect wrong order of nonces. At the same time, we think that events in reverted transactions should not be counted by any valid backend.

[joelamouche]

@omniscia-core agrees with us so we can close this