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

Moloch rage quit #32

Open
wants to merge 28 commits into
base: moloch-rage-quit
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
c32ae3e
add Moloch Rage Quit challenge description
KcPele May 30, 2024
f335446
add Moloch Rage Quit challenge description
KcPele May 30, 2024
adcf396
simple challange
KcPele May 30, 2024
2fb44ed
simple challange solution
KcPele May 30, 2024
a512149
complex challenge
KcPele May 30, 2024
787309d
added some test
KcPele May 30, 2024
0af893c
added some test
KcPele May 31, 2024
7f6d4ca
completed test
KcPele Jun 1, 2024
8a9c30c
feat: complete smart contract test
KcPele Jun 1, 2024
b248d9a
feat: complete smart contract test
KcPele Jun 1, 2024
accb922
feat: complete smart contract test
KcPele Jun 1, 2024
5825570
fix:resolved errors and made changes
KcPele Jun 4, 2024
f6c4832
updated contract
KcPele Jun 19, 2024
0b42398
updated contract
KcPele Jun 19, 2024
3bfee27
updated contract
KcPele Jun 19, 2024
5467799
test for proposal creation
KcPele Jul 9, 2024
d76f4f7
fix:executeProposal
KcPele Jul 9, 2024
04117ae
fix:addMember and removeMember
KcPele Jul 9, 2024
1714fda
fix:addMember and removeMember
KcPele Jul 9, 2024
aaf7327
fix:addMember and removeMember
KcPele Jul 9, 2024
1d39abe
fix:proposal creation
KcPele Jul 9, 2024
19c2d05
fix:proposal creation
KcPele Jul 9, 2024
51dc03f
fix:contract execution
KcPele Jul 9, 2024
eea0065
fix:contract execution
KcPele Jul 9, 2024
a87fe85
fix:added test and fixed addMember function
KcPele Jul 18, 2024
59f2654
fix:added test and fixed addMember function
KcPele Jul 18, 2024
610bdff
fix:rageQuite
KcPele Jul 18, 2024
cbcd8b0
fix:rageQuite
KcPele Jul 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Template For Challenge - ETH Tech Tree
# Moloch Rage Quit - ETH Tech Tree

## Contents

