Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit fixes brian #76

Merged
merged 11 commits into from
Oct 4, 2022
Prev Previous commit
Next Next commit
QSP-7 Signed Votes Do Not Expire
brossetti1 committed Oct 4, 2022
commit 9b5c4b0198df61473fab1a9f92551bbb05402ef0
11 changes: 7 additions & 4 deletions contracts/Baal.sol
Original file line number Diff line number Diff line change
@@ -14,7 +14,6 @@ import "@gnosis.pm/safe-contracts/contracts/GnosisSafe.sol";
import "@gnosis.pm/zodiac/contracts/core/Module.sol";
import "@gnosis.pm/safe-contracts/contracts/common/Enum.sol";
import "@opengsn/contracts/src/BaseRelayRecipient.sol";
import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import "@openzeppelin/contracts-upgradeable/utils/cryptography/draft-EIP712Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";

@@ -23,7 +22,7 @@ import "./interfaces/IBaalToken.sol";
/// @title Baal ';_;'.
/// @notice Flexible guild contract inspired by Moloch DAO framework.
contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRelayRecipient {
using ECDSA for bytes32;
using ECDSAUpgradeable for bytes32;
Copy link
Author

Choose a reason for hiding this comment

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

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

Screen Shot 2022-10-03 at 10 09 24 AM


// ERC20 SHARES + LOOT

@@ -66,7 +65,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela
string public override versionRecipient; /* version recipient for OpenGSN */

// SIGNATURE HELPERS
bytes32 constant VOTE_TYPEHASH = keccak256("Vote(string name,address voter,uint32 proposalId,bool support)");
bytes32 constant VOTE_TYPEHASH = keccak256("Vote(string name,address voter,uint256 expiry,uint32 proposalId,bool support)");

// DATA STRUCTURES
struct Proposal {
@@ -408,31 +407,35 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela

/// @notice Submit vote with EIP-712 signature - proposal must exist & voting period must not have ended.
/// @param voter Address of member who submitted vote.
/// @param expiry Expiration of signature.
/// @param id Number of proposal in `proposals` mapping to cast vote on.
/// @param approved If 'true', member will cast `yesVotes` onto proposal - if 'false', `noVotes` will be counted.
/// @param v v in signature
/// @param r r in signature
/// @param s s in signature
function submitVoteWithSig(
address voter,
uint256 expiry,
uint32 id,
bool approved,
uint8 v,
bytes32 r,
bytes32 s
) external nonReentrant {
require(block.timestamp <= expiry, "ERC20Votes: signature expired");
/*calculate EIP-712 struct hash*/
bytes32 structHash = keccak256(
abi.encode(
VOTE_TYPEHASH,
keccak256(abi.encodePacked(sharesToken.name())),
voter,
expiry,
id,
approved
)
);
bytes32 hash = _hashTypedDataV4(structHash);
address signer = ECDSA.recover(hash, v, r, s);
address signer = ECDSAUpgradeable.recover(hash, v, r, s);

require(signer == voter, "invalid signature");
require(signer != address(0), "!signer");
3 changes: 3 additions & 0 deletions src/signVote.ts
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@ export default async function signVote(
contractAddress: string,
signer: SignerWithAddress,
name: string,
expiry: number,
proposalId: number,
support: boolean

@@ -20,6 +21,7 @@ export default async function signVote(
Vote: [
{ name: 'name', type: 'string' },
{ name: 'voter', type: 'address' },
{ name: 'expiry', type: 'uint256' },
{ name: 'proposalId', type: 'uint32' },
{ name: 'support', type: 'bool' },
],
@@ -28,6 +30,7 @@ export default async function signVote(
const values = {
name,
voter: signer.address,
expiry,
proposalId,
support
}
20 changes: 11 additions & 9 deletions test/BaalSafe.test.ts
Original file line number Diff line number Diff line change
@@ -25,15 +25,10 @@ import {
hashOperation,
} from "../src/util";
import { BigNumber, BigNumberish } from "@ethersproject/bignumber";
import { buildContractCall } from "@gnosis.pm/safe-contracts";
import { ContractFactory, ContractTransaction, utils } from "ethers";
import { Test } from "mocha";
import signVote from "../src/signVote";
import signDelegation from "../src/signDelegation";
import signPermit from "../src/signPermit";
import { string } from "hardhat/internal/core/params/argumentTypes";
import { calculateProxyAddress } from "@gnosis.pm/zodiac";
import { Address } from "cluster";

use(solidity);

@@ -2508,17 +2503,19 @@ describe("Baal contract", function () {
});

it("happy case - yes vote", async function () {
const expiry = await blockTime() + 1200;
const signature = await signVote(
chainId,
baal.address,
summoner,
deploymentConfig.TOKEN_NAME,
expiry,
1,
true
);

const {v,r,s} = await ethers.utils.splitSignature(signature);
await baal.submitVoteWithSig(summoner.address, 1, true, v, r, s);
await baal.submitVoteWithSig(summoner.address, expiry, 1, true, v, r, s);
const prop = await baal.proposals(1);
const nCheckpoints = await sharesToken.numCheckpoints(summoner.address);
const votes = (
@@ -2532,37 +2529,42 @@ describe("Baal contract", function () {
expect(prop.yesVotes).to.equal(votes);
});


it("fail case - fails with different voter", async function () {
const expiry = await blockTime() + 1200;
const signature = await signVote(
chainId,
baal.address,
summoner,
deploymentConfig.TOKEN_NAME,
expiry,
1,
true
);

const {v,r,s} = await ethers.utils.splitSignature(signature);
expect(
baal.submitVoteWithSig(applicant.address, 1, true, v, r, s)
baal.submitVoteWithSig(applicant.address, expiry, 1, true, v, r, s)
).to.be.revertedWith("invalid signature");
});

it("fail case - cant vote twice", async function () {
const expiry = await blockTime() + 1200;
const signature = await signVote(
chainId,
baal.address,
summoner,
deploymentConfig.TOKEN_NAME,
expiry,
1,
true
);

const {v,r,s} = await ethers.utils.splitSignature(signature);
await baal.submitVoteWithSig(summoner.address, 1, true, v, r, s);
await baal.submitVoteWithSig(summoner.address, expiry, 1, true, v, r, s);

expect(
baal.submitVoteWithSig(summoner.address, 1, true, v, r, s)
baal.submitVoteWithSig(summoner.address, expiry, 1, true, v, r, s)
).to.be.revertedWith("voted");
});
});