Project - Multisig Wallet

I’ve added the ability to get the contract balance and to check that a transfer cannot be made that exceeds the balance:

pragma solidity 0.7.5;
pragma abicoder v2;

contract Wallet {

    address[3] public signers;
    uint limit;  
    uint balance;
    
    struct Transfer{
        uint amount;
        address payable recipient;
        uint confirmations; 
        uint id;
        bool Sent;
    }
    
   Transfer[] transferRequests;

   constructor(address _signers1, address _signers2, address _signers3, uint _limit){
       require(_signers1 != _signers2);
       require(_signers1 != _signers3);
       require(_signers2 != _signers3);
       signers = [_signers1, _signers2, _signers3];
        limit = _limit; 
              }

    mapping(address => mapping (uint => bool)) confirmations;

    modifier onlyOwners(){
   require(msg.sender == signers[0]||msg.sender == signers[1]||msg.sender == signers[2]);
              _;
          }
   
     function deposit() public payable returns (uint){
         balance+= msg.value;
         return balance;
        
      }


      function createTransfer(uint _amount, address payable _recipient) public onlyOwners{
            require (msg.sender != _recipient, "You cannot send money to yourself");
        
        transferRequests.push(
            Transfer(_amount, _recipient, 0, transferRequests.length, false)
            );
             }

function confirm(uint _id, uint _amount) public onlyOwners{
    require(confirmations[msg.sender][_id]==false);
    require(transferRequests[_id].Sent == false);
     confirmations[msg.sender][_id] = true;
    transferRequests[_id].confirmations++;
   
    if(transferRequests[_id].confirmations >= limit){
        transferRequests[_id].Sent = true;
        require (balance >= _amount, "Balance not sufficient");
        transferRequests[_id].recipient.transfer(transferRequests[_id].amount);
          
        balance = balance - _amount;
    }
    
}
    
 
function getTransferRequests() public view returns(Transfer[] memory){
     return transferRequests;
}

  function getBalance() public view returns (uint){
  
   return balance;

}
}

1 Like

Hey @m_Robbie, hope you are ok.

What are you trying to achieve exactly? the amount of limit signers?

Carlos Z

Hey @vasja, hope you are ok.

I have tested your contract, it works completely fine,

For Arrays: There are no limits in specification, so arrays may grow up to 2^256-1 elements.

I might advice you to take the Ethereum Smart Contract Security to learn the best security considerations for your smart contracts :nerd_face:

Contract storage is a key of 32 bytes and a value of 32 bytes, so the maximum a single contract can store is around 2^261 bytes (2^256 * 32b).

Carlos Z

This does the job (except for the limit of votes that one should be able to put in, but that should be straighforward too):

pragma solidity 0.7.5;
pragma abicoder v2; // to be able to return structs


contract Fund {
    uint balance;
    
    struct owner {
        address _address;
        uint _vote; // if vote == 1, confirm send, if vote==0 do not confirm send
    }
    owner[] owners; 
    
    event depositDone(uint amount);   
    
    modifier onlyOwners {
        require((msg.sender == owners[0]._address) || (msg.sender == owners[1]._address) || (msg.sender == owners[2]._address));
        _;
    }
    
    modifier enough_votes {
        require(_get_total_votes()>1);
        _;
    }
    
    constructor(address _address_2, address _address_3, uint _initial_vote){
        require((_initial_vote==0) || (_initial_vote==1));
        owners.push(owner(msg.sender, _initial_vote));
        owners.push(owner(_address_2, _initial_vote));
        owners.push(owner(_address_3, _initial_vote));        
    }
    
    function get_owner(uint _index) public view returns (owner memory ) {
        return owners[_index];
    }

    function deposit() public payable onlyOwners returns (uint) {
        balance+=msg.value;
        emit depositDone(msg.value);
        return balance;
    }
    
    function getBalance() public view returns (uint) {
        return balance;
    }
    
    function vote(uint __vote) public onlyOwners returns (owner memory) {
        // after finding the owner's index, put up his/her vote
        require((__vote==0) || (__vote==1));
        uint index = _find_owner_index(msg.sender);
        owners[index]._vote=__vote;
        return owners[index];
    }
    
    function withdraw(uint _amount) public enough_votes onlyOwners returns (uint) {
        balance-=_amount;
        return balance;
    }
    
    function _find_owner_index(address __address) private view returns (uint) {
        if (owners[0]._address==__address) {
            return 0;
        }
        if (owners[1]._address==__address) {
            return 1;
        }
        else {
            return 2;
        }
        
    }
    
    function _get_total_votes() private view returns (uint) {
        uint total_votes;
        for (uint i=0;i<3;i++){
            total_votes+=owners[i]._vote;
        }
        return total_votes;
    }

}
1 Like

