Project - Multisig Wallet

[0xtravis]

Here’s my contract. The biggest roadblock was figuring out how to have a mapping in a struct. More specifically creating instances of the struct when one of its elements is a mapping. I have it working after some research but the approach seems a little roundabout and could probably be improved. Is there a better way to handle that?

//SPDX-License-Identifier: MIT
pragma solidity 0.7.5.;

/**
We haven't gotten into actually signing transactions
so I've implemented this with a more simple approval pattern.
I hope that works.
*/

import "hardhat/console.sol";

contract MultiSigWallet {
	mapping(address => bool) approvers;
	uint private requiredApprovalCount;
	struct TransferRequest {
		uint amount;
		address payable toAddress;
		string description;
		address creator;
		mapping(address => bool) approvers;
		uint approvalCount;
		bool isComplete;
		bool exists;
	}
	mapping(uint => TransferRequest) public transferRequests;
	uint transferRequestCount;

	modifier onlyApprover {
		require(approvers[msg.sender], "address is not an approver");
		_;
	}

	modifier requestIndexIsInBounds(uint _requestIndex) {
		require(transferRequests[_requestIndex].exists, "transfer request doesn't exist");
		_;
	}

	constructor(address[] memory _approvers, uint _requiredApprovalCount) {
		console.log("Creating contract");

		// Add approvers
		for(uint i = 0; i < _approvers.length; i++) {
			approvers[_approvers[i]] = true;
		}

		requiredApprovalCount = _requiredApprovalCount;

		console.log("contract created");
	}

	function deposit() public payable {
		console.log("deposit of %d complete", msg.value);
	}

	function createTransferRequest(uint _amount, address payable _toAddress, string memory _description) public onlyApprover {
		// TODO: don't allow duplicate requests

		TransferRequest storage request = transferRequests[transferRequestCount++];
    	request.amount = _amount;
		request.toAddress = _toAddress;
		request.description = _description;
		request.creator = msg.sender;
		request.approvers[msg.sender] = true;
		request.approvalCount = 1;
		request.isComplete = false;
		request.exists = true;

		console.log("transfer request created");
	}

	function approveRequest(uint _requestIndex) public onlyApprover requestIndexIsInBounds(_requestIndex) {
		// Make sure request hasn't already been completed
        require(! transferRequests[_requestIndex].isComplete, "request has already completed");
		// Only allow one approval per approver
		require(! transferRequests[_requestIndex].approvers[msg.sender], "address already approved request");

		transferRequests[_requestIndex].approvers[msg.sender] = true;
		transferRequests[_requestIndex].approvalCount++;

		console.log("approval of request #$d complete", _requestIndex);
	}

	function completeTransfer(uint _requestIndex) public onlyApprover requestIndexIsInBounds(_requestIndex) {
		require(transferRequests[_requestIndex].approvalCount >= requiredApprovalCount, "request doesn't have enough approvals" );
		require(transferRequests[_requestIndex].amount <= address(this).balance, "insufficient balance");

		transferRequests[_requestIndex].toAddress.transfer(transferRequests[_requestIndex].amount);
		transferRequests[_requestIndex].isComplete = true;
		console.log("transfer complete");
	}

    function getBalance() view public returns(uint) {
        return address(this).balance;
    }
}

[0xtravis]

The biggest roadblock was figuring out how to have a mapping in a struct. More specifically creating instances of the struct when one of its elements is a mapping.

The error I was getting when doing it a way that made logical sense to me was:

Struct containing a (nested) mapping cannot be constructed

[thecil]

@0xtravis

	struct TransferRequest {
		uint amount;
		address payable toAddress;
		string description;
		address creator;
		mapping(address => bool) approvers;
		uint approvalCount;
		bool isComplete;
		bool exists;
	}

You cant have a mapping inside a struct

Carlos Z

[oioii1999]

Here attached my code.

Thanks for your review and comments.

pragma solidity 0.7.5;
pragma abicoder v2;

