Prevent execution with invalid signatures

[code423n4]

Handle

gpersoon

Vulnerability details

Impact

Suppose one of the supplied addrs[i] to the constructor of Identity.sol happens to be 0 ( by accident).

In that case: privileges[0] = 1

Now suppose you call execute() with an invalid signature, then recoverAddrImpl will return a value of 0 and thus signer=0.
If you then check "privileges[signer] !=0" this will be true and anyone can perform any transaction.

This is clearly an unwanted situation.

Proof of Concept

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/Identity.sol#L23-L30

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/Identity.sol#L97-L98

Tools Used

Recommended Mitigation Steps

In the constructor of Identity.sol, add in the for loop the following:
require (addrs[i] !=0,"Zero not allowed");

[Ivshti]

duplicate of #2

[Ivshti]

resolved in AmbireTech/adex-protocol-eth@08d0506

[GalloDaSballo]

This seems to be the risk of having erecover returning zero, any invalid signature ends up being usable from any address to execute arbitrary logic.

Mitigation can be achieved by either reverting when about to return address(0), which the sponsor has used for mitigation

The other mitigation is to ensure that an account with address(0) cannot have privileges set to 1

I believe mitigation from sponsor to be sufficient, however I'd recommend adding a check against having address(0) in the constructor for Identity.sol just to be sure

[Ivshti]

@GalloDeSballo an extra check is superfluous IMO, not only cause the revert on 0 in SIgnatureValidatorV2 guarantees that this is fixed, but also because it has to be in three places: constructor, setAddrPrivilege and the account creation system in js/IdentityProxyDeploy which rolls out bytecode that sstores privileges directly