# G0 baal contract review

1) Using tx.origin to authenticate sponsorship seems like a security vulnerability because user transactions can be hijacked by malicious contracts.
https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/Baal.sol#L328
Plus if the sponsor is tx.origin it won't match sponsor that is saved with the proposition:
https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/Baal.sol#L354

**resolution:**
https://github.com/HausDAO/Baal/pull/89/files#r1022971419

> because this introduces some risk I think it is fine to remove this for now. Without this all proposals that use the same pattern of TributeMinion would always require sponsorship

---

2) What is the purpose of the grace period? Relatedly, some dao implementations automatically extend proposal deadline if quorum is reached too close to the original deadline, for example: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/governance/extensions/GovernorPreventLateQuorumUpgradeable.sol
Might be useful, but it's not a 100% necessary function.

**resolution:**
Grace is the period that delays execution of all proposals, it gives an opportunity for members to exit the dao (ragequit) with their fairshare of the treasury

---

3) It would be nicer if this line: https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/Baal.sol#L487
was put immediately after this line:
https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/Baal.sol#L472
even though the external calls in between are most likely safe.

**resolution:**
https://github.com/HausDAO/Baal/pull/89/files#r1022979960

---

4) Is this because Loot and Shares balances are summed together?
https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/SharesERC20.sol#L56
If that's the case shouldn't an analogous condition be in LootERC20.mint() ?

**resolution:**

yes this needed to be in both Shares and Loot. also made it a require as per suggestion

https://github.com/HausDAO/Baal/pull/89/files#r1022982689

---

5) It seems a bit weird to me that users that ragequit can still vote. Let's say there's a malicious proposal that harms the DAO, users can ragequit in the first block of prop.votingStarts to ensure their shares/loot are not counted towards prop.maxTotalSharesAndLootAtYesVote and then vote for the proposal with no consequence.

- With the added bonus of decreasing absolute quorum requirement.

- But even allowing voting for a proposal and then ragequitting seems weird.

- maxTotalSharesAndLootAtVote should be initialised at the moment proposal is sponsored.

**resolution:**

because voting is handled by checkpoints at the time a proposal is created it is hard to get around this. But we can help with the quorum 'slip' by snapshoting the high water mark of total supply on all votes not just yes votes.

https://github.com/HausDAO/Baal/pull/89/files#r1022995497

also snapshot at proposal sponsor
https://github.com/HausDAO/Baal/pull/89/files#r1024381347
on selfSponsor
https://github.com/HausDAO/Baal/pull/89/files#r1024395378

---

6) There's an interesting DoS vector where proposer can set a very high prop.baalGas to make a proposal unprocessable. If this proposal will have more "yes" votes than "no" votes by the time voting ends, regardless of whether or not it reaches quorum, this proposal will basically jam the DAO making execution of any further proposals impossible.

**resolution:**

hard coding an upper limit to of 20M baalGas will keep someone from using something (on purpose or accidentally) which could cause a deadlock if passed
https://github.com/HausDAO/Baal/pull/89/files#r1023001984

---

### Here are some possible optimizations:

- This is already implied by ProposalState.ready https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/Baal.sol#L546

**resolution:**
there is some logic happening after the ProposalState.Ready check which updates the okToExecute.

** removed redundant check
https://github.com/HausDAO/Baal/pull/89/files#r1023267472

---

- This call should be skipped if tokens[i] == ETH
https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/Baal.sol#L652

**resolution:**
https://github.com/HausDAO/Baal/pull/89/files#r1023011829

---

- totalSupply() should be cached and called only once here:
https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/Baal.sol#L478

**resolution:**
https://github.com/HausDAO/Baal/pull/89/files#r1023018943

---

- You can start iterating from 1 here and omit the condition in the loop:
https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/Baal.sol#L619
alternately you can make the check part of this loop
https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/Baal.sol#L651

**resolution:**
https://github.com/HausDAO/Baal/pull/89/files#r1023020138

---

- Having nonce as explicit argument here is not strictly necessary:
https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/utils/BaalVotes.sol#L74

**resolution:**
this PR was merged into the active branch. also _hashTypedDataV4 adds chain id.
https://github.com/HausDAO/Baal/pull/90

---

- Since you're only including the avatar() call to satisfy the requirements of the function here, it might be better to call createProxy instead that doesn't require an initializer
https://github.com/HausDAO/Baal/blob/40daad35174cc42fb83724ee6069d3ca063338e9/contracts/BaalSummoner.sol#L231

**resolution:**
this is so we can use the canonical Zodiac module factory. there was a Module inheritance that is no longer needed here

https://github.com/HausDAO/Baal/pull/89/files#r1023024374

---

- I'm not even sure the nonce is used, because createProxy is called here: https://github.com/HausDAO/Baal/blob/2be872230eff660b21559d2640f0da1aa3d50bc6/contracts/BaalSummoner.sol#L194
which doesn't use create2
https://github.com/safe-global/safe-contracts/blob/c36bcab46578a442862d043e12a83fec41143dec/contracts/proxies/GnosisSafeProxyFactory.sol#L15

**resolution:**
https://github.com/HausDAO/Baal/pull/89/files#r1024211733

---

- Also I think it would nice to reuse the summonVault function in summonBaalAndVault

**resolution:**
https://github.com/HausDAO/Baal/pull/89/files#diff-af9619e2e79a3898ecbbfe0274e83c54c7dbbe8f2692f81e9fe2f5771ababe59R86

#### other notes about the main PR referenced:
- this includes a refactory of baalSummoner so it would work better with higher order factories, and is also upgradable now.

- Added OZ snapshot to sharesToken for an external application that requested support

- There is a new contract to work as a factory and register around DAO controlled Safes.

- this PR https://github.com/HausDAO/Baal/pull/91
aligns function names to be the same as eip5805 which may enable better future cross compatibility
https://github.com/ethereum/EIPs/blob/28be477d2df39b5fc86ec3587d630be98a6d0761/EIPS/eip-5805.md#solidity-interface