Inheritance Assignment

Ethereum Smart Contract Programming 101
#1298
  • (1) Your full code from the 2 files containing your Ownable and Destroyable contracts;
pragma solidity 0.7.5;

my contract ownable 

contract ownable {
      address owner; 
    
    modifier onlyOwner {
        
        require(msg.sender == owner);
        _; 
    }
    
    constructor() {
        owner = msg.sender;
    }
    
}

my contract destroyable

pragma solidity 0.7.5;

import "ownable.sol";

contract destroyable is ownable {
    
    function close () public onlyOwner {
        selfdestruct(msg.sender);
    }
}

pragma solidity 0.7.5;

import "ownable.sol";
import "Destroyedable.sol";

contract Bank is ownable, destroyable {
1 Like
#1299

An excellent assignment solution @ZED91 :muscle:

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable.

Also, well done for realising that you needed to make the owner address payable , so that selfdestruct() is called with a payable address and, therefore, any remaining ether in the contract can be transferred to the owner when selfdestruct() is triggered.

Just one observation…
From v0.7 constructors no longer need to be marked with public or external visibility. Here is a link to the relevant section of the Solidity v0.7.0 Breaking Changes page in the documentation. This change appears in the 1st bullet point:
https://docs.soliditylang.org/en/v0.7.5/070-breaking-changes.html#functions-and-events
The compiler should have generated an orange/yellow warning for this. Your code will still work as it is, but it would be better to remove public.

Just let me know if you have any questions :slight_smile:

1 Like
#1300

Hi @suhan_kim,

For your Destroyable.sol code, you’ve posted another copy of your Ownable contract. Your Destroyable contract is missing. Maybe this was a copy-and-paste error. Can you post Destroyable so we can see that too?

#1301

Hi @GahuckImGoofy,

Your solution is correct :ok_hand:

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable.

That’s correct. If the address calling getBalance() had funds in the contract before it was destroyed, then this is how we can check that the contract has been successfully destroyed. But it also highlights a weakness of selfdestruct(), which is that after it has executed, successful calls can still be made to the destroyed contract’s functions! This isn’t really an issue with functions such as getBalance, but it is a serious problem if a user isn’t informed that the contract has been destroyed and unwittingly sends ether to the contract (e.g by calling the deposit function) because it will be lost.

Just let me know if you have any further questions.

#1302

Nice solution @DHash :ok_hand:

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable.

I’ve noticed that you didn’t post a solution for the Transfer Assignment, which was to complete the withdraw() function. But as you’ve posted your complete Bank contract, I’ll review that here for you as well…

Apart from the onlyOwner modifier (which you don’t need to add), your withdraw() function is correct.

By adding the onlyOwner modifier, only the contract owner can withdraw funds up to their own individual balance. But the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, don’t you think? :sweat_smile:

The statements in your withdraw() function body are in the correct order to reduce security risk:

  1. check inputs (require statements)
  2. effects (update the contract state)
  3. external interactions

Just as you have done, it’s important to modify the contract state for the reduction in the balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external wallet address…

msg.sender.transfer(amount);

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. You’ll learn about the type of attack this prevents, and how it does it, in the courses which follow this one.

Just let me know if you have any questions.

1 Like
#1303

pragma solidity 0.7.5;
import “./Ownable.sol”;

contract Destoryable is Ownable {

function destory() public onlyOwner {
    selfdestruct(owner);
}

}

1 Like
#1304

Nice solution @suhan_kim :ok_hand:

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable.

Also, well done for realising that you needed to make the owner address payable , so that selfdestruct() is called with a payable address and, therefore, any remaining ether in the contract can be transferred to the owner when selfdestruct() is triggered.

#1305

Nice solution @Precious :ok_hand:

We can also streamline the inheritance structure by having Bank explicitly inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

Just one other observation…
In Solidity, we normally start the names we give to contracts with a capital letter. We also do this with events, structs and interfaces. This helps us to distinguish these from the names of variables, mappings, arrays, parameters and modifiers, which we normally start with a lower case letter. It is easier to read and manage code when these conventions are applied consistently.

Just let me know if you have any questions :slight_smile:

#1306

An excellent solution @jaejang22 :muscle:

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable.

You’re making great progress!

#1307

Cool, thanks for your feedback. Will check out the link

Cheers!

1 Like
#1308

Nice solution @adykor :ok_hand:

Just a couple of observations:

  • You don’t need to repeat the constructor in Destroyable, as this is already inherited (together with the owner state variable and the onlyOwner modifier) from Ownable. Your current solution still works, but you can remove the constructor from Destroyable.

  • We can also streamline the inheritance structure by having Bank explicitly inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

Just let me know if you have any questions :slight_smile:

#1309
  1. import “./Ownable.sol”;

contract Destroyable{

function close()public onlyOwner{
   address payable receiver = msg.sender;
   selfdestruct(receiver);

}

}

  1. pragma solidity 0.7.5;

contract Ownable {
address internal owner;

modifier onlyOwner {
    require(msg.sender == owner);
    _; //run function
}

constructor(){
    owner = msg.sender;
    
}

}

#1310

I set up a multi-level inheritance:

Ownable -> Destroyable -> Bank

Ownable.sol

pragma solidity 0.8.4;

contract Ownable {
    address owner;
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _; //run the function
    }
    
    constructor() {
        owner = msg.sender;
    }
}