contract Wallet {
    uint approvalLimit;
    address[] public owners;
    uint accountBalance;

    struct Transfer {
        uint amount;
        address payable receiver;
        uint approvalNum;
        bool hasBeenSent;
        uint id;
    }
    Transfer[] transferRequests;
    mapping(address => mapping(uint => bool)) transferApprovals;
    mapping(uint => mapping(address => bool)) transferHandled;

    //Should only allow people in the owners list to continue the execution
    modifier onlyOwners() {
        bool isOwner = false;
        for (uint i = 0; i < owners.length; i++) {
            if (owners[i] == msg.sender) {
                isOwner = true;
            }
        }
        require(isOwner == true);
        _;
    }

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

    // Every owner should be able to deposit either
    function deposit() public payable onlyOwners {
        accountBalance += msg.value;
    }

    // Check account balance
    function getBalance() public view onlyOwners returns (uint) {
        return accountBalance;
    }

    //Create an instance of the Transfer struct and add it to the transferRequests array
    function createTransfer(uint _amount, address payable _receiver) public onlyOwners {
        require(accountBalance >= _amount);

        Transfer memory t;
        t.amount = _amount;
        t.receiver = _receiver;
        t.approvalNum = 0;
        t.hasBeenSent = false;
        t.id = transferRequests.length;

        transferRequests.push(t);
        for (uint i = 0; i < owners.length; i++) {
            transferHandled[t.id][owners[i]] = false;
        }
    }

    //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 {
        require(transferRequests[_id].hasBeenSent == false);
        require(transferHandled[_id][msg.sender] == false);

        Transfer storage t = transferRequests[_id];
        t.approvalNum += 1;
        transferApprovals[msg.sender][_id] = true;
        transferHandled[_id][msg.sender] = true;

        if (t.approvalNum >= approvalLimit) {
            require(accountBalance >= t.amount);
            t.receiver.transfer(t.amount);
            t.hasBeenSent = true;
            accountBalance -= t.amount;
        }
    }

    //Should return all transfer requests
    function getTransferRequests() public view returns (Transfer[] memory){
        return transferRequests;
    }
}

[0xtravis]

Hmm… but the working code is using a mapping inside a struct, just in a somewhat roundabout way. From what I’ve seen online it is possible but with limitations.

[Joeyj007]

// SPDX-License-Identifier: MIT
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 TransferRequestCreated(uint _id, uint _amount, address _initiator, address _receiver);
    event ApprovalReceived(uint _id, uint _approvals, address _approver);
    event TransferApproved(uint _id);

    Transfer[] transferRequests;

    mapping(address => mapping(uint => bool)) approvals;
    modifier onlyOwners(){
        bool owner = false;
        for(uint i=0; i<owners.length;i++){
            if(owners[i] == msg.sender){
                owner = true;
            }
        }
        require(owner == true);
        _;
    }
    constructor(address[] memory _owners, uint _limit) {
        owners = _owners;
        limit = _limit;
    }

    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 {
        emit TransferRequestCreated(transferRequests.length, _amount, msg.sender, _receiver);
        transferRequests.push(
            Transfer(_amount, _receiver, 0, false, transferRequests.length)
        );

    }
    
    function approve(uint _id) public onlyOwners {
        require(approvals[msg.sender][_id] == false);
        require(transferRequests[_id].hasBeenSent == false);

        approvals[msg.sender][_id] = true;
        transferRequests[_id].approvals++;

        emit ApprovalReceived(_id, transferRequests[_id].approvals, msg.sender);

        if(transferRequests[_id].approvals >= limit){
            transferRequests[_id].hasBeenSent = true;
            transferRequests[_id].receiver.transfer(transferRequests[_id].amount);
            emit TransferApproved(_id);
        }
    }

    function getTransferRequests() public view returns (Transfer[] memory){
        return transferRequests;
    }


}

[limitlessandrew]

