Inheritance Assignment

[CryptoUsic]

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

contract Destroyable is Ownable {

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

contract Bank is Destroyable {

[Adrian1]

Contract for destroying

contract Destroyable{


    function Destroy() public    {
        selfdestruct(msg.sender);
    }


}

Ownable Contract

contract Ownable{

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

}

Modified bank contract (check the last few lines of code)

// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;
import "./Destroyable.sol" ;
import "./Ownable.sol";

contract Bank2 is Destroyable, Ownable{

  mapping(address => uint)Balance;
  
  event DepositDone(uint Amount, address DepositedTo);
  event TransactionDetails(address sender, address receiver, uint amount);
     
  

    function Deposit() public payable returns(uint){

        Balance[msg.sender] = Balance[msg.sender] + msg.value; //Adds ether to the contract
        emit DepositDone(msg.value, msg.sender);
        return Balance[msg.sender];
    }

    function Withdraw(uint amount) public OnlyOwner returns(uint){
        require(amount <= Balance[msg.sender]);

        msg.sender.transfer(amount); //transfers the Ether in the smart contract to the selected address

        Balance[msg.sender] = Balance[msg.sender] - amount;

        return Balance[msg.sender];

        
    }

    function transfer(address recipient, uint amount) public{

        require(Balance[msg.sender] >= amount, "Balance not enough");
        require(msg.sender != recipient, "You can't send funds to yourself!");

        uint BalanceBefore = Balance[msg.sender];

        _transfer(msg.sender, recipient, amount);

        assert( Balance[msg.sender] == BalanceBefore - amount);

         emit TransactionDetails(msg.sender , recipient, amount);

        //calls the transfer function, and takes the parameters of msg.sender(owner of an address), recipient and an amount
    }

    function _transfer(address from, address to, uint _amount) private{

        Balance[from] -= _amount;
        Balance[to] += _amount;

        //Subtracts balance from the sender and adds it to the recipient

    }
    
    function getBalance() public view returns(uint) {

        return Balance[msg.sender];

    }


    function DestroyContract() public OnlyOwner {
        Destroy();
    }
    
}

[jon_m]

Hi @Giupi,

Your solution meets all of the assignment objectives. Your implementation of a multi-level inheritance structure is also well coded and is the best approach: Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable

However, you haven’t taken full advantage of your inheritance structure, and you’ve included a lot of code which isn’t necessary. The same objectives can be met in a much more concise way …

(1) By giving your destroy() function in Destroyable internal visibility, it is inherited and available within Bank, but cannot be called externally by the contract owner. To resolve this you’ve added an additional public function to Bank which in turn calls destroy(). However, an important reason for using inheritance is to reduce code duplication, and if you change the visibility of the destroy() function from internal to public, it will be inherited and available to be called from both within Bank and externally from Remix. You could also give it external visibility, which would enable it to be called externally from Remix, but not from within Bank, which for the purposes of this assignment would be enough. This removes the need for the additional destroyContract() function in Bank.

destroy() will now appear as a function-call button in the Remix panel, instead of destroyContract()

Additionally, by keeping all of the code and functionality related to contract destruction within a single, separate contract (Destroyable), you will improve the modularity and re-usability of your code (other important reasons for using inheritance) and keep your code more organised.

(2)

@Giupi

balance[owner] += (address(this).balance - balance[owner]);
withdraw(balance[owner]);

These lines of code in your destroyContract() function achieve the desired result, and from a technical point of view they are well coded. However …

• Firstly, as the total contract balance is being transferred to the owner’s external address, and the contract then destroyed, there is no point in updating the mapping for this, in order to then be able to use the withdraw() function to transfer the funds. The following refactored code achieves the same result much more concisely …

     msg.sender.transfer(address(this).balance);

• Secondly, and more importantly, as well as destroying the contract, selfdestruct also automatically transfers the remaining contract balance to the payable address argument it is called with (in our case, the contract owner’s address). This is all part of the pre-defined selfdestruct functionality, and is why there is no need to perform the extra steps that you describe here …

@Giupi

I thought that after changing the owner’s balance, the function should have the owner withdraw() all the contract funds before calling destroy() and therefore destroying the contract, otherwise the funds will just be stuck in the contract and nobody would be able to access them.

So, in summary, and as a result of all of the above points, instead of the two functions you currently have (destroy and destroyContract) all you need is the following one in Destroyable…

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

A couple of additional observations …

(3)

@Giupi

function destroyContract() public payable onlyOwner {

Only functions that receive Ether from an external address (in order to add it to the contract address balance) should be marked payable (e.g. the deposit function in Bank). Your destroyContract() function doesn’t receive Ether: by calling withdraw() it deducts Ether from the contract address balance and sends it to an external address. Your destroy() function will also do the same when you change its visibility.

The method selfdestruct in your destroy() function does require a payable address argument. This is so it can transfer the remaining Ether in the contract to this receiving address when the contract is destroyed. You have already ensured that this address argument (owner, in your case) is payable, by defining the owner state variable in Ownable as a payable address.

(4) In your withdraw() function, it’s important to modify the contract state by reducing the individual user’s balance in the mapping by the withdrawal amount …

balance[msg.sender] -= amount;

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

msg.sender.transfer(amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one.

Just let me know if anything is unclear, or if you have any questions

[jon_m]

Hi @CryptoUsic,

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

Your solution works if your Destroyable contract is in the same file as Bank. However, better practice is to place each contract in their own separate file, so that Destroyable, as well as Ownable, can be re-used as an independent module. That would also mean adding an import statement to Bank.sol to import Destroyable.sol.

Also, calling selfdestruct() with msg.sender directly …

selfdestruct(msg.sender);

… is a more concise alternative to using the additional receiver variable …

@CryptoUsic

address payable receiver = msg.sender;
selfdestruct(receiver);

Have a look at this post for further details about the use of this additional local variable (receiver) in the model solution for this assignment.

Just let me know if you have any questions.

[jon_m]

Hi @Adrian1,

Your solution is almost there, except for one very serious flaw. If you deploy your Bank contract, you will notice that there are 2 functions that can be called from Remix to destroy the contract and transfer the Ether remaining in the contract to the calling address …

1. DestroyContract() in Bank, which only allows the contract owner to destroy the contract and receive all of the remaining funds

2. Destroy() in Destroyable can also be called directly from Remix because it has public visibility, and so is inherited by Bank and can also be called by an external address. Not only is this unnecessary code duplication, but also, more seriously, any address can call this function, destroying the contract and transferring all of the remaining funds to their own external address (not the contract owner’s), because calling this function directly bypasses the onlyOwner restriction which is only applied to DestroyContract()

One easy fix would be to change the visibility of the Destroy() function in Destroyable from public to internal. This would still allow it to be inherited, but only available to call from within Bank. This means that only the contract owner would be able to call DestroyContract(), and DestroyContract() could then still call Destroy(), destroying the contract and transferring the remaining funds to msg.sender (the caller of the DestroyContract function, which can only be the owner).

However, this would still leave you with unnecessary code duplication, and reducing code duplication is an important reason for using inheritance. So, if you keep Destroy() public and add the onlyOwner modifier to this function instead, you can remove DestroyContract() from Bank altogether. You’ll also need to change your inheritance structure, so that onlyOwner is available in Destroyable and not just Bank. But this will mean that Destroy() is now the only function available to call from Remix which can trigger selfdestruct, but this time only the contract owner will be able to perform this operation, destroy the contract and receive the remaining contract balance.

Additionally, by keeping all of the code and functionality related to contract destruction within a single, separate contract (Destroyable), you will improve the modularity and re-usability of your code (other important reasons for using inheritance) and keep your code more organised.

---

A couple of additional comments …

• Always include the pragma statements for each file in your posted solutions. Some students choose to use different Solidity versions, and so it’s always best to confirm which version you’re using by including the pragma statements.

• You’ve modified the withdraw function with the onlyOwner modifier, but this means that only the contract owner will be able to withdraw funds while the Bank contract is deployed and operating normally. The contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so, while the contract is still deployed, it only seems fair that all users should be able to withdraw their funds as well, don’t you think? Your withdraw() function header didn’t include the onlyOwner modifier before. I think during the course we were adding and removing onlyOwner from various functions to demonstrate different scenarios, and I think it might have ended up being left in the withdraw() function in the solution code for this assignment by mistake!

Just let me know if anything is unclear, if you have any questions, or if you need any help modifying your code for these points.

[peg]

Hey Filip,

ownable.sol

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

destroyable.sol

import "./ownable.sol";

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

bank.sol

import "./destroyable.sol";

contract Bank is Destroyable {
//...

Initially I worked out sending by creating close() function in bank.sol:

function close() public onlyOwner {
        payable(owner).transfer(address(this).balance);
        super.destroy();
}

But then I read a bit more and realized selfdestruct(owner) actually sends all funds to owner, so that is simpler and works good far as I tested.

I have question…since there is payable “deposit” function in Bank contract. After contract is destroyed any sending there will not “revert” and will take/burn funds sent…that’s sort of understandable but…
…would it be good to make some kind of checks/fallbacks on payables that so we know if there’s active contract and maybe revert transaction? Or that is not possible, since contract won’t do any code when functions are called.
I understand from our frontend we can disable calls - but people still may interact with contract directly. If you can clarify that part and maybe how that’s done.

Thanks,
Peg

[Giupi]

Hi Jon, thanks for your response, I found it really helpful and I modified the contract according to your suggestions. Everything is a lot clearer now, I’m looking forward to more feedback on the following assignments.
Thanks again,
Giupi.

[jon_m]

Hi @peg … welcome to the forum! I hope you’ve been enjoying this course

First of all … your final version is a good solution
Your implementation of a multi-level inheritance structure is the best approach. Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable.

@peg

Initially I worked out sending by creating close() function in bank.sol:

function close() public onlyOwner {
        payable(owner).transfer(address(this).balance);
        super.destroy();
}

But then I read a bit more and realized selfdestruct(owner) actually sends all funds to owner, so that is simpler and works good far as I tested.

Yes … As well as destroying the contract, selfdestruct also automatically transfers the remaining contract balance to the payable address argument it is called with (in our case, the contract owner’s address). This is all part of the pre-defined selfdestruct functionality, and is why there is no need to include the extra line of code …

@peg

payable(owner).transfer(address(this).balance);

Including your additional close() function in Bank does also achieve the desired result, and from a technical point of view it’s well coded. However, an important reason for using inheritance is to reduce code duplication. By giving the destroy() function in Destroyable public visibility, it will be inherited and available to be called from both within Bank and externally from Remix. You could also give it external visibility, which would enable it to be called externally from Remix, but not from within Bank, which for the purposes of this assignment would be enough. This removes the need for the additional close() function in Bank. If you deploy your Bank contract with the additional close() function included, you will notice that both close() and destroy() can be called by the contract owner from Remix. Both of these functions will produce exactly the same result, and so this also demonstrates why close() is unnecessary code duplication.

Additionally, by keeping all of the code and functionality related to contract destruction within a single, separate contract (Destroyable), you will improve the modularity and re-usability of your code (other important reasons for using inheritance). It also keeps your code more organised, and makes it more readable and easier to manage.

By the way, if we did need to call a function inherited from Destroyable from the derived contract Bank, we wouldn’t need to use super (or the Destroyable contract name) as you have …

@peg

super.destroy();

You can call the function using just its name:   destroy();

We only need to prepend the function name with super. or the contract name, e.g. Destroyable.destroy() , if there are functions with the same name in different contracts within the inheritance structure (polymorphism) and we need to call an overriden function; and super is only needed when an overriden function cannot be called using the contract name, because its contract is on a different “branch” of the same overall inheritance structure. For more detail on this, and a coded example, see the following section of the Solidity documentation (specifically, the bottom part with the 2nd and 3rd code examples) …

https://docs.soliditylang.org/en/latest/contracts.html#inheritance

---

@peg

since there is payable “deposit” function in Bank contract. After contract is destroyed any sending there will not “revert” and will take/burn funds sent

Well observed!

This is a serious disadvantage of using selfdestruct, the potential repercussions of which need to be fully understood before choosing to implement it. 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 (as you have discovered) those funds will be lost.

Obviously, this is hardly an ideal situation!

@peg

…would it be good to make some kind of checks/fallbacks on payables that so we know if there’s active contract and maybe revert transaction? Or that is not possible, since contract won’t do any code when functions are called.

You are right that this isn’t possible, because once selfdestruct() has been triggered, even though function calls can still be made from the web3 client (i.e. the frontend), the smart contract itself has been destroyed and its code removed from the blockchain; from this point the contract state ceases to exist. The zeros that are returned and, more seriously, any Ether that is sent and lost, are the unwanted side-effects of making external calls to a destroyed (and now non-existant) contract.

But, you are thinking along the right lines In practice, other types of security mechanisms, such as pausable or proxy contracts, can be implemented to deal with these kinds of emergency situations.

Instead of destroying the contract, which is permanent and irreversible, pausable contracts are able to provide a range of more nuanced solutions, where…

• an authorised address can both pause and unpause a contract; and
• by using two different modifiers, specific functions can be activated or deactivated depending on whether the contract is currently paused or unpaused.

The following blog post sets out and explains the reasons why a pausable contract has many advantages over the more “primitive” selfdestruct …

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

OpenZeppelin Contracts (see their GitHub repository, and README file) provides a smart contract library which includes a “ready-made” Pausable contract:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.3.0/contracts/security/Pausable.sol

The functionality provided by OpenZeppelin in this library can be implemented by inheriting the relevant contracts.

Even though using selfdestruct() has clear disadvantages, for the purposes of this introductory course it 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. So, don’t worry if you don’t fully understand everything discussed in the blog post, or all of the information and code in the other links to OpenZeppelin Contracts. You will learn about pausable contracts and other smart-contract emergency and security features, such as proxy contracts, if you go on to take the Ethereum Smart Contract Security course. The 201 course will also introduce more advanced concepts and techniques which will help you to more fully understand and work effectively with this material.

Just let me know if you have any further questions … and keep up the research

[peg]

Wow Jon thanks a lot for this detailed walkthrough and explanation - especially that “paused” openzeppelin contract I’m gonna definitely look into it - much appreciated…

Cheers!

[jon_m]

You’re very welcome @peg! … you’re asking some great questions! When you’ve got the time, I think you’ll find the info in the links really interesting and helpful.

[tbem]

contract Ownable{
  address payable public owner;

  modifier onlyOwner(){
    require(msg.sender == owner, "Not the owner of the contract!");
    _; // Run the function
  }
  constructor(){
    owner = msg.sender;
  }
}

contract Destroyable is Ownable{

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

}

contract Bank is Destroyable{

[jon_m]

Hi @tbem,

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

I’m assuming you must have included a pragma statement at the top of your file, otherwise it won’t compile and Bank can’t be deployed. If so, then your solution works, but only if your 3 contracts are all in the same file, because you haven’t included any import statements. However, better practice is to place each contract in their own separate file, so that Ownable and Destroyable can be re-used as independent modules to incorporate their specialised contract ownership and contract destruction functionalities. That would also mean adding an import statement to Destroyable.sol to import Ownable.sol, and to Bank.sol to import Destroyable.sol.

Let me know if you have any questions

[ismael_zamora]

here is my scram function

pragma solidity ^0.8.0;

import './Owneable.sol';

contract Destroyable is Owneable {

    function scram() public onlyOwner {
        selfdestruct(payable(owner));
    }

}

[North2L]

contract Destroyable is Ownable{

function hakai() public onlyOwner{
selfdestruct(payable(owner));
}

}

[jon_m]

That’s great @ismael_zamora

As it’s the Bank contract that we want to deploy and the contract owner to then destroy, can we also see how you’ve coded your Bank contract for the inheritance?

[jon_m]

Hi @North2L,

This is a great start, but can you post your complete solution: the full Destroyable.sol file including pragma and import statements; and the first few lines of Bank.sol up to and including the contract header?

As it’s the Bank contract that we want to deploy and the contract owner to then destroy, we need to see how you’ve coded your Bank contract for the inheritance.

If you don’t want to post your separate solutions to the other assignments in this course, you could post your full Bank contract for this assignment, and I’ll take a look at how you’ve coded that for the other assignment tasks like the withdraw() function and the transfer event

[ismael_zamora]

i delete the file to clean my remix sorry

[jon_m]

OK, Ismael, not to worry
… as long as you’re happy that your Bank contract was compiling and deploying successfully, and that after various different transactions had been performed, the contract owner (only) was able to destroy the contract and have the remaining contract balance sent to their external address.

[JonPag]

Ownable file.

pragma solidity 0.7.5;
contract Ownable {
address public owner;
}

Destroyable file

pragma solidity 0.7.5;

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

Bank contract

pragma solidity 0.7.5;

import “./Ownable.sol”;
import “./Destroyable.sol”;

contract Bank is Ownable, Destroyable {
}

[ekr990011]

Destroyable.sol:

pragma solidity 0.7.5;

import "Ownable.sol";

contract Destroyable is Ownable {

    function close() internal onlyOwner {
        selfdestruct(owner);
    }
}

Ownable.sol:

pragma solidity 0.7.5;

contract Ownable {
    address payable public owner;

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

Finally I did add a new function to my Bank contract. It just seemed right to have the destroyable contract have its’ function be internal
Bank.sol:

ragma solidity 0.7.5; 

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

contract Bank is Ownable, Destroyable {
// lots of other stuff
    function destroy() public {
        close();
    }
} 

Looking back I didn’t make the destroy function in the Bank contract onlyOwner and that could most likely save some gas for anyone that tried? I just don’t feel like it was safe leaving Destroyable contract’s function of close without internal and onlyOwner modifier.

Just curious what would a normal pattern be?