Hey @ybernaerts, hope you are ok.

I have tested your contract, is working partially great, but there is plenty of space to improve.

I did not understand quite well how to withdraw the funds or vote.

    function vote(uint __vote) public onlyOwners returns (owner memory) {
        // after finding the owner's index, put up his/her vote
        require((__vote==0) || (__vote==1));
        uint index = _find_owner_index(msg.sender);
        owners[index]._vote=__vote;
        return owners[index];
    }
    
    function withdraw(uint _amount) public enough_votes onlyOwners returns (uint) {
        balance-=_amount;
        return balance;
    }

If i deploy the contract by using ‘1’ in the constructor for _initial_vote, then im able to deposit with an account and withdraw with another account, now the thing is, no one receive the funds, the withdraw function just substract the amount to the balance and thats it, so basically is not using properly the funds.

The other situation is with the vote, what it does exactly? cuz if your _initial_vote is 1, then all addresses are valid to withdraw at once.

Also I suggest you to add a error message in all your require, its help a lot when facing a revert error and does not show anything.

require(vote == 0, 'vote is not zero!')

Carlos Z

Thanks a lot Carloz. Frankly I did not expect so much great feedback!

I actually do know my code wasn’t perfect in the sense you describe here. For me it was important to have a good first draft that works and then go to the next course.

But part of the learning process it to continually improve on these things, so I take your comments to heart!

Thanks again!

1 Like

Short question: Is it really possible to have an array of addresses in the constructor? It compiles but I can’t deploy it from Remix. An array of strings works.I get the following error message:

creation of MultisigWallet errored: Error encoding arguments: Error: types/values length mismatch (count={"types":2,"values":3}, value={"types":["address[]","uint256"],"values":["[0x5B38Da6a701c568545dCfcB03FcB875f56beddC4,",",0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db]","2"]}, code=INVALID_ARGUMENT, version=abi/5.1.2)

from the following arguments:

_owners: [0x5B38Da6a701c568545dCfcB03FcB875f56beddC4,0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2,0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db]
_limit: 2

I see several solutions above passing an array of addresses in the constructor but does it really work? It seems most solutions use a fixed number of approvers, avoiding the problem (but not creating the contract as specified).

Please advice!

1 Like

Always glad to help sir :nerd_face:

Keep learning, we will be helping you over your journey in the Academy

Carlos Z

1 Like

It is possible, there are many solutions that does the same, the problem might be on the syntax of the array when your sending it to the constructor.

Try sending the array on this format, which is the same but the addresses we are sending them as an string, instead of a value.

["0x5B38Da6a701c568545dCfcB03FcB875f56beddC4", "0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2", "0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db"]

Check that each element of the array (which are addresses) are encapsulated has strings (""). Its a common mistake on remix :nerd_face:

Carlos Z

1 Like

pragma solidity 0.7.5;

import “./destroyable.sol”;

