# Daohaus <> Dili
6-26 spot check audit notes

## Things to focus on
(priority list)
1. Minions (Section 1)
* Escrow minion
* Conditional Minion
* ERC20ZapMInion
* Hash Multicall example
2. Baal Dao Contract (Section 2)
* Below is an initial Review by Issac that might help with context
* places to focus on if time permits:
* submit proposal function, especially period proposals, i.e. to change DAO parameters,
* member action function (special whitelisted contract calls)
3. Section 3 has further notes and the start on some user stories for baal

# Minion User Stories: Section 1

These are for Moloch V2 minions. an initial version of this was looked at in the last one dayer, basically same pattern.

Minion is a bespoke contract from Moloch v2, it allows ACE by verifying that a proposal has passed before executing a stored data hash.

Contracts use a EIP-1167 factory patern

Moloch v2.1 repo https://github.com/HausDAO/Molochv2.1

### Escrow Minion

https://github.com/ipatka/minions/blob/master/contracts/EscrowMinion.sol

This Minion enables a prospective member to exchange an NFT for shares in a Moloch V2 DAO.

#### Propose

* Approve the minion to transfer NFT on behalf of applicant
* Minion should have IERC721 recipient interface to safely receive NFT
* During propose action, NFT moves to escrow contract
* Applicant should be able to specify destination address for NFT post escrow if proposal is successful. Usually will be the DAO's NFT bank

#### Cancel

* Before a proposal is sponsored, the applicant should be able to cancel the proposal and receive their NFT in the same transaction

#### Vote

* Traditional moloch v2 voting

#### Execute

* Once voting period is done and proposal is processed, Minion action has to be executed
* If proposal has passed, NFT goes to destination
* If proposal has failed, NFT goes back to applicant

### Conditional Minion

https://github.com/ipatka/minions/blob/master/contracts/ConditionalMinionSummoner.sol

This Minion enables a Minion's execution to be tied to an arbitrary on chain condition. A DAO may want to approve an action but only allow it to execute if preconditions are met.

For example, A member may wish to propose a buyout for their shares in an amount additional to their proportional rage-quittable balance. If the DAO approves, additional funds will be sent to an escrow prior to the member quitting. However the DAO would not want the member to be able to withdraw funds if they don't end up quitting. Therefore the buyout proposal would include an execution dependency of the recipient having 0 shares.

#### Propose

* Calculate the target action data and the target condition
* Submit all data with the minion proposal

#### Vote

* Traditional moloch v2 voting

#### Satisfy Condition

* External actions are taken to satisfy the Minion execution condition

#### Execute

* Only if external conditions are satisfied, execute the action.

#### Failure

* If the condition is unreachable, allow the DAO to clawback any funds

### ERC20ZapMinion
https://github.com/HausDAO/ERC20-Zap-Minion

This one has no tests or docs but a second pair of eyes would be nice on the constructor and upgrade function.

### Hash and multicall Minion

This is more an example of a pattern we may using going forward to support multicall and use less storage. THis seems like a pretty standard pattern just want to see if we are implimenting ok?

https://gist.github.com/dekanbro/a5d70f3401fee4376c1b6ff7855429bc#file-minion2-sol-L260

Main idea:
On proposeAction hash the target and data then save to the action struct

and on executeAction require the hash matched

so you pass the main values to both proposAction and executeAction

        address[] calldata actionTos,
        uint256[] calldata actionValues,
        bytes[] calldata actionDatas


## Section 2: Baal
working branch
https://github.com/Moloch-Mystics/Baal/tree/checkpoint/dili-628

## Isaac's Baal review

Baal is a moloch inspired contract with the following new features compared to V2:

* Early proposal execution if solo member or super majority
* Relayed delegation - using EIP712 sigs for setting delegates
* Relayed share transfers - using EIP712 sigs for permitting share transfer
* Share and loot mainpulation by custom external contract
* Adjustable min & max voting periods
* Integrated external action proposals

**Scope:** This review is a spot check of the core logic, some suggestions, questions, and some hopefully helpful info for other reviewers.


## Member Actions

The design pattern for member actions leads me to the following understanding of the user journey.

1. Users interact directly with Shamans
2. Interactions with Shamans results in a state that can be queried by Baal to see how shares should be updated
3. Member interacts with Baal via member action and the Shaman tells Baal how to adjust shares for member
4. Baal executes share/ loot changes

QUESTION - Should we allow Shamans to return abitrary actions for processing as well? This would enable them to spend funds while also adjusting shares.

QUESTION - Why specify shares & loot in the call to Shaman rather than a proposal ID style system?

QUESTION - Should we also make delegated memberActions possible via the same pattern as relayed delegation & relayed share transfers?

### Use cases for member actions/ shamans

* Guild kick, rage quit could be implemented as a shaman which would allow a DAO to decide if they want these features
* open enrollment DAO with fixed share price
* integrated buyout minion - basically rage quit with modified fair share calculator
* integrated nft tribute without escrow step

## Shares vs Loot

There are some discrepancies in how shares and loot are handled which may be unnecessarily confusing or limiting.

Shares represent voting interests while loot represents non-voting interests.

* Shares are tracked in the balance storage, while loot is tracked in the member struct. 
* Shares can be transfered with approvals and permits while loot can only be transfered by the holder
* Loot cannot be direcly requested in a proposal. A member has to request shares and then include data to convert some shares to loot (correct if wrong please)


## Vote Tallying

This is my understanding of vote tallying which may be useful to future reviewers.

Votes are tallied according to the state of the voter's shares & delegated shares at the block timestamp when the proposal was submitted.

