Skip to content

Commit

Permalink
Merge pull request #2 from MSalman6/fix/audit-issue-64
Browse files Browse the repository at this point in the history 
Audit Fixes
  • Loading branch information
@MSalman6
MSalman6 authored Jan 27, 2025
2 parents 8fc4d33 + ab1141d commit 8f957bc
Show file tree
Hide file tree
Showing 9 changed files with 172 additions and 30 deletions.
63 changes: 48 additions & 15 deletions contracts/DiamondDao.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ contract DiamondDao is IDiamondDao, Initializable, ReentrancyGuardUpgradeable, V
/// @notice To make sure we don't exceed the gas limit updating status of proposals
uint256 public daoPhaseCount;
uint256 public constant MAX_NEW_PROPOSALS = 1000;
uint256 public constant REQUIRED_EXCEEDING_50_PERCENT = 50 * 100;
uint256 public constant REQUIRED_EXCEEDING_33_PERCENT = 33 * 100;

///@dev this is the duration of each DAO phase.
///A full DAO cycle consists of 2 phases: Proposal and Voting,
Expand Down Expand Up @@ -131,6 +133,7 @@ contract DiamondDao is IDiamondDao, Initializable, ReentrancyGuardUpgradeable, V
}

function initialize(
address _contractOwner,
address _validatorSet,
address _stakingHbbft,
address _reinsertPot,
Expand All @@ -151,6 +154,7 @@ contract DiamondDao is IDiamondDao, Initializable, ReentrancyGuardUpgradeable, V
revert InvalidStartTimestamp();
}

__Ownable_init(_contractOwner);
__ReentrancyGuard_init();

validatorSet = IValidatorSetHbbft(_validatorSet);
Expand All @@ -165,15 +169,9 @@ contract DiamondDao is IDiamondDao, Initializable, ReentrancyGuardUpgradeable, V
daoPhaseCount = 1;

uint256[] memory createProposalFeeAllowedParams = new uint256[](9);
createProposalFeeAllowedParams[0] = 10 ether;
createProposalFeeAllowedParams[1] = 20 ether;
createProposalFeeAllowedParams[2] = 30 ether;
createProposalFeeAllowedParams[3] = 40 ether;
createProposalFeeAllowedParams[4] = 50 ether;
createProposalFeeAllowedParams[5] = 60 ether;
createProposalFeeAllowedParams[6] = 70 ether;
createProposalFeeAllowedParams[7] = 80 ether;
createProposalFeeAllowedParams[8] = 90 ether;
for (uint256 i = 0; i < 9; ++i) {
createProposalFeeAllowedParams[i] = (i + 1) * 10 ether;
}

__initAllowedChangeableParameter(
this.setCreateProposalFee.selector,
Expand All @@ -200,15 +198,16 @@ contract DiamondDao is IDiamondDao, Initializable, ReentrancyGuardUpgradeable, V
}