contract SmartContractWallet is Destroyable {

struct TransferRequest {
    address payable recipient;
    uint amount;
    address requestor;
    // would have been nice to use a mapping within a struct :(
    //mapping(address => bool) approvers;
    address[] approvers;
}

mapping (address => bool) walletOwners; // bool is true when the address represents a wallet owner
uint minimumApprovals;
TransferRequest[] transferRequests;

event Deposit(address indexed _from, uint _value);
event TransferRequested(uint requestId, address indexed requestor, address indexed recipient, uint amount);
event TransferApproval(uint requestId, address indexed approver);
event TransferCompleted(address indexed recipient, uint amount);

modifier onlyWalletOwners()
{
    require(walletOwners[msg.sender], "Must be requested from a wallet owner");  // evaluates to true when wallet owner
    _;
}

modifier approveOnce(uint requestId) {
    for (uint index = 0; index < transferRequests[requestId].approvers.length; index++) {
        require(transferRequests[requestId].approvers[index] != msg.sender, "You already approved this request");    // fails if msg.sender has already approved this request
    }
    _;
}

modifier incompleteTransfer(uint requestId) {
    require(transferRequests[requestId].amount > 0, "Transfer already completed");
    _;
}

modifier notWalletOwners(address _additionalWalletOwner) {
    require(!walletOwners[_additionalWalletOwner], "Not available for wallet owners"); // fails if attempt to add a second time
    _;
}

// The contract creator should be able to input the numbers of approvals required for a transfer, in the constructor
constructor() {
    minimumApprovals = 2;
}

// The contract creator should be able to input the addresses of the owners
function addWalletOwner(address _additionalWalletOwner) public onlyOwner notWalletOwners(_additionalWalletOwner) {
    walletOwners[_additionalWalletOwner] = true;
}

// Anyone should be able to deposit ether into the smart contract
function deposit() public payable {
    emit Deposit(msg.sender, msg.value);
}

// Anyone of the owners should be able to create a transfer request. The creator of the transfer request will specify what amount and to what address the transfer will be made.
function createTransferRequest(address payable _recipient, uint _amount) public onlyWalletOwners returns (uint) {
    require(_amount > 0, "Must request a transfer larger than zero");
    require(address(this).balance >= _amount, "Request amount exceeds wallet balance");
    address[] memory emptyApproversList;
    transferRequests.push(TransferRequest(_recipient, _amount, msg.sender, emptyApproversList));
    emit TransferRequested(transferRequests.length, msg.sender, _recipient, _amount);
    return transferRequests.length;
}

// Owners should be able to approve transfer requests.
function approveTransferRequest(uint transferRequestId) public payable onlyWalletOwners approveOnce(transferRequestId) incompleteTransfer(transferRequestId) {
    require(msg.value == 0, "Approval doesn't require any wei");
    require(address(this).balance >= transferRequests[transferRequestId].amount, "Not enough balance available to complete the requested transfer");
    transferRequests[transferRequestId].approvers.push(msg.sender);
    emit TransferApproval(transferRequestId, msg.sender);
    // When a transfer request has the required approvals, the transfer should be sent
    if (transferRequests[transferRequestId].approvers.length >= minimumApprovals) {
        transferRequests[transferRequestId].recipient.transfer(transferRequests[transferRequestId].amount);
        emit TransferCompleted(transferRequests[transferRequestId].recipient, transferRequests[transferRequestId].amount);
        transferRequests[transferRequestId].amount = 0;
    }
}

// not part of requirements, used for testing
function getTransferDetails(uint requestId) public view incompleteTransfer(requestId) returns (address, uint) {
    return (transferRequests[requestId].recipient, transferRequests[requestId].amount);
}

// not part of requirements, used for testing
function getBalance() public view returns (uint) {
    return address(this).balance;
}

}

1 Like

Below are the codes that I have been coding so far, it is not yet working. I am still working out and figuring out the final solution, any comments are welcomed, thank you.

pragma solidity 0.7.5;
pragma abicoder v2;