Destroyable.sol

pragma solidity 0.8.4;

import "./Ownable.sol";

contract Destroyable is Ownable {
    
    function destroy() onlyOwner payable public {
        address payable owner;
	    owner.transfer(address(this).balance);
	    selfdestruct(owner);
    }

}

Bank.sol header

pragma solidity 0.8.4;

import "./Destroyable.sol";

contract Bank is Destroyable {

I didn’t receive errors in Remix but welcome feedback for improvement.

Also, 2 questions:

  1. It seems like the selfdestruct function is a nuclear option - is it a better practice in production to use something like Openzeppelin’s “pausable” function instead, to hedge against a zero day exploit, catastrophe, etc. with the contract, then fix as needed and then “unpause”?

  2. Also, it seems like including self-destruct in a contract might damage trust with the community behind the project, because of fears of a potential rug-pull. What are your thoughts on including self-destruct? (Or is this type of thing covered in the 2nd course?)

Thanks-

#1311

The Ownable contract:

pragma solidity 0.7.5;

contract Ownable{
    
    address public owner;
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _;  
    }
    
    constructor(){
        owner = msg.sender;
    }
    
}

The self destroyable contract:

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable{
    
    function destroy() public onlyOwner {
        selfdestruct (msg.sender);
    }
    
}

Hearder of the Bank contract:

pragma solidity 0.7.5;

import "./Ownable.sol"; 
import "./Destroyable.sol";