So, I’m just attempting to reproduce your code from this video, however when I try to implement the final approval with one of the top 3 owner addresses I receive an error. I can’t seem to get the final approval using the code you developed. This code should be identical to yours currently. If I am getting the error that I just explained, why could it be happening?

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 TransferRequestCreated(uint _id, uint _amount, address _initiator, address _receiver);
    event ApprovalReceived(uint _id, uint _approvals, address _approver);
    event TransferApproved(uint _id);


    Transfer[] transferRequests;

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

    //Should only allow people in the owners list and the limit
    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 {
        emit TransferRequestCreated(transferRequests.length, _amount, msg.sender, _receiver);
        transferRequests.push(
            Transfer(_amount, _receiver, 0, false, transferRequests.length)
        );
    }

    //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 transfer request that has already been sent.
    function approve(uint _id) public onlyOwners {
        require(approvals[msg.sender][_id] == false);
        require(transferRequests[_id].hasBeenSent == false);

        approvals[msg.sender][_id] = true;
        transferRequests[_id].approvals++;

        emit ApprovalReceived(_id, transferRequests[_id].approvals, msg.sender);

        if(transferRequests[_id].approvals >= limit){
            transferRequests[_id].hasBeenSent = true;
            transferRequests[_id].receiver.transfer(transferRequests[_id].amount);
            emit TransferApproved(_id);
        }
    }

    function getTransferRequests() public view returns (Transfer[] memory){
        return transferRequests;
    }

}

[limitlessandrew]

Somehow it worked when I tried the owner addresses out of order. Thanks for the help coding.

[ekr990011]

So here is my attempt with only the first video.

Luckily I was successful with my testing!

I initially tried without a struct and was trying to use mappings but because a mapping never allows you to search the keys or the length of the mapping I kept running into problems with logic.

Could use an array to store the the signatories address keys for the mapping for approval which is eventually what I did with using an Owners struct.

I wanted to make it so that approvals could be mapped directly to their signatory so that one could not approve multiple times themselves.

// SPDX-License-Identifier: MIT

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Multisig is Ownable {
    struct Owners {
        address[] owners;
        mapping (address => bool) approval;
        uint mOfN;
        address payable transferTo;
        uint amount;
    }

    Owners public owners;

    // modifier to make it so that only owners can perform some certain requests
    // like approving a transaction or make a transaction request
    modifier onlyOwners(address _requester) {
        bool ownerCheck = false;

        for (uint i = 0; i < owners.owners.length; i++) {
            if(owners.owners[i] == _requester ) { ownerCheck = true; }
        }
        require(ownerCheck, "Not a signatory in this multisig wallet!");
        _;
    }

    event Approvals(uint _approvals);
    event TransferFails(string);
    event TransferSuccess(string, uint);
    event transferRequestCleared(string);

    // Owners put into an array so that we can have their keys to map to their
    // approval. mOfN to give the required amount of approvals to approve
    // a transfer request. This can be checked by running through the mapping later
    constructor(address[] memory _owners, uint _mOfN) {
        require(_mOfN != _owners.length + 1);
        
        owners.owners.push(owner);
        for (uint i = 0; i < _owners.length; i++) {
            owners.owners.push(_owners[i]);
        }
        owners.mOfN = _mOfN;
    }

    // The owner that requests a transfer should not need to also run approval
    // their intention is implied by making the transfer request
    function transferRequest(address payable _transferTo, uint _amount) onlyOwners(msg.sender) public {
        owners.transferTo = _transferTo;
        owners.amount = _amount;

        owners.approval[msg.sender] = true;
    }

    function transferApproval() onlyOwners(msg.sender) public {
        owners.approval[msg.sender] = true;

        if(checkMofN() == true) {
            transfer();
        } else {
            emit TransferFails("Transfer not at required approval # yet");
        }
    }

    function checkMofN() internal returns(bool) {
        uint approvals;
        for(uint i = 0; i < owners.owners.length; i++) {
            if(owners.approval[owners.owners[i]]) { approvals++; }
        }

        emit Approvals(approvals);

        return(owners.mOfN <= approvals);
    }

    function transfer() internal {
        (bool sent, bytes memory data) = owners.transferTo.call{value: owners.amount * 1 ether}("");
        require(sent, "Failed to send");
        clearTransfer();

        emit TransferSuccess("Transfer success of ether amount", owners.amount);
    }

    // Clear the transaction request so that the same request is not accidentally run
    // again upon any owner approving a transfer
    function clearTransfer() internal {
        owners.amount = 0;
        owners.transferTo = address(0);

        for (uint i = 0; i < owners.owners.length; i++) {
            owners.approval[owners.owners[i]] = false;
        }

        emit transferRequestCleared("Transfer completed, request reset.");
    }

    function deposit() payable public {}

    function getBalance() public view returns(uint) {
        return address(this).balance;
    }

}