contract Wallet {
    address[] public owners;
    uint limit;
    
    struct Transfer{
        uint amount;
        address payable receiver;
        uint approvals;
        bool hasBeenSent;
        uint id;
    }
    
    Transfer[] transferRequests;
    
    mapping(address => mapping(uint => bool)) approvals;
    
    //Should only allow people in the owners list to continue the execution.
    modifier onlyOwners(){
      for (uint i = 0; i < limit; i++){
           if (msg.sender == owners[i]){
            _; // run the funct
           }
       }
    }
    //Should initialize the owners list and the limit 
    constructor(address[] memory _owners, uint _limit) {
        _owners = new address[](_limit);
    }
    
    //Empty function
    function deposit() public payable {}
    
    //Create an instance of the Transfer struct and add it to the transferRequests array
    function createTransfer(uint _amount, address payable _receiver) public onlyOwners {
        transferRequests.push (Transfer(_amount, _receiver, 0, false, uint(msg.sender)));
    }
    
    //Set your approval for one of the transfer requests.
    //Need to update the Transfer object.
    //Need to update the mapping to record the approval for the msg.sender.
    //When the amount of approvals for a transfer has reached the limit, this function should send the transfer to the recipient.
    //An owner should not be able to vote twice.
    //An owner should not be able to vote on a tranfer request that has already been sent.
    function approve(uint _id) public onlyOwners {
        
        if ((transferRequests[_id].approvals <= 0) && (transferRequests[_id].hasBeenSent == false)) {
            approvals[msg.sender][_id] = true;
        
            transferRequests[_id].approvals += 1;
        
            if (transferRequests[_id].approvals >= limit){
                createTransfer(transferRequests[_id].amount, transferRequests[_id].receiver);
                deposit();
                transferRequests[_id].hasBeenSent = true;
            }
        }
    }
    
    //Should return all transfer requests
    function getTransferRequests() public view returns (Transfer[] memory){
            return (transferRequests);
    }
}

1 Like

Here’s my solution. But first a few thoughts:

  • this was a valuable exercise but lots of struggle with how to structure the requests. I tried several solutions which required me to have a dynamic array of approvers in the request struct. But this would not work as I couldn’t initialize the array in the function creating the request. Lots of missing pieces here.
  • a better IDE is needed which allows me to debug, inspect variables, store program files
  • if I don’t mark functions as “view”, it’s not possible to see the function result in Remix. The createWithdrawalRequest function returns the transaction id but Remix won’t let me see the return value. Is this a bug?

I made a small change in the requirements: Anyone can create a “withdrawalRequest”, not only the owners (or approvers). At least for me, it makes sense as it makes the code cleaner and as the approvals are needed anyway, security isn’t compromised.

The code:

pragma solidity 0.7.5;

contract MultisigWallet {
    
    address[] public approvers;
    uint public requiredApprovals;
    struct WithdrawalRequest {
        address payable recipient;
        uint amount;
        uint currentApprovals;
    }
    mapping(uint => WithdrawalRequest) requests;
    mapping(address => mapping(uint => bool)) approvals;
    uint txid;
    
    constructor(address[] memory _approvers, uint _requiredApprovals) {
        approvers = _approvers;
        requiredApprovals = _requiredApprovals;
    }
    
    event depositDone(address indexed depositedFrom, uint amount);
    event withdrawalDone(address indexed withdrawnTo, uint amount);
    event withdrawalRequested(address indexed requestedBy, address recipient, uint amout);
    event withdrawalApproved(address indexed approvedBy, uint txid);

    function deposit() public payable returns (uint) {
        emit depositDone(msg.sender, msg.value);
        return address(this).balance;
    }
    
    function createWithdrawalRequest(address payable _recipient, uint _amount) public returns (uint) {
        requests[txid++] = WithdrawalRequest(_recipient, _amount, 0);
        emit withdrawalRequested(msg.sender, _recipient, _amount);
        return txid;
    }
    
    function isApprover(address sender) private returns (bool) {
        bool isAppr = false;
        for (uint8 i = 0; i < approvers.length; i++) {
            if (sender == approvers[i]) {
                isAppr = true;
                break;
            }
        }
        return isAppr;
    }
    
    function approveWithdrawalRequest(uint _txid) public returns (uint) {
        require(isApprover(msg.sender), "Sender is not approver!");
        if (!approvals[msg.sender][_txid]) {
            approvals[msg.sender][_txid] = true;
            emit withdrawalApproved(msg.sender, _txid);
            requests[_txid].currentApprovals++;
            if (requests[_txid].currentApprovals == requiredApprovals) {
                requests[_txid].recipient.transfer(requests[_txid].amount);
                emit withdrawalDone(requests[_txid].recipient, requests[_txid].amount);
                delete requests[_txid];
                delete approvals[msg.sender][_txid];
            }
        }
    }
    
    function getTransactionDetails(uint _txid) public view returns (address, uint) {
        WithdrawalRequest memory req = requests[_txid];
        return (req.recipient, req.amount);
    }
    
    function getBalance() public view returns (uint) {
        return address(this).balance;
    }
}
1 Like

