Audit fixes brian #76

brossetti1 · 2022-09-27T00:43:24Z

wip fixing quantstamp issues

brossetti1 · 2022-09-28T22:25:10Z

.vscode/settings.json

@@ -0,0 +1,6 @@
+{
+  "solidity.defaultCompiler": "localNodeModule",


oops should proably update this?

dekanbro · 2022-09-29T16:36:02Z

updated docs for some of these issues
QSP-5 Ownership Can Be Renounced
QSP-6 Shamans Can Be an EOA Address
QSP-10 Risk of Killing Upgrades
QSP-12 A DAO's Safety Is Dependent on the Safety of Its Shamans
QSP-13 Upgradability
QSP-14 msg.sender Can Be Overridden.
QSP-15 External Calls to Malicious Contracts

HausDAO/baal-docs@5151a94

brossetti1 · 2022-10-03T16:09:57Z

contracts/Baal.sol

-contract Baal is Module, EIP712, ReentrancyGuard, BaseRelayRecipient {
-    using ECDSA for bytes32;
+contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRelayRecipient {
+    using ECDSAUpgradeable for bytes32;


so this is actually an upgrade on the ECDSA logic - there was some signature validations that openzeppelin removed from the ECDSA checks

OpenZeppelin/openzeppelin-contracts@d693d89

brossetti1 · 2022-10-03T16:15:25Z

contracts/Baal.sol

@@ -268,7 +277,7 @@ contract Baal is Module, EIP712, ReentrancyGuard, BaseRelayRecipient {
                _initializationMultisendData,
                Enum.Operation.DelegateCall
            ),
-            "call failure"
+            "call failure setup"


adding one more word to better distinguish errors -- there were two call failure errors in this file from different functions.

brossetti1 · 2022-10-03T16:16:31Z

contracts/Baal.sol

+        proposalCount++; /*increment proposal counter*/
+        proposals[proposalCount] = Proposal( /*push params into proposal struct - start voting period timer if member submission*/
+            proposalCount,
+            selfSponsor ? latestSponsoredProposalId : 0, /* prevProposalId */
+            selfSponsor ? uint32(block.timestamp) : 0, /* votingStarts */
+            selfSponsor ? uint32(block.timestamp) + votingPeriod : 0, /* votingEnds */
+            selfSponsor
+                ? uint32(block.timestamp) + votingPeriod + gracePeriod
+                : 0, /* graceEnds */
+            expiration,
+            baalGas,
+            0, /* yes votes */
+            0, /* no votes */
+            0, /* highestMaxSharesAndLootAtYesVote */
+            [false, false, false, false], /* [cancelled, processed, passed, actionFailed] */
+            selfSponsor ? _msgSender() : address(0),
+            proposalDataHash,
+            details
+        );


qsp for removing the unchecked call as the QuantStamp team pointed out that there are some holes with unchecked overflow here so its safer to have it checked

brossetti1 · 2022-10-03T16:16:56Z

contracts/Baal.sol

@@ -573,7 +585,7 @@ contract Baal is Module, EIP712, ReentrancyGuard, BaseRelayRecipient {
        bytes calldata _data
    ) external baalOnly {
        (bool success, ) = _to.call{value: _value}(_data);
-        require(success, "call failure");
+        require(success, "call failure execute");


other error update

brossetti1 · 2022-10-03T16:17:19Z

contracts/Baal.sol

        } else if(!pauseLoot && lootToken.paused()){
            lootToken.unpause();
+            emit LootPaused(false);


only emitting events if status changed on paused

brossetti1 · 2022-10-03T18:12:52Z

test/BaalSafe.test.ts

+      await baal.submitVoteWithSig(summoner.address, expiry, 0, 1, true, v, r, s);

+      const signatureTwo = await signVote(
+        chainId,
+        baal.address,
+        summoner,
+        deploymentConfig.TOKEN_NAME,
+        expiry,
+        1,
+        1,
+        true
+      );
+      const sigTwo = await ethers.utils.splitSignature(signatureTwo);


interesting how this test included a check on nonces inherently as it was using the same signature for both submitVoteWithSig - after adding a second signature with the nonce bumped, the test resolved from the error that triggered from adding the nonce. ie notice how nonce had to go from 0 to 1

brossetti1 · 2022-10-04T17:05:30Z

README.md

+## Coverage
+
+currently, coverage is turned off for test efficiency purposes. In order to switch coverage on, add `yul` to the hardhat config:
+
+```
+{
+  ...
+  compilers: [
+    {
+      version: "0.8.7",
+      settings: {
+        optimizer: {
+          enabled: true,
+          runs: 200,
+          details: {
+            yul: true
+          }
+        },
+      },
+    }
+  ]
+}
+```
+
+then run the coverage command:
+
+```
+npx hardhat coverage
+```


coverage docs - should be turned on manually and turned off for faster test runs

…eable dependencies on oz

…t logic

…nged.

Co-authored-by: dekanbro <[email protected]>

brossetti1 requested a review from dekanbro September 27, 2022 00:43

brossetti1 force-pushed the audit-fixes-brian branch 2 times, most recently from a0939bb to 740ca4a Compare September 28, 2022 22:23

brossetti1 commented Sep 28, 2022

View reviewed changes

.vscode/settings.json

@@ -0,0 +1,6 @@

{

"solidity.defaultCompiler": "localNodeModule",

Copy link

Author

brossetti1 Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops should proably update this?

brossetti1 force-pushed the audit-fixes-brian branch from 1914e14 to e090e14 Compare October 3, 2022 16:58

brossetti1 commented Oct 3, 2022

View reviewed changes

brossetti1 marked this pull request as ready for review October 3, 2022 17:12

brossetti1 commented Oct 3, 2022

View reviewed changes

brossetti1 force-pushed the audit-fixes-brian branch from 1dcfbff to c12b07e Compare October 3, 2022 20:48

brossetti1 commented Oct 4, 2022

View reviewed changes

dekanbro approved these changes Oct 4, 2022

View reviewed changes

brossetti1 and others added 11 commits October 4, 2022 12:50

QSP-2 Baal Inherits From Non-Upgradeable Contracts: fixing non upgrad…

f9dca50

…eable dependencies on oz

QSP-3 Integer Overflow / Underflow - remove unchecked on proposalCoun…

6bd908d

…t logic

QSP-4 Missing Input Validation - reuqiring checks on missed validation

e2319d2

QSP-8 Application Monitoring Can Be Improved by Emitting More Events

62396f8

QSP-7 Signed Votes Do Not Expire

9b5c4b0

QSP-9 setAdminConfig Always Emits Two Events Even if State Is Not Cha…

3c1792a

…nged.

QSP-1 Checkpoints May Not Be Written Correctly (#77)

68e53af

Co-authored-by: dekanbro <[email protected]>

QSP-16 Proposals Can Pass without a Valid Sponsor

c20ee60

updating version recipient

67dfe26

adding nonce to submitVote

143f9dc

adding docs for running coverage

cd28a98

brossetti1 force-pushed the audit-fixes-brian branch from c12b07e to cd28a98 Compare October 4, 2022 18:56

brossetti1 merged commit 26470d2 into feat/baalZodiac Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit fixes brian #76

Audit fixes brian #76

brossetti1 commented Sep 27, 2022

brossetti1 Sep 28, 2022

dekanbro commented Sep 29, 2022 •

edited

Loading

brossetti1 Oct 3, 2022

brossetti1 Oct 3, 2022

brossetti1 Oct 3, 2022

brossetti1 Oct 3, 2022

brossetti1 Oct 3, 2022

brossetti1 Oct 3, 2022

brossetti1 Oct 4, 2022

		@@ -0,0 +1,6 @@
		{
		"solidity.defaultCompiler": "localNodeModule",

Audit fixes brian #76

Audit fixes brian #76

Conversation

brossetti1 commented Sep 27, 2022

Choose a reason for hiding this comment

dekanbro commented Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekanbro commented Sep 29, 2022 •

edited

Loading