Inheritance Assignment

Denzel · July 10, 2021, 6:34pm

Fixed it!
bank file:

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

contract Bank is Ownable, Destroyable{
    
   mapping(address => uint) balance;

   event depositDone(uint amount, address depositedTo);
    
    struct User{
        uint id;
        uint balance;
    }
   
  
   function deposit() public payable returns (uint){
       balance[msg.sender] += msg.value;
       emit depositDone(msg.value, msg.sender);
       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);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
   }
   
   function withdraw(uint amount) public returns (uint){
       require (balance[msg.sender]>=amount);
       balance[msg.sender]=-amount;
       msg.sender.transfer(amount);
   }
   
   function _transfer(address from, address to, uint amount) private {
       balance[from] -= amount;
       balance[to] += amount;
   }
  
}

ownable file:

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

destroyable file:

import "./Ownable.sol";
contract Destroyable is Ownable{
    
    function destroy() public onlyOwner {
        address payable receiver = msg.sender;
        selfdestruct(receiver);
    }
    
}

jon_m · July 12, 2021, 12:22am

A very well-coded assignment solution @godi

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

jon_m · July 12, 2021, 1:03am

A very well-coded assignment solution @Denzel

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.

I can’t actually find anything in the first version you posted which would cause that problem. I’ve also deployed the Bank contract and all of its functions are available as well as the inherited functionality from Ownable and Destroyable.
Also, the only difference I can see between your two versions is the owner state variable definition in Ownable:

1st version

2nd Version

But state variables have internal visibility by default, and so adding the keyword internal, in fact, makes no difference, anyway.

So what else did you do differently?

Was it simply that you deployed Destroyable instead of Bank the first time. That would result in the destroy() function being the only function available to call…

I’ve also noticed that you have a stray struct in your Bank contract which doesn’t belong there. I think it’s slipped in from the Data Location Assignment code

Also, don’t forget those additional modifications you need to make to your withdraw() function, as explained here.

Just let me know if you have any questions

jon_m · July 12, 2021, 1:12am

I’m glad you find the feedback helpful, Matthew

There’s no rush Do it when you can, take your time, and I’ll check your revised code whenever you post it

Cero · July 13, 2021, 10:33am

Ownable contract:

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

We inherite Ownable function from Ownable.sol file
Destroyable contract:

import "./Ownable.sol";

contract Destroyable is Ownable{
  
    function destroy() public onlyOwner { 
        //selfdestruct(msg.sender);  should include payable function and define receiver
        address payable receiver = msg.sender;
        selfdestruct(receiver);
    }
}

We inherite only destroy function from Destroyable.sol file, since Ownable function from Ownable.sol file is already imported (multy-level)
Bank contract:

pragma solidity 0.7.5;

import "./Destroyable.sol";

contract Bank is Destroyable{

    mapping(address=>uint) balance;
    
    event depositDone(uint amount, address depositedTo);
    
    function deposit() public payable returns (uint){
        balance[msg.sender]+=msg.value; //no need to track anymore
        
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdrawal(uint amount) public onlyOwner returns (uint){
        require(balance[msg.sender]>=amount);
        msg.sender.transfer(amount);
        balance[msg.sender]-=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);
    
        assert(balance[msg.sender]==previousSenderBalance-amount);
    }
    
    function _transfer(address from, address to, uint amount) private{
        balance[from]-=amount;
        balance[to]+=amount;
    }

}

jon_m · July 12, 2021, 3:28pm

Hi @ImmMeta,

Apologies for the delay in giving you feedback on this assignment.

On the whole, this is a good solution which works

That’s because your require statement is always passing (evaluating to true) because you have used an assignment operator (=) in the condition instead of an equality operator (==) …

However, I don’t think adding the additional password validation improves security at all. The password is hard-coded into the smart contract, and smart contracts deployed on the blockchain are publicly available to view. To call the implode function, the contract owner first needs to sign the transaction with their private key. In blockchain technology this replaces the need for passwords.

You can also remove the return statement from the implode() function, because it’s unreachable. If selfdestruct() executes successfully, the contract will be destroyed and removed from the blockchain, so any code positioned after selfdestruct() within the implode() function body will never be executed.

So, in summary, your implode() function will provide just as good a solution as follows:
(In fact, it provides a better solution, because it’s more concise and does not contain any redundant code. Unnecessary code just provides more opportunities for bugs or vulnerabilities to appear)

function implode() public reqAdmin {
    selfdestruct(payable(msg.sender));
}

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 final observation…

You do not need the parentheses in the following two statements:

These can be written as follows:

bool pass;

Solidity does not have an equivalent to the data type undefined. All unassigned variables automatically evaluate to their equivalent zero values:

uint/int 0
string ""
bool false
address 0x00..000