Hey @yllee9127, hope you are ok.

I have tested your contract, it deploys properly, but there are few minor improvements to do on it :nerd_face:

    //Should only allow people in the owners list to continue the execution.
    modifier onlyOwners(){
      for (uint i = 0; i < limit; i++){
           if (msg.sender == owners[i]){
            _; // run the funct
           }
       }
    }

Modifiers are used to validate data using require, but this one does not contain any, there are many examples for this in this same topic from different students.

   //Should initialize the owners list and the limit 
   constructor(address[] memory _owners, uint _limit) {
       _owners = new address[](_limit);
   }

You are not assigning any address to the owners array…

Overall you have made a great progress, keep coding it a little bit cuz you almost have it !! :nerd_face:

Carlos Z

I’ve got to be honest, I wasn’t able to do anything on my own so I watched all videos first and spend days to understand all this and now I am happy to say that I can code that on my own with understanding all and without checking completed code although need to read comments what to do next and what need to be done, those are very helpful, but also tells me that I truly understand what I’m doing if someone tells me what to do :upside_down_face:

Although this code is almost identical to Filip’s one, here is my code:

pragma solidity 0.7.5;
pragma abicoder v2;

contract Wallet {
    address[] public owners;
    uint limit;
    
    struct Transfer{
        uint amount;
        address payable receiver;
        uint approvals;
        bool hasBeenSent;
        uint id;
    }
    
    event RequestCreated(uint _id, uint _amount, address requestedBy, address _receiver);
    event ApprovalReceived(uint _id, uint _approvals, address approvedBy);
    event TransferApproved(uint _id);
    
    Transfer[] transferRequests;
    
    mapping(address => mapping(uint => bool)) approvals;
    
    //Should only allow people in the owners list to continue the execution.
    modifier onlyOwners(){
        bool owner = false;
        for(uint i=0; i<=owners.length; i++){
            if(owners[i] == msg.sender){
                owner = true;
            }
        }
        require(owner == true);
        _;
    }
    //Should initialize the owners list and the limit 
    constructor(address[] memory _owners, uint _limit) {
        _owners = owners;
        _limit = limit;
    }
    
    //Empty function
    function deposit() public payable {}
    
    //Create an instance of the Transfer struct and add it to the transferRequests array
    function createTransfer(uint _amount, address payable _receiver) public onlyOwners {
        
        //emiting event RequestCreated(uint _id, uint _amount, address requestedBy, address _receiver)
        emit RequestCreated(transferRequests.length, _amount, msg.sender, _receiver);
        
        transferRequests.push(Transfer(_amount, _receiver, 0, false, transferRequests.length));
    }
    
    function approve(uint _id) public onlyOwners {
        //mapping(address => mapping(uint => bool)) approvals;
        require(approvals[msg.sender][_id] = false);
        require(transferRequests[_id].hasBeenSent = false);
        
        transferRequests[_id].approvals++;
        approvals[msg.sender][_id] = true;
        
        //emitting event ApprovalReceived(uint _id, uint _approvals, address approvedBy);
        emit ApprovalReceived(_id, transferRequests[_id].approvals, msg.sender);
        
        if(transferRequests[_id].approvals >= limit){
            transferRequests[_id].receiver.transfer(transferRequests[_id].amount);
            transferRequests[_id].hasBeenSent = true;
            //emitting event TransferApproved(uint _id);
            emit TransferApproved(_id);
        }
    }
    
    //Should return all transfer requests
    function getTransferRequests() public view returns (Transfer[] memory){
        return transferRequests;
    }
    
    
}
2 Likes

Hey @Kamil37, hope you are well.

The contracts works good, although you have mention to be the same from filip, let me know what you would like to know a little bit further so i can help you understand properly what ever you need :nerd_face:

Carlos Z

1 Like