- [Requirements](#requirements)
- [Start Here](#start-here)
- [Challenge Description](#challenge-description)
Expand All @@ -14,36 +16,46 @@ Before you begin, you need to install the following tools:
- [Foundryup](https://book.getfoundry.sh/getting-started/installation)

## Start Here

Run the following commands in your terminal:

```
yarn install
foundryup
```

## Challenge Description
*Edit this section*
Write challenge description here...

### Objective:

In this challenge, you will create a smart contract that simulates a simple DAO (Decentralized Autonomous Organization) allowing members to propose and exchange ETH for shares. Additionally, members can "rage quit" to exchange their shares back for ETH if they are dissatisfied with the DAO's direction.

### Scenario:

Imagine a DAO where members contribute ETH to acquire shares. These shares represent ownership and voting power within the DAO. Members can propose to acquire a specific amount of shares for a certain amount of ETH. If the proposal is approved by the existing members, the proposer can exchange their ETH for the proposed shares. As the DAO evolves, it can accumulate ETH through various activities or spend ETH on services. If a member becomes unhappy with the DAO's direction, they can call the RageQuit function to exchange their shares for their proportionate share of the DAO's ETH holdings.

Here are some helpful references:
- [one](buidlguidl.com)
- [two](buidlguidl.com)
- [three](buidlguidl.com)

*End of challenge specific section*
- [Moloch DAO Primer](https://medium.com/odyssy/moloch-primer-for-humans-9e6a4f258f78)

**Don't change any existing method names** as it will break tests but feel free to add additional methods if it helps you complete the task.

When you think you are done run `yarn foundry:test` to run a set of tests against your code. If all your tests pass then you are good to go! If some are returning errors then you might find it useful to run the command with the extra logging verbosity flag `-vvvv` (`yarn foundry:test -vvvv`) as this will show you very detailed information about where tests are failing.

For a more "hands on" approach you can try testing your contract with the provided front end interface by running the following:

```
yarn chain
```

in a second terminal deploy your contract:

```
yarn deploy
```

in a third terminal start the NextJS front end:

```
yarn start
```
```
6 changes: 0 additions & 6 deletions packages/foundry/contracts/Challenge.sol

This file was deleted.

25 changes: 25 additions & 0 deletions packages/foundry/contracts/MaliciousContract.sol
KcPele marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "../contracts/MolochRageQuit.sol";

contract MaliciousContract {
MolochRageQuit public dao;

constructor(address _daoAddress) {
dao = MolochRageQuit(_daoAddress);
}

receive() external payable {
if (address(dao).balance >= 1 ether) {
dao.rageQuit();
}
}

function attack() external payable {
// Propose and approve shares for ETH

// Perform reentrancy attack
dao.rageQuit();
}
}
257 changes: 257 additions & 0 deletions packages/foundry/contracts/MolochRageQuit.sol
KcPele marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,257 @@
// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0 <0.9.0;

////////////////////
Copy link
Collaborator

Choose a reason for hiding this comment

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

These errors should be defined inside the contract. I realize the layout guidelines are confusing as it lists contracts below errors but what this means is contracts variables defined inside the contract.

// Imports
////////////////////
import "@openzeppelin/contracts/access/Ownable.sol";

////////////////////
// Errors
////////////////////
error InsufficientETH();
error ProposalNotApproved();
error UnauthorizedAccess();
error InsufficientShares();
error ZeroAddress();
error InvalidSharesAmount();
error AlreadyApproved();
error FailedTransfer();
error ProposalNotFound();
error NotEnoughVotes();
error AlreadyVoted();
error MemberExists();

////////////////////
KcPele marked this conversation as resolved.
Show resolved Hide resolved
// Contract
////////////////////
contract MolochRageQuit is Ownable {
///////////////////
// Type Declarations
///////////////////
struct Proposal {
address proposer;
uint256 ethAmount;
uint256 shareAmount;
uint256 votes;
bool approved;
mapping(address => bool) voted;
}

///////////////////
// State Variables
///////////////////
uint256 public totalShares;
uint256 public totalEth;
Copy link
Collaborator

Choose a reason for hiding this comment

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

totalEth doesn't need to be defined or tracked in any way. You can get the contract balance with address(this).balance

uint256 public proposalCount;
uint256 public quorum;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The way quorum is used is somewhat flawed. It should probably be replaced with a memberCount and then a formula is used to make sure a majority has voted. It should never allow one person to be the sole decider of a vote unless there is only one member.

In your current setup the only place they can set the quorum is in the constructor which it could never be set to more than 1 or else there would be no way to add a second member. If the quorum remains one indefinitely then one vote decides the outcomes for any proposal to no matter how many members there are. Yikes.

Perhaps there should be a minimum quorum of voters (maybe as low as 5%) that is required for a proposal to be approved but there is a more robust yea vs nay formula used after the deadline has passed. Because maybe only 10% of members voted and maybe that minimum quorum was reached but there were more nay votes than yea votes. I don't want to over complicate the contract so I am open to your ideas but the current mechanism is broken.

mapping(address => uint256) public shares;
mapping(uint256 => Proposal) public proposals;
mapping(address => bool) public members;

///////////////////
// Events
///////////////////
event ProposalCreated(
uint256 proposalId,
address proposer,
uint256 ethAmount,
uint256 shareAmount
);
event ProposalApproved(uint256 proposalId, address approver);
event SharesExchanged(
address proposer,
uint256 ethAmount,
uint256 shareAmount
);
event RageQuit(address member, uint256 shareAmount, uint256 ethAmount);
event MemberAdded(address member);
event MemberRemoved(address member);
event Voted(uint256 proposalId, address voter);
event Withdrawal(address owner, uint256 amount);

///////////////////
KcPele marked this conversation as resolved.
Show resolved Hide resolved
// Modifiers
///////////////////
modifier onlyMember() {
if (!members[msg.sender]) {
revert UnauthorizedAccess();
}
_;
}

///////////////////
KcPele marked this conversation as resolved.
Show resolved Hide resolved
// Constructor
///////////////////
constructor(uint256 _quorum) {
members[msg.sender] = true;
quorum = _quorum;
}

///////////////////
// External Functions
///////////////////

/**
* @dev Propose to acquire shares for ETH.
* @param ethAmount The amount of ETH to exchange for shares.
* @param shareAmount The amount of shares to acquire.
* Requirements:
* - `ethAmount` must be greater than 0.
* - `shareAmount` must be greater than 0.
* - should revert with `InvalidSharesAmount` if either `ethAmount` or `shareAmount` is 0.
* - Increment the proposal count.
* - Create a new proposal
* Emits a `ProposalCreated` event.
*/
function propose(uint256 ethAmount, uint256 shareAmount) external {
if (ethAmount == 0 || shareAmount == 0) {
revert InvalidSharesAmount();
}

proposalCount++;
Proposal storage proposal = proposals[proposalCount];
proposal.proposer = msg.sender;
proposal.ethAmount = ethAmount;
proposal.shareAmount = shareAmount;

emit ProposalCreated(proposalCount, msg.sender, ethAmount, shareAmount);
}

/**
* @dev Vote on a proposal.
* @param proposalId The ID of the proposal to vote on.
* Requirements:
* - Revert with `ProposalNotFound` if the proposal does not exist.
* - Revert with `AlreadyVoted` if the caller has already voted on the proposal.
* - Caller must be a member.
* - Proposal must exist.
* - Caller must not have already voted on the proposal.
* - Increment the proposal's vote count.
* - Mark the caller as having voted on the proposal.
* - If the proposal has enough votes, mark it as approved.
* Emits a `Voted` event.
* Emits a `ProposalApproved` event if the proposal is approved.
*/
function vote(uint256 proposalId) external onlyMember {
Proposal storage proposal = proposals[proposalId];
if (proposal.proposer == address(0)) {
revert ProposalNotFound();
}
if (proposal.voted[msg.sender]) {
revert AlreadyVoted();
}

proposal.votes++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In regards to my suggestions for updating the quorum variable: Maybe the proposal should track yea votes and nay votes instead of just votes in general.

proposal.voted[msg.sender] = true;

emit Voted(proposalId, msg.sender);

if (proposal.votes >= quorum) {
proposal.approved = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove approved on the proposal and instead track yea votes and nay votes. Approval depends on the proposal deadline being reached, total votes > quorum, and proposal.yeaVotes > proposal.nayVotes

emit ProposalApproved(proposalId, msg.sender);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it makes sense for a proposal to be approved before the voting period is over. More nay votes might counteract the current approval. The approval/denial should be decided in the executeProposal function. That is the only place where it being approved/denied matters.

}
}

/**
* @dev Exchange ETH for shares after approval.
* @param proposalId The ID of the approved proposal.
* Requirements:
* - The caller must be the proposer of the proposal.
* - The proposal must be approved.
KcPele marked this conversation as resolved.
Show resolved Hide resolved
* - The amount of ETH sent must match the proposal's ETH amount.
* Emits a `SharesExchanged` event.
*/
function exchangeShares(uint256 proposalId) external payable onlyMember {
KcPele marked this conversation as resolved.
Show resolved Hide resolved
Proposal storage proposal = proposals[proposalId];
if (proposal.proposer != msg.sender || !proposal.approved) {
revert ProposalNotApproved();
}
if (msg.value < proposal.ethAmount) {
revert InsufficientETH();
}

totalEth += msg.value;
totalShares += proposal.shareAmount;
shares[msg.sender] += proposal.shareAmount;

emit SharesExchanged(msg.sender, msg.value, proposal.shareAmount);
}

/**
* @dev Rage quit and exchange shares for ETH.
* Requirements:
* - The caller must have shares and must be a member.
KcPele marked this conversation as resolved.
Show resolved Hide resolved
* - Calculate the amount of ETH to return to the caller.
* - Update the total shares and total ETH.
* - Mark the caller as having 0 shares.
* - Transfer the ETH after calculating the share of eth to send to the caller.
* - Revert with `FailedTransfer` if the transfer fails.
* Emits a `RageQuit` event.
*/
function rageQuit() external onlyMember {
KcPele marked this conversation as resolved.
Show resolved Hide resolved
uint256 memberShares = shares[msg.sender];
if (memberShares == 0) {
revert InsufficientShares();
}
uint256 ethAmount = (memberShares * totalEth) / totalShares;
totalShares -= memberShares;
totalEth -= ethAmount;
shares[msg.sender] = 0;
(bool sent, ) = msg.sender.call{value: ethAmount}("");
if (!sent) {
revert FailedTransfer();
}
emit RageQuit(msg.sender, memberShares, ethAmount);
}
KcPele marked this conversation as resolved.
Show resolved Hide resolved

/**
* @dev Add a new member to the DAO.
* @param newMember The address of the new member.
* Requirements:
* - Only callable by the owner.
* - The address must not already be a member.
* - Mark the address as a member.
* Emits a `MemberAdded` event.
*/
function addMember(address newMember) external onlyOwner {
KcPele marked this conversation as resolved.
Show resolved Hide resolved
if (members[newMember]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check is the only thing keeping someone from executing the proposal multiple times. But what if I get in at a low share price and Rage Quit at a high share price? Then I can become a member again at my low share price by reexecuting my old proposal! I could do this over and over until the contract was drained.

revert MemberExists();
}
members[newMember] = true;
emit MemberAdded(newMember);
}

/**
* @dev Remove a member from the DAO.
* @param member The address of the member to remove.
* Requirements:
* - Only callable by the owner.
* - Mark the member as not a member.
* Emits an `MemberRemoved` event.
*/
function removeMember(address member) external onlyOwner {
members[member] = false;
emit MemberRemoved(member);
}

/**
* @dev Withdraw ETH from the DAO.
* @param amount The amount of ETH to withdraw.
* Requirements:
* - Only callable by the owner.
* - Revert with `InsufficientETH` if the amount exceeds the contract's balance.
* - Revert with `FailedTransfer` if the transfer fails.
*/
function withdraw(uint256 amount) public onlyOwner {
KcPele marked this conversation as resolved.
Show resolved Hide resolved
if (amount > address(this).balance) {
revert InsufficientETH();
}
(bool sent, ) = payable(msg.sender).call{value: amount}("");
if (!sent) {
revert FailedTransfer();
}

emit Withdrawal(msg.sender, amount);
}
}
1 change: 1 addition & 0 deletions packages/foundry/foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ src = 'contracts'
out = 'out'
libs = ['lib']
fs_permissions = [{ access = "read-write", path = "./"}]
solc_version = "0.8.19"

[rpc_endpoints]
default_network = "http://127.0.0.1:8545"
Expand Down
11 changes: 6 additions & 5 deletions packages/foundry/script/Deploy.s.sol
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
//SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "../contracts/Challenge.sol";
import "../contracts/MolochRageQuit.sol";
import "./DeployHelpers.s.sol";

contract DeployScript is ScaffoldETHDeploy {
error InvalidPrivateKey(string);
uint256 public constant QUORUM = 2;

function run() external {
uint256 deployerPrivateKey = setupLocalhostEnv();
Expand All @@ -15,11 +16,11 @@ contract DeployScript is ScaffoldETHDeploy {
);
}
vm.startBroadcast(deployerPrivateKey);
Challenge challenge = new Challenge();
MolochRageQuit molochRageQuit = new MolochRageQuit(QUORUM);
console.logString(
string.concat(
"Challenge deployed at: ",
vm.toString(address(challenge))
"MolochRageQuit deployed at: ",
vm.toString(address(molochRageQuit))
)
);
vm.stopBroadcast();
Expand All @@ -31,4 +32,4 @@ contract DeployScript is ScaffoldETHDeploy {
exportDeployments();
}
function test() public {}
}
}
Loading