Ownable contract?

[William]

My brave Browser deleted ALL of my data, including all of my files on Remix.Ethereum.org…

I’m trying to salvage certain assignments for reference, and added this code:

pragma solidity 0.7.5;
pragma abicoder v2;
import "./Ownable.sol";

contract Bank is Ownable {
    
    mapping(address => uint) balance;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint)  {
        balance[msg.sender] = msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public onlyOwner returns (uint){
        require(balance[msg.sender] >= amount);
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        //govermentInstance.addTransaction(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] - amount;
        balance[to] + amount;
    }
      function getTransfers() external view returns(uint[] memory) {

    }
    
}

My question is, I can’t find the “Ownable” contract anywhere on Filip’s github. Can someone help me try to put together a simple Ownable contract that would work with the above code ^?

[William]

Would this work?

 pragma solidity 0.7.5;
 pragma abicoder v2;
 
 contract Ownable{
     
    address[] public owners;
 
    //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);
        _;
    }
 }

[Bhujanga]

// SPDX-License-Identifier: GPL-3.0

pragma solidity >=0.7.0 <0.8.0;

/**
 * @title Owner
 * @dev Set & change owner
 */
contract Ownable {

    address private owner;
    
    // event for EVM logging
    event OwnerSet(address indexed oldOwner, address indexed newOwner);
    
    // modifier to check if caller is owner
    modifier isOwner() {
        // If the first argument of 'require' evaluates to 'false', execution terminates and all
        // changes to the state and to Ether balances are reverted.
        // This used to consume all gas in old EVM versions, but not anymore.
        // It is often a good idea to use 'require' to check if functions are called correctly.
        // As a second argument, you can also provide an explanation about what went wrong.
        require(msg.sender == owner, "Caller is not owner");
        _;
    }
    
    /**
     * @dev Set contract deployer as owner
     */
    constructor() {
        owner = msg.sender; // 'msg.sender' is sender of current call, contract deployer for a constructor
        emit OwnerSet(address(0), owner);
    }

    /**
     * @dev Change owner
     * @param newOwner address of new owner
     */
    function changeOwner(address newOwner) public isOwner {
        emit OwnerSet(owner, newOwner);
        owner = newOwner;
    }

    /**
     * @dev Return owner address 
     * @return address of owner
     */
    function getOwner() external view returns (address) {
        return owner;
    }
}

[Bhujanga]

@William

dress[] public owners;

Note that in your code, there is no constructor that will set the owner when deploying the contract. Therefore, the contract does not know which address is an owner. The modifier looks good to me.

[Bhujanga]

When looking through your bank contract, I see that you are using transfer to move ether. I know that this is teached in the course. From practical work experience outside of the academy, I know that using transfer() or send() is not recommended anymore since the Instanbul Fork. Instead, a call() is used, or even better, a pull-pattern. I recommend you do some research about it, to learn best practices right away.

Read This…

[jon_m]

Hi @Bhujanga,

Nice Ownable contract

Did you write it yourself, or have you got it from somewhere else?

The only thing I would do differently is emit the event after assigning the new owner like this:

function changeOwner(address newOwner) public isOwner {
    address oldOwner = owner;
    owner = newOwner;
    emit OwnerSet(oldOwner, owner);
}

instead of before…

@Bhujanga

function changeOwner(address newOwner) public isOwner {
    emit OwnerSet(owner, newOwner);
    owner = newOwner;
}

I think it’s better practice, and always safer, to emit the event after the event has actually happened, like it has been here:

@Bhujanga

constructor() {
    owner = msg.sender; // 'msg.sender' is sender of current call, contract deployer for a constructor
    emit OwnerSet(address(0), owner);
}

@William

[jon_m]

Hi @William,

Your Bank contract has a few issues with it:

In the deposit function:

@William

balance[msg.sender] = msg.value;

This will replace the current balance with the deposit amount rather than adding to it. To add the deposit to the balance you need to change the assignmnet operator to  +=

Your _transfer function is calculating the adjustments to the sender and recipient’s balances, but it doesn’t assign them to the mapping:

@William

balance[from] - amount;
balance[to] + amount;

You need…

balance[from] = balance[from] - amount;
balance[to] = balance[to] + amount;

or the more concise…

balance[from] -= amount;
balance[to] += amount;

Your withdraw function is transferring the withdrawal amount from the contract’s ether balance to the sender’s external wallet address, but their balance in the mapping is not reduced! I’m sure you can see that this is a very serious bug! You need to add…

balance[msg.sender] -= amount;

… above  msg.sender.transfer(amount);

The order of the statements is important in order to reduce security risk:

1. check inputs (require statement)
2. effects (update the contract state for reduction in balance)
3. interactions (perform the transfer of funds from smart contract address to wallet address).

It’s important to modify the contract state:

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract:

msg.sender.transfer(amount);

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. We cover the type of attack this prevents, and how it does it, in the Smart Contract Security course.

---

@William

function getTransfers() external view returns(uint[] memory) {

}

This function is empty. And unless you start keeping a record of transfer data in an array, this function will be redundant, anyway. The output is an array of integers, which suggests to me an array of each amount transferred. But just the transfer amounts by themselves wouldn’t really be of any use, unless additional data about each transfer was also provided (e.g. sender and recipient’s addresses).

[jon_m]

@William

Can someone help me try to put together a simple Ownable contract that would work with the above code ^?

@William

Would this work?

I agree with @Bhujanga …

@Bhujanga

Note that in your code, there is no constructor that will set the owner when deploying the contract. Therefore, the contract does not know which address is an owner. The modifier looks good to me.

As your contract suggests multiple owners, you will need to add functionality to add the additional owners e.g.

address[] public owners;
    
constructor() {
    owners.push(msg.sender);
}

function addOwner(address owner) public onlyOwners {
     owners.push(owner);
}

You only need   pragma abicoder v2   in the file where you are returning the array, but even then, only if it is an array of strings, structs, or … arrays* (i.e. a complex data type). But from v0.8 we can return any type of array without needing  pragma abicoder v2
You don’t need  pragma abicoder v2  in either v0.7 or v0.8 if the array you are returning contains values with an elementary data type like uint[] . So, you don’t need it in either of your files.

* unless the arrays contain values with an elementary data type, and the outer array is a fixed-size array e.g. uint[5][]

And if your modifier is called onlyOwners , then you need to add the ‘s’ when it is used in withdraw() in Bank.

[Bhujanga]

True, it is better practice to emit the event after. Although I did see this in differently from developer to developer. This is not my own, but the ownable template you can find on remix.ethereum.org.

[jon_m]

This is a bit more complicated…

@jon_m

You only need pragma abicoder v2 in the file where you are returning the array (Bank, but not Ownable).

But, you also don’t need  pragma abicoder v2  if the array you are returning contains values with an elementary data type like uint[]. So you don’t need it in Bank either.
With v0.7 we only need it when we return an array of strings, structs, or … arrays* (i.e. a complex data type). But from v0.8 we can return any type of array without needing  pragma abicoder v2

* unless the arrays contain values with an elementary data type, and the outer array is a fixed-size array e.g. uint[5][]