Votes use a checkpoint system to check what the shares were at the time of proposal submission.

Checkpoints are updated whenever shares are created/ transferred.

The tallying system works in the following order:

1. If the latest checkpoint is before or equal to the start block, use that one
2. If the first checkpoint is after the proposal start time, no votes counted
3. Search for the checkpoint closest to the start block, or the one just before it

There may be a risk if too many checkpoints are created after a proposal starts, the calculation for the proper checkpoint could get too expensive.

Also, vote sponsoring is currently missing.

### Delegation

Members can delegate their full shares to another member. This triggers `moveDelegates` which updates the checkpoint. Partial delegation is not possible

## Membership proposals

Membership proposals can be used to issue new shares or convert shares to loot. It appears that there is no way to issue loot directly, but instead you have to issues shares and then immediately convert them to loot. The contract infers whether to issue or convert based on the contents of proposal data.

## Whitelist proposals

Whitelist proposals are used to approve or disable minions or guild tokens.

It appears that a token can be removed from the whitelist even with nonzero balance. This could have unintended consequences for rage quit.

Also, the whitelist proposal makes assumptions about the type of proposal based on whether value, data, and to are present in the proposal. Perhaps this should be made more explicit.

## Action proposals

Action proposals store targets, arbitrary call data, and values in ETH to be used in those external calls. This is used to tie DAO governance to interactions with any external contract.

QUESTION - Is the 10 action count a tested maximum or based on assumptions about execution complexity?

QUESTION - Are funding proposals replaced by action proposals? any funding proposal could just be implemented as an action

## Period proposals

These are used to adjust the min, max voting periods and grace periods.

## Rage quit

User specifies amount of loot and shares to burn. Looks like solidity 0.8 integrated underflow checkers are used to make sure the amount is valid.


Delegated shares are also burned

## Signature Nonces

The EIP712 domains contain nonces. These nonces strictly increment which enforce that signatures are submitted and executed in the order specified by the user. However users have no way to cancel signatures.

## Suggestions

### Signature Nonces

We should add a way, via a 3rd type of EIP712 signature, for a user to sign/ invalidate a nonce so that nonce holes can be filled, or they can cancel a signature. Similar to 0 value transactions in the ETH mempool.

### Conditional Execution

We can add an additional property to proposal actions which would restrict a proposal from being processed until some arbitrary external conditions are set. This can be done using fields `conditionTarget`, `conditionData`, `expectedCondition`. When a member attempts to process a proposal, the `conditionTarget` would be called with `conditionData` and only if the return data matches `expectedCondition` would the processing be allowed.

### Make Shamans even more poweful

Allow Shamans to return data similar to an action proposal for Baal to execute while also adjusting shares.

### Align shares and Loot accounting

We could use the same accounting methods for shares and loot and eliminate some confusion

## Missing Comments

Missing documentation on the following things:

* Proposal `to`, `data`, and `value` have different meanings depending on if it is an action, whitelist, membership, or period proposal. This isn't mentioned in the proposal struct comments
* The vote tallying section would benefit from some comments on why the search tree functions the way it does to find the checkpoint closest to the start of the proposal
* User story for Member Action should be detailed more


# Baal User Stories and other notes - Section 3
### summon
* Should be able to summon a new dao, 
    * Should take a list of member addresses and a list of share amounts. 
    * Should be able to take a list of approved (whitelisted) tokens.
    * Should take a max and min voting period
    * Should be able to set a share symbol and name
    * Should be able to pause transferability of shares or loot and should be paused by default

### new member proposal
* New member should be able to make a proposal for shares
* New member should be able to make rewards

### request custom share/loot mint or burn (via `memberAction`)

### new period proposal
- DAO wants to change its voting period parameters
- DAO wants to make shares transferrable
- DAO wants to make loot transferrable

### new whitelist proposal
- should be able to whitelist a contract to be used in member action

### sponsor proposal
- *missing, this needs to be added*

### vote on proposal
- should be able to vote once (yes, no or abstain), unless shares are delegated.

### process proposal
* should be able to process proposal period has finished a
* should be able to process if more than 51% voted yes

### ragequit
* should be able to ragequit unless voted yes on a current proposal

### guildkick
- *missing, this needs to be added*

### delegate shares (votes)
* should be able to delegate all shares to another member for vote power

### approve shares for transfer

### transfer shares

### transfer loot


## Baal Walkthrough and Dili Meeting Notes 6-25

### DAO contract (WIP)
Moloch Baal
https://github.com/Moloch-Mystics/Baal

working branch
https://github.com/Moloch-Mystics/Baal/tree/checkpoint/dili-628

#### video walk throughs (3hrs)
the 3hrs of moloch goodness from the walkthrough. We really go on a lot of tangents and people inject their opinion all over the place.

todo - get some notes and timestamps

1hr Part 1: https://youtu.be/vKbhiDPBNXc
2hr Part 2: https://youtu.be/yN5iHE1GA5Y

things to do for us:
* User journey - what are the key paths a user will take when making a proposal and how will the proposal flow through the contract



## other stuff and notes from the dili review meeting
* Key things for focus - what should be doable and what should be not allowed
* key risk areas - what areas do we want special attention
    * delegation, esp. interaction w/ ragequit
    * period proposals, i.e. to change DAO parameters
* priority list - what things a really important to get a second pair of eyes on
* Documentation

* A few things they had initial questions on:
* I think we went over this in the walkthrough but could not remember the answers
* more info abi.encodeWithSelector(0xff4c9884...
* why using unchecked{}

 
* list of commit hashes or branches to review
* Review starts monday morning  European time