return "self destruct complete";

We only need to enclose return values in parentheses when returning more than one value.

Let me know if you have any questions

jon_m · July 12, 2021, 4:43pm

You could do — in fact, for reasons I’m sure you’ve already read about in the article you’ve linked to, and during your wider research, using the pausable pattern has many advantages over the more “primitive” selfdestruct(). Using selfdestruct() brings with it certain clear disadvantages, the potential repercussions of which need to be fully understood before choosing to implement it; but 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. 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 201 course.

jon_m · July 12, 2021, 10:49pm

Hi @decrypter,

OK, I understand what you mean now

What I still don’t understand, though, is why you are concerned about whether the close() function in Destroyable can still be called if it is overridden in Bank.

As a result of the onlyOwner modifier, the close() function in Destroyable can only ever be called by the contract owner’s address, and so it is already secure (at least in as much as the contract owner can be trusted as custodian of the contract’s remaining funds). If we didn’t want selfdestruct() to be triggered (even by the contract owner), then all we would need to do is just not include Destroyable within our inheritance structure.

And I still come back to my original point that, if you give close() internal visibility, then you still overcomplicate the situation by creating a need for an additional public or external function to be added to enable the contract owner’s external address to access the selfdestruct() functionality. This issue would still be present if close() was overriden in Bank, because if close() has internal visibility in Destroyable, then the overriding close() function in Bank would also have to be internal. This is because only external visibility of an overridden function can be modified to public visibility in the overriding function; all other visibility types must remain unchanged.

However, to answer your specific question…

Overridden f() in the base contract (A) cannot be called directly from either an external contract or non-contract (i.e. wallet) address. However, you could add another external or public function which could call it indirectly, as follows:

// Destroyable

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

// Excerpt from Bank, which inherits Destroyable

// Call the overriding function in Bank directly, to destroy the Bank contract
function close() public override onlyOwner {
    selfdestruct(owner);  
}

// Call this function, which calls the overridden function in Destroyable
// to destroy the Bank contract
function accessOverriddenFx() external {
    Destroyable.close();
}

If f() in the base contract (A) has internal visibility (which I think is what you were suggesting as a preventative measure) and is overridden in the derived contract (B), then overriding f() in B can also only have internal visibility. Neither of the two f() functions can be called directly from either an external contract or non-contract (i.e. wallet) address. However, you could implement an external or public function which could call either overriding f() in B, or overridden f() in A, as follows:

// The same Destroyable contract for both alternatives

contract Destroyable is Ownable {
    function close() internal virtual onlyOwner {
        selfdestruct(owner);
    }
}

// Excerpt from Bank, which inherits Destroyable - alternative 1
// Executing the overriding function in Bank to destroy the Bank contract

function close() internal override onlyOwner {   // overriding function
    selfdestruct(owner);                         // executed
}

function accessOverridingFx() external {         // call this function
    close();
}

// Excerpt from Bank, which inherits Destroyable - alternative 2
// Executing the overridden function in Destroyable to destroy the Bank contract

function close() internal override onlyOwner {   // overriding function
    selfdestruct(owner);                         // not executed
}

function accessOverriddenFx() external {         // call this function
    Destroyable.close();                         
}

Below are the links to the official Solidity documentation in terms of Inheritance and Function Overriding. You may already have consulted this, but I just want to include it here for completeness, as this is the documentation I have checked to draw my conclusions on the scenario you have asked about.

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

Let me know if you have any further questions

jon_m · July 12, 2021, 10:06pm

This is a very good solution to the assignment @decrypter

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.

See my comments in the other reply I’ve sent you in terms of your question about function overriding and making close() internal.

A couple of other observations…

Your transfer function will revert every time it is called, because your assert statement is still suffering from the same copy-and-paste slip.
After what I explained here, are you still not convinced that the first two statements in your withdraw() function should be in the opposite order?

Just let me know if you have any questions

Gos · July 12, 2021, 11:40pm

In my Code i used “Selfdestruct(msg.sender)” because “Selfdestruct(owner)” was not working. Can you please explain why “(owner)” is not working even if owner is specified in Ownable.sol?

My code:

pragma solidity 0.7.5;

contract Ownable {
    
    address public owner;
    
    modifier onlyOwner {
              require(msg.sender == owner, "Must be contract owner");
              _; //continue execution
      }
    constructor(){
        owner = msg.sender;
    
    }
    
}

||
||
/

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Suiside is Ownable{
    
    function destroyContract () public onlyOwner{
        selfdestruct (msg.sender);
    }

}

||
||
/

pragma solidity 0.7.5;

import "./Selfdestruct.sol";

