Audit fixes

[ipatka]

Safe Minion Audit Rev

NOTE: items below are from an external review of Safe Minion. Using this issue to track public response and implement recommended updates

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.

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

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

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

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

11. 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.

[ipatka]

1 - implemented

2 - I don't see how these can be updated through a delegate call. The delegate call is only done from the Safe itself. And executeAsMinion does not have delegate call functionality

[ipatka]

3 - marking the action as executed was done so as a (probably redundant) re-entrency protection. The concern was there could be unexpected behavior if part of the multicall transaction bytes included a re-entrant call back into the safe, calling executeAction again

[ipatka]

4 - changing - target looks very useful for future chained conditionals

For safe minion it would would probably cause issues if target and avatar are set to different addresses, but no worse than the current chances for things to go wrong by a DAO calling setAvatar or transferOwnership

[ipatka]

owner:

• Can set avatar, target, delete actions, execute as minion

avatar:

• can delete actions, trigger execution as minion

target:

• module that can act as an intermediary between minion and safe

Usually, owner, avatar, and target are all the Safe

[ipatka]

5 - This allows us to execute early if quorum is reached. often tokens are not required to be allocated for a proposal to be executed

I actually think the noVotes clause can be removed because it only exists to allow for rage quit/ grace period, and that protection is already baked into the fact that doWithdraw can only be done after full proposal time @dekanbro

[ipatka]

6 - yea i agree his is a griefing vector but not a big vuln. Also this is probably a vector someone could use to force tokens back into the DAO from the safe, bypassing governance

[ipatka]

7 - removed action.moloch - good gas savings changed amount to nonZeroAmount

[ipatka]

8 - good find, i think I like this functionality thought. Anything that involves moving tokens out of the DAO is probably ok to gate behind memberOnly. thoughts @dekanbro ?

[ipatka]

9 - moved to a local var

[ipatka]

10 - changed minionCount to a view function of list length

[ipatka]

11 - updated to use more concise deployModule function and added quorum to salt

[ipatka]

6 - yea i agree his is a griefing vector but not a big vuln. Also this is probably a vector someone could use to force tokens back into the DAO from the safe, bypassing governance

successfully tested the griefing exploit on the minion, and implemented a fix w/ new tests in this commit:

https://github.com/HausDAO/MinionSummonerV2/pull/21/commits/d6c80d6c2d2d400ee143730da1e564e24a413590

i am now checking that _moloch != moloch which prevents the front-run grief

also checking that safe token balance after is >= safe token balance before which catches the dummy moloch scenario