safe-global / safe-modules / 9906147334 / 1

100%

master: 85%

push

github

Signature length check (#453)

Related to https://github.com/safe-global/safe-smart-account/issues/754

## Changes in PR

- Add a test case that verifies that UserOp is reverted if signature
contains additional bytes data
- Add `_checkSignatureLength` function in `Safe4437Module` contract .
This function retrieves threshold from Safe and iterates over signature
data.
- Add a test contract which is used in testing the internal function:
- Add tests
- Summarise `_checkSignatureLength` in FV specs. The prover was
reporting error without giving call traces.
- Unrelated to issue: [Fix
script](https://github.com/safe-global/safe-modules/pull/453/files#diff-d6fef544648a27dda677a99a74fcd6a44R24)
in `package.json`

## Open for discussion

Should `_checkSignatureLength` also do more stricter checks that are
already there in `checkNSignature` function of `Safe`? If
`Safe4337Module` does not perform below mentioned checks then it will be
implicitly assumed to be done in `Safe` contract and this creates a
requirement that all future `Safe` versions to have these checks if used
with this `Safe4337Module`.


1. `if (signatures.length < offset)` this check is currently done twice.
- In Safe4337Module:
https://github.com/safe-global/safe-modules/pull/453/files#diff-8ec9433e2997ad55ed210299abddb7908R223
- In Safe:
https://github.com/safe-global/safe-smart-account/blob/<a class=hub.com/safe-global/safe-modules/commit/8f80a8372d193be121dcdb52e869a258824e5c0f">8f80a8372/contracts/Safe.sol#L279
   
2. Other potential check that can be repeated:
https://github.com/safe-global/safe-smart-account/blob/8f80a8372d193be121dcdb52e869a258824e5c0f/contracts/Safe.sol#L233

## Code size change

### This PR

```
Safe4337Module 8910 bytes (limit is 24576)
```

### Main branch

```
Safe4337Module 8373 bytes (limit is 24576)
```

## Impact on gas usage

### This PR

```
  Gas Metering
    Safe Deployment + Enabling 4337 Module
  ... (continued)

24 of 24 branches covered (100.0%)

Branch coverage included in aggregate %.

51 of 51 relevant lines covered (100.0%)

42.96 hits per line