contract Bank is Suiside {
    
mapping(address => uint) balance;

jon_m · July 13, 2021, 1:08am

Great work @Cero

If it wasn’t for the missing _; in your onlyOwner modifier in Ownable, your contracts would compile and the inherited selfdestruct() functionality would work as it should.

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined.

You’ve definitely got the right idea, and this is a really good attempt at an explanation of why multi-level inheritance is the most efficient inheritance structure to implement here. Just have a look at the modifications I’ve made to your explanation to improve the accuracy…

Note that…

Bank still inherits Ownable, but it does so via Destroyable, so that’s why we don’t need to state the inheritance relationship with Ownable in the Bank contract header.
All of the functionality from Ownable is inherited (constructor, owner state variable, and onlyOwner modifier) not just a function.

Well done for realising that the address argument passed to selfdestruct() needs to be a payable address, in order for the contract’s remaining ether balance to be transferred to that address when the contract is destroyed. To receive ether an address must be payable. However, as well as using this code …

… in Solidity v0.7 it is also correct to just use the much more concise alternative that you have commented out…

selfdestruct(msg.sender);

This is because prior to Solidity v0.8, msg.sender is payable by default. This means that whenever you want to use msg.sender as a payable address, you don’t have to explicitly convert it. However, from v.0.8, msg.sender is non-payable by default, and so whenever you are using v.0.8 and need to use msg.sender as a payable address, this requires an explicit conversion e.g.

selfdestruct(payable(msg.sender));

Just a couple of other comments…

(1)

Maybe you didn’t mean to include this comment, but just to confirm, we do still need this statement in the deposit function. It’s essential to adjust the user’s individual account balance in the mapping for any deposits or withdrawals, either increasing or decreasing it by the same amount as any ether the user transfers into or out of the contract.

(2) You’ve modified the withdraw function with the onlyOwner modifier, but this means that 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? Your withdrawal() 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 various 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 you have any questions

Cero · July 13, 2021, 10:59am

Hello Jon,
I appreciate your comments and corrections.
missing _; was by mistake when deleting comments etc.
I checked your comments and have a question. In the last paragraph you mentioned that i left onlyOwner modifier in the contract helloworld. Here we call modifier onlyOwner from Ownable contract. In case I remove this from helloworld I do not even need ownable contract. I understand the point that everyone should be able to withdrawal their own deposited amount. In which case we need onlyOwner modifier here in withdrawal function?

jon_m · July 13, 2021, 3:15pm

Hey Matej,

You do need it, because the onlyOwner modifier needs to be inherited by Destroyable in order to restrict access to the destroy() function to the contract owner address. HelloWorld needs to inherit the selfdestruct() functionality and so needs to inherit a destroy() function which applies the onlyOwner restriction, so that only the contract owner can destroy HelloWorld…
… I hope that makes sense

decrypter · July 13, 2021, 3:23pm

Hi Jon,

What I still don’t understand, though, is why you are concerned about whether the close() function in Destroyable can still be called if it is overridden in Bank.

The reason is, that the close() function in Bank or another contract inheriting from Destroyable may, and probably will, do much more than the close() function in Destroyable. That’s why it’s overridden in the first place.

An example may be to reimburse the clients of the bank with any remaining balance in the Bank contract. If I can call Destroyable.close() from outside of Bank I could bypass any code implemented in Bank::close()

Now, you are right that only the owner can call the close() function and we can assume that the owner is not malicious. Nonetheless, even honest people make mistakes and the idea with the internal visibility was to prevent that mistake from being possible in the first place.

This may not be a perfect example but there will be many other cases where I have functions that may be called by non-owners as well. In this case the possibility of calling the function of the base class may present an attack vector.

Now I understand that the derived contract close() function must also be internal. In that case I would not override it at all, rather implement a new function. Indeed something like:

function accessOverriddenFx() external {

        // Do some Bank specific pre-close clean up

        Destroyable.close();

This brings it back to my original point. If Destroyable.close() was public this opens the possibility of closing the bank without the cleanup implemented in accessOverriddenFx().

Makes sense?

decrypter · July 13, 2021, 4:09pm

Yes, this should fix it.

function transfer   (address recipient, uint amount) enoughBalance(amount) public {
    //require(balance[msg.sender]>= amount, "Not enough funds");
    require(msg.sender!= recipient, "Dont transfer to yourself");
    
    uint previousSenderBalance=balance[msg.sender];
    
    balance[msg.sender]-=amount;
    balance[recipient]+=amount;
    assert (balance[msg.sender]==previousSenderBalance-amount);
}

sohamdey_2006 · July 13, 2021, 5:56pm

pragma solidity 0.7.5;

contract Ownable{
address payable public owner;

constructor(){
    owner = msg.sender;
}

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

}

pragma solidity 0.7.5;

import “./Ownable.sol”;

contract Destroyable is Ownable{

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

}

contract Bank is Destroyable{
//code for Bank contract
}

sohamdey_2006 · July 13, 2021, 6:00pm

Hi @filip,

As per your solution in the Ownable contract, why is the address internal ? What if I make the address as ‘payable public’ in the Ownable contract and use that variable directly in the Destroyable contract without any change ( code snippet below)?

contract Ownable{
address payable public owner;

constructor(){
    owner = msg.sender;
}

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

}

contract Destroyable is Ownable{

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

}

jon_m · July 13, 2021, 9:36pm

Hi @decrypter,

OK, I understand what you are trying to prevent happening now. But it can’t happen whether close() in Destroyable is internal or public…

If you choose to override close() in Destroyable, then all you have to do is either (i) not include the function-call Destroyable.close() anywhere in Bank, or (ii) include it only where you want it to be called. You, as the developer of the smart contract, have control over this. Even if close() in Destroyable has public visibility, it can’t just be called directly from outside Bank. You can see that demonstrated in Remix when you deploy Bank: if close() is overridden in Bank, then only the overriding function will be available to call externally (if it’s public or external). In order to call Destroyable.close() you would have to have placed that function call within a public or external function as you have here…

Because Destroyable.close() can only be called directly from within the deployed Bank contract, it makes no difference whether close() in Destroyable has public or internal visibility.

So, no… this won’t be possible if you don’t specifically add an additional function which calls Destroyable.close() without the addtional cleanup.

Also, remember that Destroyable isn’t being deployed as a separate contract. We are only deploying Bank; and as Bank inherits Destroyable, and Destroyable inherits Ownable, all of the inherited functionality is available within Bank as if it was in Bank in the first place. The exception being, of course, if a public or internal function in either of the base contracts is overridden. In this case the overridden function would only be available as implemented in the base contract, if it is called on an instance of the base contract from within Bank, and from a specific location within Bank of your choosing.

jon_m · July 13, 2021, 9:59pm

An excellent solution @Gos

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.

The important thing to understand, here, is that the address argument passed to selfdestruct() needs to be a payable address, in order for the contract’s remaining ether balance to be transferred to that address when the contract is destroyed. To receive ether an address must be payable. There are several ways we can code this…

(1) Use msg.sender (as you have done) to reference the contract owner’s address, because this is the only address that it can ever represent in the destroyContract() function (as a result of the onlyOwner modifier restricting who can call this function in the first place). Prior to Solidity v.0.8, msg.sender is payable by default, and so that’s why you haven’t needed to explicitly convert it in your solution (this course is based on v0.7.5).
However, from v.0.8, msg.sender is non-payable by default, and so whenever you are using v.0.8 and need to use msg.sender as a payable address, this requires an explicit conversion e.g.

selfdestruct(payable(msg.sender));

(2) Use owner (like you wanted to) to reference the contract owner’s address. To be able to do this, you also need to do either of the following:

Declare the owner state variable in Ownable with a payable address type, instead of with the default, non-payable address type;
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 destroyContract() public onlyOwner {
          selfdestruct(payable(owner));
      }

Just let me know if you have any further questions

jon_m · July 13, 2021, 10:39pm

Nice solution @sohamdey_2006

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.

The only thing you’re missing is an import statement in your Bank.sol file to import the file with your Destroyable contract.

Your solution is perfectly correct. There are several alternatives. Whether the owner state variable in Ownable is declared with public or internal visibility is purely down to whether the developer wants this variable to be accessed externally as well as internally, or only internally. In fact, state variables have internal visibility by default (i.e. when no visibility is explicitly declared), and so there is actually no need for the solution code to include the keyword internal in the definition of the owner state variable.

The important thing to understand about the address argument passed to selfdestruct() is that it needs to be a payable address, so that the contract’s remaining ether balance can be transferred to that address when the contract is destroyed. To receive ether an address must be payable. Some of the alternative ways to code this are …

(1) Use owner (like you have) to reference the contract owner’s address. To be able to do this, you also need to do either of the following:

Declare the owner state variable in Ownable with a payable address type (like you have). The solution code leaves it with the default, non-payable address type, because it passes msg.sender to selfdestruct() instead;
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));
      }

(2) Use msg.sender (as the solution code does) to reference the contract owner’s address, because this is the only address that it can ever represent in the destroy() function (as a result of the onlyOwner modifier restricting who can call this function in the first place). Prior to Solidity v.0.8, msg.sender is payable by default, and so that’s why it doesn’t need to be explicitly converted (this course is based on v0.7.5).
However, from v.0.8, msg.sender is non-payable by default, and so whenever we use v.0.8 and need to use msg.sender as a payable address, this requires an explicit conversion e.g.

selfdestruct(payable(msg.sender));

Let me know if you have any further questions