The advantages of this alternative design are:

It’s a simpler life cycle. Nobody can make a user an applicant without their consent. Users can approve the Moloch DAO just once. It can be implemented in a way that the proposal deposit gets collected from the members, and not their delegates. See [MOL-O07] below.

Update from Moloch: We agree that this is an important vulnerability and that the proposed alternative design is sound. As stated in MOL-L02 , in the interest of time we decided not to implement these changes for this version of the contract, but plan to in the future. We did however document this vulnerability prominently in the readme.

Other comments & recommendations

[MOL-O01] GuildBank#withdraw emits Withdrawal on failure

This function should only emit an event when it can successfully call approvedToken.transfer , but it does it unconditionally.

This has no real impact in the system, given that it’s only called from the Moloch DAO, and with a well behaved ERC20 as approvedToken .

Update from Moloch: This actually will not emit the event on failure because the GuildBank.withdraw is wrapped in a require when it is called from the Moloch contract. If the token transfer fails, this function will return false, causing the require in Moloch to fail, and the event emission to be reverted.

[MOL-O02] Usage of OpenZeppelin’s ERC20 instead of IERC20

Both contracts, Moloch and GuildBank , use OpenZeppelin’s ERC20 as an interface, but that contract is their own implementation of the standard. IERC20 should be used instead.

Update from Moloch: fixed in commit acbee32057225ca210013e7ba217e37b73cb6f42

[MOL-O03] The dilution bound functionality is confusing

Moloch#processProposal imposes a “dilution bound” with the intention to protect members from excessive dilution in case of a mass ragequit.

The mechanism implemented successfully protects the users in such cases, but it’s not a bound of the dilution, as it ignores the number of shares requested. We recommend renaming it.

Update from Moloch: We kept this as-is because we couldn’t think of a better name. We feel that the current name fits because the parameter bounds the total dilution you would be expected to suffer if the proposal were to pass.

[MOL-O04] The README.md file is outdated

We recommend not duplicating that much information in the README file, especially code, as it gets outdated very easily.

Update from Moloch: Fixed, but we still like keeping code in the README.md. Now that the contracts are done it shouldn’t get outdated anymore!

[MOL-O05] Member’s isActive field has a confusing name

This field seems to imply that the member has shares and can participate in the voting process, but that’s not the case. It only signals that the member has been active at any point in time.

Update from Moloch: Fixed. We changed it to exists .

[MOL-O06] onlyMember and onlyDelegate have confusing names

These names can cause confusion, as they check for members with shares, not just any member. An example of confusing behavior because of this is [MOL-L02] .

Update from Moloch: We kept it as is. If you no longer have at least 1 share, we don’t consider you a member.

[MOL-O07] Moloch#submitProposal collects the proposalDeposit from delegate instead of proposer

This function has the following comment:

collect proposal deposit from proposer and store it in the Moloch until the proposal is processed

but collects the deposit from the msg.sender , which is the proposer’s delegate, which is not necessarily the member.

We recommend documenting this behavior. As an alternative, the deposit could be collected from the actual member, but that would imply using their key more often, as submitting a proposal requires a token approval transaction.

If the design explained in [MOL-L03] were to be implemented, this function could collect the funds from the member, without making it less secure.

Update from Moloch: This is true, and we pull the deposit from the delegate instead of the member address to avoid needing extra interactions with the member key. We kept it as is for now, but when we implement the recommendations from MOL-L03 we will reconsider allowing drawing deposit funds from the member address.

[MOL-O08] Period 0 used as default highestIndexYesVote can be confusing

When adding a new member, highestIndexYesVote is set to 0 . This makes the Moloch behavior somewhat different during the first votingPeriodLength + gracePeriodLength periods.

This doesn’t affect any member except for the summoner, but we recommend documenting why it’s safe.

Update from Moloch: Good catch. Added a note about this in the readme.

[MOL-O09] Duplicated logic for adding members

The constructor and processProposal have duplicated logic for adding members. We recommend extracting a function with it.

Update from Moloch: Kept as is in the interest of time.

[MOL-O10] Submitting a uintVote greater than two throws an exception

We recommend checking this and reverting with a nicer error message.

Update from Moloch: Fixed.

[MOL-O11] Resetting a member’s delegate key in processProposal is unexpected

We find this behavior unexpected and somewhat hard to reason about. We evaluated some alternatives and the following proposal to be the easiest to implement without opening the door to potential denial of service attacks.

We recommend adding an event so that members can know when their delegate keys have been reset.

An interesting alternative to consider is to let addresses be delegate keys of multiple members. This would require an extra parameter in submitProposal and submitVote , indicating which member is being represented but would simplify reasoning about the system.

Update from Moloch: Kept as is in the interest of time.

[MOL-O12] Overflow checks can be confusing

Moloch#submitProposal checks that the number of shares requested doesn’t overflow and lock the submissions processing. This is done by computing a bound on the number of shares with safeMath , in a way that it will revert the transaction if an overflow happens. This is not very evident, especially because it’s done inside a require.

Update: While writing this report, PR #11 was opened, which implements clearer and more exhaustive overflow checks.

[MOL-013] Incorrect error messages

The constructor has the following require with an incorrect error message:

require(_abortWindow <= _votingPeriodLength, “Moloch::constructor — _abortWindow must be smaller than _votingPeriodLength”);

Moloch#abort has this require with an incorrect error message:

require(approvedToken.transfer(proposal.applicant, tokensToAbort), “Moloch::processProposal — failing vote token transfer failed”);

Update from Moloch: Fixed.

[MOL-014] Some require are repeated multiple times

Some like require(proposalIndex < proposalQueue.length, “Moloch::abort — proposal does not exist”); are repeated multiple times. They are mostly array index checks. We recommend creating getters for those array elements.

Update from Moloch: Kept as is in the interest of time.

[MOL-015] State guards are not always clear

Each of the Moloch DAO functions has several requires to validate that they are only called in the correct state of the proposal they refer to. We recommend moving this logic into boolean functions with clearer semantics and names.

For example, submitVote would have a single require checking the return value of isVotable(proposalIndex) . Similar functions would be implemented for each or most of the states shown in the diagrams in [MOL-L03] .

Update from Moloch: Kept as is in the interest of time.