function switchPhase() external nonReentrant {
if (block.timestamp < daoPhase.end) {
uint64 currentTimestamp = uint64(block.timestamp);

if (currentTimestamp < daoPhase.end) {
return;
}

Phase newPhase = daoPhase.phase == Phase.Proposal ? Phase.Voting : Phase.Proposal;

uint64 newPhaseStart = daoPhase.end + 1;
daoPhase.start = newPhaseStart;
daoPhase.end = newPhaseStart + DAO_PHASE_DURATION;
daoPhase.start = currentTimestamp;
daoPhase.end = currentTimestamp + DAO_PHASE_DURATION;
daoPhase.phase = newPhase;

ProposalState stateToSet = newPhase == Phase.Voting
Expand Down Expand Up @@ -336,6 +335,36 @@ contract DiamondDao is IDiamondDao, Initializable, ReentrancyGuardUpgradeable, V
emit SubmitVoteWithReason(voter, proposalId, _vote, reason);
}

function changeVote(
uint256 proposalId,
Vote _vote,
string calldata reason
) external exists(proposalId) onlyPhase(Phase.Voting) onlyValidator {
if (!_proposalVoters[proposalId].contains(msg.sender)) {
revert NoVoteFound(proposalId, msg.sender);
}

address voter = msg.sender;

Proposal storage proposal = proposals[proposalId];

if (proposal.state != ProposalState.Active) {
revert UnexpectedProposalState(proposalId, proposal.state);
}

VoteRecord storage voteRecord = votes[proposalId][voter];

if (voteRecord.vote == _vote && keccak256(bytes(voteRecord.reason)) == keccak256(bytes(reason))) {
revert SameVote(proposalId, voter, _vote);
}

voteRecord.vote = _vote;
voteRecord.reason = reason;
voteRecord.timestamp = uint64(block.timestamp);

emit ChangeVote(voter, proposalId, _vote, reason);
}

function finalize(uint256 proposalId) external nonReentrant exists(proposalId) {
_requireState(proposalId, ProposalState.VotingFinished);

Expand Down Expand Up @@ -464,9 +493,9 @@ contract DiamondDao is IDiamondDao, Initializable, ReentrancyGuardUpgradeable, V
uint256 totalStakedAmount = daoEpochTotalStakeSnapshot[daoEpoch];

if (_type == ProposalType.ContractUpgrade) {
requiredExceeding = totalStakedAmount * (50 * 100) / 10000;
requiredExceeding = totalStakedAmount * REQUIRED_EXCEEDING_50_PERCENT / 10000;
} else {
requiredExceeding = totalStakedAmount * (33 * 100) / 10000;
requiredExceeding = totalStakedAmount * REQUIRED_EXCEEDING_33_PERCENT / 10000;
}

return totalVotes > 0 && result.stakeYes >= result.stakeNo + requiredExceeding;
Expand Down Expand Up @@ -510,6 +539,10 @@ contract DiamondDao is IDiamondDao, Initializable, ReentrancyGuardUpgradeable, V
) private {
_requireState(proposalId, ProposalState.Active);

if (_proposalVoters[proposalId].contains(voter)) {
revert AlreadyVoted(proposalId, voter);
}

_daoEpochVoters[daoPhase.daoEpoch].add(voter);
_proposalVoters[proposalId].add(voter);

Expand Down
21 changes: 12 additions & 9 deletions contracts/interfaces/IDiamondDao.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,26 @@ interface IDiamondDao {

event ProposalExecuted(address indexed caller, uint256 indexed proposalId);

event VotingFinalized(address indexed caller, uint256 indexed proposalId, bool accepted);
event VotingFinalized(address indexed caller, uint256 indexed proposalId, bool indexed accepted);

event SubmitVote(address indexed voter, uint256 indexed proposalId, Vote vote);
event SubmitVote(address indexed voter, uint256 indexed proposalId, Vote indexed vote);

event SubmitVoteWithReason(
address indexed voter,
uint256 indexed proposalId,
Vote vote,
Vote indexed vote,
string reason
);

event SwitchDaoPhase(Phase phase, uint256 start, uint256 end);
event ChangeVote(address indexed voter, uint256 indexed proposalId, Vote indexed vote, string reason);

event SetCreateProposalFee(uint256 fee);
event SwitchDaoPhase(Phase indexed phase, uint256 indexed start, uint256 indexed end);

event SetIsCoreContract(address contractAddress, bool isCore);
event SetCreateProposalFee(uint256 indexed fee);

event SetChangeAbleParameters(bool allowed, string setter, string getter, uint256[] params);
event SetIsCoreContract(address indexed contractAddress, bool indexed isCore);

event SetChangeAbleParameters(bool indexed allowed, string setter, string getter, uint256[] params);

error InsufficientFunds();
error InvalidArgument();
Expand All @@ -52,11 +54,12 @@ interface IDiamondDao {
error UnavailableInCurrentPhase(Phase phase);
error UnexpectedProposalState(uint256 proposalId, ProposalState state);
error ContractCallFailed(bytes funcSelector, address targetContract);
error FunctionUpgradeNotAllowed(bytes4 funcSelector, address targetContract);
error InvalidUpgradeValue(uint256 currentVal, uint256 newVal);
error UnfinalizedProposalsExist();
error OutsideExecutionWindow(uint256 proposalId);
error NotProposer(uint256 proposalId, address caller);
error SameVote(uint256 proposalId, address vote, Vote _vote);
error AlreadyVoted(uint256 proposalId, address voter);
error NoVoteFound(uint256 proposalId, address voter);

function propose(
address[] memory targets,
Expand Down
1 change: 0 additions & 1 deletion contracts/library/DaoStructs.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ enum ProposalState {
}

enum Vote {
Abstain,
No,
Yes
}
Expand Down
1 change: 1 addition & 0 deletions scripts/deploy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ async function deploy() {
console.log("Deploying DiamondDao contract");

const dao = await deployProxy("DiamondDao", [
deployer.address, // _contractOwner
"0x1000000000000000000000000000000000000001", // ValidatorSetHbbf
"0x1100000000000000000000000000000000000001", // StakingHbbft
"0x2000000000000000000000000000000000000001", // _reinsertPot
Expand Down
1 change: 1 addition & 0 deletions scripts/deployForChainSpec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ async function compileProxy() {
let startTimeBigInt = BigInt(Math.floor(startTimeStamp));

let daoInitArgs: any[] = [
daoProxyAddress, //address _contractOwner,
"0x1000000000000000000000000000000000000001", //address _validatorSet,
"0x1100000000000000000000000000000000000001", //address _stakingHbbft,
"0x2000000000000000000000000000000000000001", //address _reinsertPot,
Expand Down
109 changes: 107 additions & 2 deletions test/DiamondDao.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ enum DaoPhase {
}

enum Vote {
Abstain,
No,
Yes
}
Expand Down Expand Up @@ -60,6 +59,7 @@ describe("DiamondDao contract", function () {
const startTime = await time.latest();

const daoProxy = await upgrades.deployProxy(daoFactory, [
users[0].address,
await mockValidatorSet.getAddress(),
await mockStaking.getAddress(),
reinsertPot.address,
Expand Down Expand Up @@ -142,13 +142,25 @@ describe("DiamondDao contract", function () {
}
}

async function changeVote(
dao: DiamondDao,
proposalId: bigint,
voters: HardhatEthersSigner[],
vote: Vote
) {
for (const voter of voters) {
await dao.connect(voter).changeVote(proposalId, vote, "");
}
}

describe("initializer", async function () {
it("should not deploy contract with invalid ValidatorSet address", async function () {
const daoFactory = await ethers.getContractFactory("DiamondDao");
const startTime = await time.latest();

await expect(
upgrades.deployProxy(daoFactory, [
users[0].address,
ethers.ZeroAddress,
users[1].address,
users[2].address,
Expand All @@ -167,6 +179,7 @@ describe("DiamondDao contract", function () {

await expect(
upgrades.deployProxy(daoFactory, [
users[0].address,
users[1].address,
ethers.ZeroAddress,
users[2].address,
Expand All @@ -185,6 +198,7 @@ describe("DiamondDao contract", function () {

await expect(
upgrades.deployProxy(daoFactory, [
users[0].address,
users[1].address,
users[2].address,
ethers.ZeroAddress,
Expand All @@ -203,6 +217,7 @@ describe("DiamondDao contract", function () {

await expect(
upgrades.deployProxy(daoFactory, [
users[0].address,
users[1].address,
users[2].address,
users[3].address,
Expand All @@ -221,6 +236,7 @@ describe("DiamondDao contract", function () {

await expect(
upgrades.deployProxy(daoFactory, [
users[0].address,
users[1].address,
users[2].address,
users[3].address,
Expand All @@ -238,6 +254,7 @@ describe("DiamondDao contract", function () {
const startTime = await time.latest();

const dao = await upgrades.deployProxy(daoFactory, [
users[0].address,
users[1].address,
users[2].address,
users[3].address,
Expand All @@ -252,6 +269,7 @@ describe("DiamondDao contract", function () {

await expect(
dao.initialize(
users[0].address,
users[1].address,
users[2].address,
users[3].address,
Expand Down Expand Up @@ -896,6 +914,93 @@ describe("DiamondDao contract", function () {
});
});

describe("changeVote", async function () {
it("should revert in case of double voting", async function () {
const { dao, mockValidatorSet } = await loadFixture(deployFixture);

const proposer = users[10];
const voter = users[9];

const { proposalId } = await createProposal(dao, proposer, "a");

await mockValidatorSet.add(voter.address, voter.address, true);
await swithPhase(dao);

await dao.connect(voter).vote(proposalId, Vote.Yes);

await expect(
dao.connect(voter).vote(proposalId, Vote.No)
).to.be.revertedWithCustomError(dao, "AlreadyVoted")
.withArgs(proposalId, voter.address);
});

it("should revert change vote if voter has not voted yet", async function () {
const { dao, mockValidatorSet } = await loadFixture(deployFixture);

const proposer = users[10];
const voter = users[9];

const { proposalId } = await createProposal(dao, proposer, "a");

await mockValidatorSet.add(voter.address, voter.address, true);
await swithPhase(dao);

await expect(
dao.connect(voter).changeVote(proposalId, Vote.Yes, "reason")
).to.be.revertedWithCustomError(dao, "NoVoteFound")
.withArgs(proposalId, voter.address);
});

it("should revert if same vote is submitted", async function () {
const { dao, mockValidatorSet } = await loadFixture(deployFixture);

const proposer = users[10];
const voter = users[9];

const { proposalId } = await createProposal(dao, proposer, "a");

await mockValidatorSet.add(voter.address, voter.address, true);
await swithPhase(dao);

await dao.connect(voter).voteWithReason(proposalId, Vote.Yes, "reason");

await expect(
dao.connect(voter).changeVote(proposalId, Vote.Yes, "reason")
).to.be.revertedWithCustomError(dao, "SameVote")
.withArgs(proposalId, voter.address, Vote.Yes);
});

it("should allow user to change vote", async function () {
const { dao, mockValidatorSet } = await loadFixture(deployFixture);

const proposer = users[10];
const voter = users[9];

const { proposalId } = await createProposal(dao, proposer, "a");

await mockValidatorSet.add(voter.address, voter.address, true);
await swithPhase(dao);

await dao.connect(voter).vote(proposalId, Vote.Yes);
await expect(dao.connect(voter).changeVote(proposalId, Vote.No, "reason")).to.not.be.reverted;
});

it("should allow user to change vote reason", async function () {
const { dao, mockValidatorSet } = await loadFixture(deployFixture);

const proposer = users[10];
const voter = users[9];

const { proposalId } = await createProposal(dao, proposer, "a");

await mockValidatorSet.add(voter.address, voter.address, true);
await swithPhase(dao);

await dao.connect(voter).voteWithReason(proposalId, Vote.Yes, "reason");
await expect(dao.connect(voter).changeVote(proposalId, Vote.No, "new reason")).to.not.be.reverted;
});
});

describe("countVotes", async function () {
it("should revert count votes for non-existing proposal", async function () {
const { dao } = await loadFixture(deployFixture);
Expand Down Expand Up @@ -955,7 +1060,7 @@ describe("DiamondDao contract", function () {
0n
]);

await vote(dao, proposalId, voters, Vote.No);
await changeVote(dao, proposalId, voters, Vote.No);

expect(Object.values(await dao.countVotes(proposalId))).to.deep.equal([
0n,
Expand Down
Loading

0 comments on commit 8f957bc

Please sign in to comment.