Thanks mate, I think I understand all this pretty well by now, I just mentioned that at first I didn’t although not sure how could you do it any other way. I know Filip mentioned in the video that If I code it myself the code may look differently but functionality is the same. What I did I just watched it over and over again and also in few cases had to come back to few videos from solidity basics, until I understood all and basically can code it myself but only the way that Filip showed in the video.

Here is my solution, any feedback will be appreciated:

pragma solidity >=0.7.0 <0.9.0;

contract MultiWallet {
    
    //owner of the contract
    address private owner;
    
    //authorized addresses
    address[] authorized;
    
    //initialize transfer request array
    TransferRequest[] transferRequests;
    
    //keep track of authorized addresses which voted
    mapping(address => mapping(uint => bool)) votedForApproval;
    
    constructor() {
        owner = msg.sender;
        authorized.push(owner);
    }
    
    struct TransferRequest {
        uint amount;
        address payable recipient;
        uint8 approvals;
        bool completed;
    }
    
    //modifiers
    modifier onlyOwner() {
        require(msg.sender == owner);
        _;
    }
    
    modifier onlyAuthorized(){
      require(owner == msg.sender || authorized[1] == msg.sender || authorized[2] == msg.sender);
      _;
    }
    
    //add authorized address
    function addAdress(address buddyAddress) public onlyOwner {
        require(authorized.length < 3);
        for (uint8 index = 0; index < authorized.length; index++) {
            require(authorized[index] != buddyAddress);
        }
        authorized.push(buddyAddress);
    }
    
    //get authorized addresses
    function getAuthorized() public view onlyOwner returns(address[] memory) {
        return authorized;
    }
    
    //deposit
    function deposit() public payable onlyAuthorized () {
        
    }
    
    //create transfer request and push it in the array
    function transferRequest(uint amount, address payable recipient) public onlyAuthorized  {
        TransferRequest memory newTransferRequest = TransferRequest({
            amount: amount,
            recipient: recipient,
            approvals: 0,
            completed: false
        });
        transferRequests.push(newTransferRequest);
    }
    
    //get transfer requests
    function getTransferRequests() public view onlyAuthorized returns(TransferRequest[] memory){
        return transferRequests;
    }
    
    //approve transfer request 
    function approveRequest(uint index) public onlyAuthorized  {
        require(!votedForApproval[msg.sender][index]);
        votedForApproval[msg.sender][index] = true;
        transferRequests[index].approvals++;
    }
    
    //execute payment
    function executePaymentRequest(uint index) public payable onlyAuthorized  {
        TransferRequest storage request = transferRequests[index];
        require(!request.completed && request.approvals == 2);
        request.recipient.transfer(request.amount);
        request.completed = true;
    }
    
    //get contract balance
    function getBalance() public view onlyAuthorized returns(uint) {
        return address(this).balance;
    }
    
}

Hey @IvoS, hope you are ok.

I have tested your contract, after deployment, owner was added correctly, then i add 2 more addresses, then i deposited 3 eth with the 3rd owner, create a transaction Request with the contract owner to send 2 eth to 2nd address.

By far, all good. 2nd step:
I approve Request with 2nd and 3rd owner, then execute the transaction with 2nd owner and funds were sent to his wallet properly.

I also tried with the incorrect amount or authorized, all the reverts apparently trigger properly.

Some suggestions:
Its always great to have a error message in case a require is triggered and reverts the transactions, that help us identify why is being reverted.

Although this modifier is good, what would happen if i add an extra authorized? it will end into errors.

    modifier onlyAuthorized(){
      require(owner == msg.sender || authorized[1] == msg.sender || authorized[2] == msg.sender);
      _;
    }

since authorized is an array, it could grow more than 3 elements, i know there are good examples in this topic from other students that solve this issue in a nice way (using a loop to iterate the array).

Overall you have made a decent work! congratulations! :partying_face:

Carlos Z

Thanks for the feedback! :slightly_smiling_face:

1 Like

Hey guys, this is my multisig wallet. My goal was to not look at any of the solutions or hints at all. This is what I came up with. Feedback is highly appreciated since I’m still a programming noob and I want to improve my mistakes :smiley:

https://github.com/1heurer/multisigwallet/tree/main/MultiSigWallet