contract Bank is Ownable, Destroyable {
1 Like
#1312

Hi @Epicdisaster,

Your Destroyable contract won’t compile because it’s not inheriting the onlyOwner modifier. It’s not enough to just import Ownable.sol. You need to add some additional code to establish Destroyable as a derived contract of Ownable.

The point of the assignment is to be able to deploy Bank, perform some transactions and then for the contract owner to destroy the Bank contract. So, we also need to see the first few lines of your Bank.sol file, up to and including the contract header, to see how you’ve coded Bank’s inheritance relationship within the overall inheritance structure.

Let me know if anything is unclear, or if you have any questions.

#1313

Hi @WMXgTW6wh,

Your implementation of a multi-level inheritance structure is well coded, and is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable.

The contract owner is able to destroy Bank by calling destroy(), and this action is appropriately restricted to the contract owner.

However, if you deploy Bank, perform some transactions, and then call selfdestruct() when there are still funds remaining in the contract, the contract balance is not transferred to the contract owner’s external address (the address that was displayed in the Account field, in the Deploy & Run Transactions panel, when the contract was deployed).

To see this for yourself, you need to check what the owner’s external address balance is before calling destroy(), and make sure you also know how much ether should still be in the contract at this point. Then, after calling destroy(), you should see the owner’s external address balance increase by that amount.

You can also add the following function to your Bank contract, which will allow you to view the total contract address balance at any point in time, instead of having to try to remember it.

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

The contract owner’s address is the address which is assigned to the owner state variable (declared in Ownable) via the constructor, and which is inherited by Destroyable and available in that contract. But by defining a separate local variable with the same name (owner) within destroy(), it appears that you have overridden the state variable; and so by not assigning any address to this local owner variable, you are calling transfer() on a zero address and passing a zero address to selfdestruct().

You are right that the address receiving the contract’s remaining ether balance needs to be a payable address, but in order to convert the contract owner’s address stored in owner to a payable address, and ensure that this payable address receives the remaining funds, then you need to do either of the following:

  1. Declare the owner state variable in Ownable with a payable address type, instead of with the default, non-payable address type.
address payable owner;

You can then just reference it as follows:

      function destroy() public onlyOwner {
          selfdestruct(owner);
      }
  1. If you only need the contract owner’s address to be payable for the purposes of executing selfdestruct(), you can leave the owner state variable as non-payable, and explicitly convert the address to payable locally, within the function where you actually need to use it as payable. You can even do this directly within the selfdestruct() function call itself, as follows:
      function destroy() public onlyOwner {
          selfdestruct(payable(owner));
      }

You will notice that I haven’t included   owner.transfer(address(this).balance);   or   payable(owner).transfer(address(this).balance);
This is because, as well as destroying the contract, selfdestruct() automatically transfers the remaining contract balance to the address argument it is called with. This is all part of the pre-defined selfdestruct() functionality.

Also, the destroy() function itself shouldn’t be marked as payable. A function only needs to be payable when it receives ether from an external address in order to update the contract balance. That’s why we marked our deposit function in Bank as payable, but not our withdraw function.

Yes, I agree with both of the issues you’ve raised, and using the pausable pattern does have many advantages over the more “primitive” selfdestruct(). The following blog post (which you may already have read) sets out and explains the reasons for this:

https://blog.b9lab.com/selfdestruct-is-a-bug-9c312d1bb2a5

Using selfdestruct() brings with it certain clear disadvantages, the potential repercussions of which need to be fully understood before choosing to implement it. After selfdestruct() has executed, successful calls can still be made to the destroyed contract’s functions! As a result of this, if a smart contract deployed on the mainnet is ever destroyed, all users need to be notified and informed that they must not send any more funds to that contract address, otherwise those funds will be lost.

However, for the purposes of this introductory course, selfdestruct() provides a simple and straightforward way to practise implementing inheritance, which is our main objective in this assignment. Pausable contracts involve more advanced techniques and are therefore out of the scope of this introductory course. You will learn about pausable contracts and other smart-contract emergency and security features, such as proxy contracts, in the Smart Contract Security course, which I highly recommend you take either after this one, or after the Ethereum Smart Contract Programming 201 course.

Let me know if anything is unclear regarding the modifications you need to make to your solution, and also if you have any further questions :slight_smile:

#1314

An excellent solution @Nuno_Silva :muscle:

We can also streamline the inheritance structure by having Bank explicitly inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

Just let me know if you have any questions :slight_smile:

#1315

ownable.sol

pragma solidity 0.7.5;

contract Ownable {
    
    address payable public owner;
    
    modifier onlyOwner {
        require(msg.sender == owner);
    _;
    }
    constructor(){
        owner = msg.sender;
    }
}

destroyable.sol

pragma solidity 0.7.5;

import "./ownable.sol";

contract Destroyable is Ownable {
    
    function implode() public onlyOwner {
        selfdestruct(owner);
    }
}

bank.sol

pragma solidity 0.7.5;

import "./destroyable.sol";

contract Bank is Destroyable {
1 Like
#1316

Great info as usual, thanks very much. I chose the 2nd option - converting the owner address to payable within the destroy function, and I also appreciate the feedback on the selfdestruct function. All checks out in Remix now.

1 Like
#1317

Ownable.sol

pragma solidity 0.7.5;

contract Ownable {
    address payable private owner; 
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _;
    }
    
    constructor(){
        owner = msg.sender;
    }
}

Destroyable.sol


pragma solidity 0.7.5;

contract Destroyable {
    address payable private owner; 
	 
    constructor() {
        owner = msg.sender;
    }
	
	function close() public { 
		selfdestruct(owner); 
	}  
}

Banl.sol

pragma solidity 0.7.5;

import "./Ownable.sol";
import "./Destroyable.sol";

contract Bank is Ownable, Destroyable {
...
}