Alligator Roast 🔥

alligator-roast.md

Note Thanks for the chance to work on something interesting!

Source: https://github.com/w1nt3r-eth/liquid-delegator

🦭 Overall architecture:

• Test Coverage is unknown as foundry coverage fails with the next error:

  Compiler run failed
  CompilerError: Stack too deep when compiling inline assembly: Variable value0 is 1 slot(s) too deep inside the stack.
  

  Consider fixing this as it is not clear if all the logic branches have been test-covered.
• Proxy contracts are usually working on top of delegatecall instruction, but this Proxy is not using its storage to execute external code. So in fact it's not a proxy.
• Generally, Proxy and Alligator contracts create a Clone Factory pattern, where Alligator is an implementation and Proxy is a clone. I recommend to use OZ Clones implementation, or even better, use its modification with Immutable Args (https://github.com/wighawag/clones-with-immutable-args).

😵‍ Confusing code:

Alligator.sol:

• L303: RefundableVote event should probably not being emitted within the _refundGas function, and instead being used at L177.
• L231: subDelegateBatch should call subDelegate inside to not repeat the logic, so make subDelegate public and use it in the batch method.

OnlyEthLessThan100.sol:

• L8: voter and support arguments of validate(...) function are not used anywhere. Consider dropping the argument names leaving just argument types like this:

  // before
  function validate(address governor, address voter, uint256 proposalId, uint8 support) ...

  to this:

  // after
  function validate(address governor, address, uint256 proposalId, uint8) ...

👷🏻‍♂️ Gas optimizations:

Alligator.sol:

• L226-L243: Function subDelegateBatch has a logic on top of subDelegate which creates a proxy. The proxy is not used in the latter business logic, so consider dropping it.
• L237: Revert as soon as possible

🫣 Security concerns:

Alligator.sol:

• L191: Please use OZ implementation of ECSDA.sol or Solady variant (https://github.com/Vectorized/solady/blob/main/src/utils/ECDSA.sol). If you want to be IERC1271 compliable use OZ SignatureChecker.sol library. Using ecrecover could be dangerous when used along with short signatures (EIP2098).
• L193-195: signatory == address(0) check is insufficient because chance it happens a 1 in 2^256. Most likely incorrect or malformed signature would make a signatory a pretty entropy-filled-looking-like address.

😃 UX:

• Use clones, don't mix things with proxies as in fact they're not proxies. No delegatecall = No proxy.

🙌 Opportunities:

• Pure functions can be separated in another library, i.e. min at Alligator.sol:L307.

😇 Open-ended questions:

• I don't really understand what the gas refunds are for here and which actor will fill the ETH inside. Why the gas is only refunded in a single function Alligator.sol:L166?
• I haven't seen the usage of OnlyEthLessThan100.sol in tests, but why not making a general one that can check not only for 100 ether but for any amount?