V2 review comments - Githubissues

jfschwarz commented 1 year ago

PR is not meant for merging, just has some code comments (you can also just ignore them)

Reviewed

[x] Core.sol
[ ] Decoder.sol
[ ] Integrity.sol
[x] Periphery.sol
[x] PermissionBuilder.sol
[x] PermissionChecker.sol
[x] PermissionLoader.sol
[x] Roles.sol
[x] ScopeConfig.sol
[ ] Topology.sol
[x] Types.sol
[x] WriteOnce.sol
[x] adapters/MultisendAdapter.sol
[x] adapters/Types.sol

Issues

none found

Optimizations

O1. WriteOnce.sol

store function uses create. It should use create2, so that identical parameter configs are stored only once (globally).

O2. WriteOnce.sol

store and creationCodeFor both use abi.encodePacked. As you pointed out, it can be reduced to a single call to save gas.

(We should compare notes, I ended up with this implementation for Mech: https://github.com/gnosis/mech/blob/main/contracts/libraries/Bytecode.sol.)

O4. PermissionLoader.sol / PermissionChecker.sol

Let's avoid doing this half-baked allowance check in _withinAllowance() and instead only record the consumption/trace and then rely on the final, proper check in PermissionBuilder's _track().

The check is half-baked, because it reads stale storage values in two scenarios:

if the call uses the same allowance multiple times
if the call is to the roles mod's exec function (reentrency)

This will allow us to get rid of the _loadAllowances function in PermissionChecker for some gas gains. We can also remove the allowance field from the ParameterConfig struct.

O5. PermissionChecker.sol

revertWith() should be internal or private.

O6. ScopeConfig.sol

packing (2 bits per param) could be killed. It's implicitly defined by the value of comparison, provided we do the following unification: We always hash compValues for EqualTo. And: We never hash compValue for anything else (which is already the case).

O7. Core.sol

Role assignments could be stored as a bitmap for minimizing storage used if an account is assigned multiple roles. Proof-of-concept on branch poc-assignment-bitmap, benchmark results:

# roles	gas used (current)	gas used (bitmap)
1	81067	77001
2	105030	77001
3	128993	77001
4	152956	77001
5	176931	77001
6	200894	77001
7	224858	77001
8	248821	77013

It makes the membership check a bit more expensive, by about 50 gas, though. So maybe let's just keep it like it is.

O8. Integrity.sol

We might consider expanding the integrity checks on condition trees (e.g. assert parent <= own id). As adding more checks will increase gas, we could add an unchecked variant for the scopeFunction, e.g. scopeFunctionUnchecked. That way people can run checks in advance via static calls and save some gas on the real apply.

Alternatively, we could consider removing all integrity checks and leaving this aspect entirely to the client (Roles SDK). This would be my preference.

Nits & Names

N1. General – Pragmas

Most contracts have pragma solidity >=0.7.0 <0.9.0;. We use custom errors which are supported only since v0.8.4 and some type conversions also are not allowed on earlier solidity versions.

Suggestion: update pragmas to pragma solidity >=0.8.17 <0.9.0;

N2. PermissionChecker.sol – Traces

Why do we need to track the entire ParameterConfig? It should be enough to track the allowance ID.

Trace.value is of type bytes32, it should rather be casted to uint256 directly.

"Trace" is not a good name. It took me a bit to realize that the sole purpose of traces is compiling a list of used allowances. I assumed it would "trace" the entire path through the scope config tree.

Proposal:

   struct Consumption {
       uint256 allowanceId;
       uint256 amount;
   }

   _consume(consumed, more);

_track(trace) could become _digest(consumed). It reverts 🤮 if too much, otherwise stores in sto~~mach~~rage.

N3. General – ScopeConfig/ParameterConfig

The names "scope config" and "parameter config" used somewhat interchangeably. Suggestion:

rename ParameterConfig to Condition
rename ParameterConfigFlat to ConditionFlat
rename library ScopeConfig to ConditionPacking

N4. PermissionLoader.sol / ScopeConfig.sol

The separation of concerns between these is a bit fuzzy. It could be sharpened by moving all functions concerned with packing and unpacking to the ScopeConfig (ConditionPacking) lib. This would leave the following functions in PermissionLoader: _store, _load, ~~_loadAllowances~~.

Alternatively, just move everything into PermissionLoader. (Name not optimal, because it not only loads but also stores.)

N5. Types.sol – ExecutionOptions (super nit)

In my taste, two separate booleans would be better:

boolean send and boolean delegatecall (1 bit each), instead of enum { None, Send, DelegateCall, Both } with 2 bits.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
zodiac-roles-app	❌ Failed (Inspect)		Mar 17, 2023 at 1:43PM (UTC)