Had some extra fun getting my head around the actual transfer to an address. Of course the contract already controls its own address so the call to send to another address does not require a “from” address.

I initially did not put * 1 ether and kept wondering why my transfer failed. Then I realized it had worked – as it showed up in the console – but had only sent the request in wei!

Got it all figured out and despite having to look up a few things pretty happy with my attempt. Took me a lot longer but I didn’t look at any of the help videos or the forums yet!

I saw some really good ideas above where they put the transfer request into an array of struct. This allows more than one request at a time instead of the explicit reset of my transfer request, just pop it off the array when it is approved!

This also makes me think I could have done better defining a Transfer struct like others have instead of putting the signatories in the same struct that the transfer request details are.

[decrypter]

pragma solidity 0.7.5;
pragma abicoder v2;

import “…/Inheritance/Destroyable.sol”;

contract MultiSig is Destroyable {

address[] private owners;
uint private req_approvals;

struct Transfer {
uint amount;
address payable recipient;
uint approvals;
bool sent;
}

// Note 1: this will only grow since we want to keep track of all transfers.
// Note 2: we use the array index for the transfer id.
Transfer[] transfers;

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

event fundsDeposited(uint amount, address indexed depositedTo);

modifier enoughBalance (uint amount_) {
require(address(this).balance>= amount_, “Not enough funds”);
_; //Run the function
}

constructor(address[] memory _s, uint _appr){
// We dont worry about double addreses in owners for now. Possible imporvement for later.
owners=_s;
req_approvals=_appr;
}

function getTransfer(uint tid_) view public returns (Transfer memory) {
return transfers[tid_];
}

function isOwner() view public returns (bool){
//
bool result=false;
for (uint index=0;index<owners.length;index++){
if (owners[index]==msg.sender) {
result=true;
break;
}
}
return result;
}

function approve (uint tid_) public{
require (isOwner(), “Only owners may approve”);
require (approvals[msg.sender][tid_]==false, “Already approved”);
approvals[msg.sender][tid_]=true;
transfers[tid_].approvals++;
if (transfers[tid_].approvals>=req_approvals && transfers[tid_].sent==false) {
// >= should not be required. Maybe just == since the >= should never happen…
transfer(transfers[tid_].recipient, transfers[tid_].amount);
transfers[tid_].sent=true;
}
}

function newTransfer (address payable recipient_, uint amount_) public returns (int) {
require(address(this)!= recipient_, “Dont transfer to yourself”);
require (isOwner(), “Only owners may create new transfers”);
int tid; // Transaction id
Transfer memory nt;
nt.amount=amount_;
nt.recipient=recipient_;
nt.approvals=0;
nt.sent=false;
tid=int(transfers.length); // ID is the size of the array before the push
transfers.push(nt);
approve (uint(tid)); // We assume that the creator of a new transfer also wants to approve it so we do that right away.
return tid;
}

function deposit() public payable returns(uint) {
emit fundsDeposited(msg.value, msg.sender);
return address(this).balance;
}

function transfer (address payable recipient_, uint amount_) enoughBalance(amount_) private {
uint previousSenderBalance=address(this).balance;
recipient_.transfer(amount_);
assert (address(this).balance==previousSenderBalance-amount_);
}

function getBalance () view public returns(uint){
return address(this).balance;
}
}

