Safe Minion Audit Rev

  1. All state variables in SafeMinionSummoner except minionList, minionCount and minions can be immutable since they can't change after deployment anyway

  2. Same probably goes for some variables in SafeMinion even though in that case they can be changed through a delegate call

  3. The Action.executed flag is redundant since the action is deleted right after it's set to true anyway, instead the action can be deleted in executeAction on the same line executed is currently being set to true

  4. I noticed the contract is built on an older version of the zodiac Module contract, for example the target variable is missing

  5. I'm not sure I understand the intent behind implementation of isPassed , for example the minQuorum is only considered before the proposal is processed and ignored after and if the minQuorum is reached, the action can be executed before processing, which also means before the necessary tokens have been allocated, also not clear on why noVotes < 1 is part of the condition (and why it isn't expressed in a clearer form of noVotes == 0)

  6. crossWithdraw effectively allows two things:

  • any member can make the avatar call withdrawBalance(uint256, uint256)on an arbitrary address
  • any member can make the avatar call transfer(moloch, uint256) on an arbitrary address

and both calls can be made basically independently, the second one by calling withdrawBalance on a dummy address that the member owns
I'm not sure allowing this is wise, imagine a situation when a proposal has been passed and one member wants to sabotage the call. They can simply frontrun the executeAction call with their own call to crossWithdraw and withdraw and immediately return the requested payment to the moloch DAO, which will make the call fail.

  1. Action.moloch is never read, Action.amount is only checked for being larger than 0 so could be replaced by boolean value.

  2. doWithdraw call in executeAction introduces memberOnly requirement even to many calls that are not memberOnlyEnabled

  3. getUserTokenBalance is unnecessarily called twice with the same arguments in executeAction

  4. SafeMinionSummoner.minionCount state variable doesn't seem necessary, since SafeMinionSummoner.minionCount.lenght will always have the same value and can be made accessible.

  5. In summonMinionAndSafe both safe and minion salt is missing the _minQuorum argument which affects behavior of the contract that will be deployed on the deterministic address, in case of safe the minion address in salt could be replaced by the _moloch address which would allow reordering the calls and using the more concise deployModule call to deploye the minion contract.

Select a repo