[decrypter]

I noticed a strange behavior with this code. The transfer works but it seems like the receiver pays the fee. I thought the caller always pays the transaction cost.

[mcgrane5]

can you please post the code properly in a code snippet. it is hard to read other wise

[decrypter]

pragma solidity 0.7.5;
pragma abicoder v2;

import "../Inheritance/Destroyable.sol";

contract MultiSig is Destroyable {

address[] private owners;
uint private req_approvals;

struct Transfer {
    uint amount;
    address payable recipient;
    uint approvals;
    bool sent;
}

// Note 1: this will only grow since we want to keep track of all transfers.
// Note 2: we use the array index for the transfer id.
Transfer[] transfers; 

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

event fundsDeposited(uint amount, address indexed depositedTo);

modifier enoughBalance (uint amount_) {
    require(address(this).balance>= amount_, "Not enough funds");
    _; //Run the function
}

constructor(address[] memory _s, uint _appr){
// We dont worry about double addreses in owners for now. Possible imporvement for later.
    owners=_s;
    req_approvals=_appr;
}

function getTransfer(uint tid_) view public returns (Transfer memory) {
    return transfers[tid_];
}

function isOwner() view public returns (bool){
    // 
    bool result=false;
    for (uint index=0;index<owners.length;index++){
        if (owners[index]==msg.sender) {
            result=true;
            break;
        }
    }
    return result;
}

function approve (uint tid_) public{
    require (isOwner(), "Only owners may approve");
    require (approvals[msg.sender][tid_]==false, "Already approved");
    approvals[msg.sender][tid_]=true;
    transfers[tid_].approvals++;
    if (transfers[tid_].approvals>=req_approvals && transfers[tid_].sent==false) {
    // >= should not be required. Maybe just == since the >= should never happen...
        transfer(transfers[tid_].recipient, transfers[tid_].amount);
        transfers[tid_].sent=true;    
    }    
}

function newTransfer (address payable recipient_, uint amount_) public returns (int) {
    require(address(this)!= recipient_, "Dont transfer to yourself");
    require (isOwner(), "Only owners may create new transfers");
    int tid; // Transaction id
    Transfer memory nt;
    nt.amount=amount_;
    nt.recipient=recipient_;
    nt.approvals=0;
    nt.sent=false;
    tid=int(transfers.length); // ID is the size of the array before the push
    transfers.push(nt);
    approve (uint(tid)); // We assume that the creator of a new transfer also wants to approve it so we do that right away.
    return tid;
}

function deposit() public payable returns(uint) {
    emit fundsDeposited(msg.value, msg.sender);
    return address(this).balance;
}

function transfer  (address payable recipient_, uint amount_) enoughBalance(amount_) private {    
    uint previousSenderBalance=address(this).balance;    
    recipient_.transfer(amount_);
    assert (address(this).balance==previousSenderBalance-amount_);
}

function getBalance () view public returns(uint){
     return address(this).balance;
}
}

[mcgrane5]

you need to create a balance mapping so you can keep track of each users balance in the contract. and then deduce the balance of the ower who is transfering the funds.

[decrypter]

Right… I didn’t think about anybody else than me using the wallet Hold on…

[mcgrane5]

so the idea of the multisig is that you have multiple users who are owners of the wallet. and in order to spend funds you must have consensus between all the owners. so all owners hsould be able to make transfers

[mcgrane5]

and just to clarify the person paying the fee will always be the person calling the transfer function

[decrypter]

@mcgrane5

ultisig is that you have multiple users who are owners o

Yeah, I got that part. And that is implemented. Let me just add the multi user functionality as well.

[mcgrane5]

even from your above statement the recipient wont be the one paying the fee. as msg.sender always pays the fee

[decrypter]

I cant reproduce that behavior in Remix. Not the whole sent amount is credited to the recipient.