Inheritance Assignment

[jon_m]

Nice solution @cgironda

@cgironda

I did not change the ownable.sol original file, but I am sure something must be changed, otherwise, the instructions to solve this problem wouldn’t have mention it.

Not necessarily… Whether you need to make any changes to the Ownable contract you already had, depends on how you code Destroyable. The way you’ve coded Destroyable doesn’t require any changes to Ownable
If you have a look at some of the other students’ solutions which are different to yours, and the comments and feedback they’ve received, you’ll see how if the owner address variable is passed to selfdestruct() instead of msg.sender, then this could require Ownable to be modified… but, again, not necessarily.

The only code you’re missing is the import statements in bank.sol, importing ownable.sol and destroyable.sol.

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]

A good solution @Bitborn

@Bitborn

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

You’ve passed msg.sender directly to selfdestruct() which is perfectly fine. But, this then makes the line of code above completely redundant. The compiler should have generated an orange/yellow warning for this, pointing out this unused variable receiver. As you have assigned msg.sender to receiver, and receiver is declared with a payable-address type, you could use this variable by passing it to selfdestruct(), instead of removing it…

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

This is, however, unnecessary.

Be careful with your curly brackets…your Destroyable contract won’t compile because you’re missing the one which should close the contract body!

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.

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

[jon_m]

An excellent assignment solution @Nathan_Burkiewicz

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 paid/transferred to the owner when selfdestruct() is triggered.

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

[Ruzz]

This is the solutions I have

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.7.5;

import “./Ownable.sol”;

contract Destructible is Ownable {

function kill(address payable to) public onlyOwner {
selfdestruct(to);
}

function killer() public payable onlyOwner {
require(msg.sender == owner);
selfdestruct(msg.sender);
}

}

The kill function above I added originally as I thought it may be good to be able to send the funds from funds to a wallet of the owners choosing? Is there any security risk present in doing this? The only thing I could think of is if a wallet keys were compromised and the attacker sent contract funds to their wallet but then they could send to the owner and and transfer out with the other function as well. Just interesting to think about

[jon_m]

A very well-coded assignment solution @Julian97

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]

Hi @Filip_Rucka,

You’re nearly there with this … but first there are some important issues you need to fix …

(1) If you pass owner to selfdestruct() then you are right to declare it as a state variable with a payable address-type. However, it is the owner variable in Ownable which is assigned the contract owner’s address when Bank is deployed, and not the other owner variable you’ve declared in Destroyable. If you add a getter to Destroyable to return owner, you will see that it holds a zero address and not the contract owner’s address. This means that when selfdestruct is triggered and the contract is destroyed, the remaining ether balance will be transferred to the zero address and lost, and not to the contract owner’s address. You can remove this additional owner variable from Destroyable, because it is redundant.

(2) You’ve given the owner variable in Ownable, which stores the contract owner’s address, private visibility. This means that it isn’t available in Destroyable. If you give it internal or public visibility, instead, it will be inherited by Destroyable and you’ll be able to pass it to selfDestruct() as a payable address, so that the remaining ether balance held in the contract address will be transferred to the contract owner’s external wallet address when the contract is destroyed.

(3) The onlyOwner modifier declared in Ownable is inherited by Destroyable, but you’re not using it to restrict access to the close() function to the contract owner, so that only the contract owner can trigger selfdestruct() and destroy the contract. You’ll see that, at the moment, any address can destroy the contract!

Can we also see what modifications you’ve made to the start of your Bank.sol file, and Bank contract header, for the inheritance? After all, Bank is the derived contract that we are actually deploying.

Let me know if you need any help to make these modifications, or if you have any questions

[jak]

You are a beast.
I’m going to go back over the content this weekend and I’ll reference this along the way.

Thank you so much for all the crazy helpful feedback you provide.

[MattPerry]

absolutely.

bank.sol:

import "./Ownable.sol";

interface GovernmentInterface {
    function addTransaction(address _from, address _to, uint _amount) external;
}

contract Bank is Ownable {
    
    GovernmentInterface GovernmentInstance = GovernmenInterface(0xd9145CCE52D386f254917e481eB44e9943F39138);
    
    mapping(address => uint) balance;
    
   
    
    event depositDone(uint amount, address indexed depositedTo);
    event transferEvent(uint amount, address toAddress, address fromAddress);
    
    
    
    
    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 returns(uint) {
       
        require(balance[msg.sender] >= amount, "insufficient funds!");
        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);
        require(msg.sender != recipient);
        
        uint previousSenderBalance =  balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);

        GovernmentInstance.addTransaction(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
        
        //event logs and further checks
        emit transferEvent(amount, recipient, msg.sender);
    }
 
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}

ownable.sol:

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

[ImmMeta]

Hey all,
EthBank header:

//SPDX-License-Identifier: UNLICENSED //so visual studio stops yelling at me.
pragma solidity >= 0.8.4;
import "./metaOwner.sol";
import "./metaSelfDestruct.sol";
contract metaEthBank is metaOwner, metaSelfDestruct{

admin priv contract

//SPDX-License-Identifier: UNLICENSED
pragma solidity >= 0.8.4;

contract metaOwner {

constructor(){//upon deployment tells contract that msg.sender is owner;
    owner = msg.sender;
}

address owner;//tells contract that owner is an address variable;

modifier reqAdmin{ //modifier req admin priv to execute function;
    require(msg.sender == owner);
    _; //run the function.
}

}

self destruct function:

// SPDX-License-Identifier: UNLICENSED
pragma solidity >= 0.8.4;
import "./metaOwner.sol";
contract metaSelfDestruct is metaOwner{

function implode(uint input) public reqAdmin returns(string memory){
    (bool pass)= false;
        if(input == 42069){
            pass = true;
        }

    require(pass = true, "Incorrect Password");
selfdestruct(payable(msg.sender));
return("self destruct complete");
}
//why not use pausable instead? https://blog.b9lab.com/selfdestruct-is-a-bug-9c312d1bb2a5

} 

The only bug I noticed in my testing is that the self destruct function will fire regardless of the input value as long as msg.sender==owner. Any insight?
thanks.

[Julian97]

Thanks! And thank you for making me aware of this!

[Carlo]

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

    constructor() {
        owner = msg.sender;
    }
}

// Destroyable.sol
contract Destroyable is Ownable {

    function destroy() public onlyOwner {
        msg.sender.transfer(address(this).balance);
        selfdestruct(owner);
    }
}


// Bank.sol
import "./Destroyable.sol";

contract Bank is Destroyable {
...
}

[jon_m]

Hi @Ruzz,

@Ruzz

The kill function above I added originally as I thought it may be good to be able to send the funds from funds to a wallet of the owners choosing? Is there any security risk present in doing this?

This is fine, because access to the kill() function, and therefore selfdestruct(), is still restricted to the contract owner — the address we are saying we trust to be the custodian of the remaining funds.

@Ruzz

The only thing I could think of is if a wallet keys were compromised and the attacker sent contract funds to their wallet

If the contract owner’s private keys are compromised, and an attacker is able to call the kill() function, they will be able to obtain the contract’s funds whether or not the kill() function has an address parameter enabling them to choose the address the funds are transferred to. This is because the alternative to choosing the receiving address is for the remaining funds to be sent to the caller’s address (which can only be the contract owner’s), and if this address is compromised, then we can only assume that the attacker would have access to any funds sent to it. But isn’t this the conclusion you’ve also come to? …

@Ruzz

but then they could send to the owner and and transfer out with the other function as well.

---

@Ruzz

function killer() public payable onlyOwner {
require(msg.sender == owner);
selfdestruct(msg.sender);
}

In this alternative function, you don’t need the require() statement, because this is
already applied by the onlyOwner modifier inherited from Ownable.

Also, the killer() function 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.

---

Can we also see what modifications you’ve made to the start of your Bank.sol file, and Bank contract header, for the inheritance? After all, Bank is the derived contract that we are actually deploying.

Just let me know if you have any questions

[jon_m]

Hi Matthew,

If your Bank contract is deployed, the owner will not be able to destroy the contract and transfer the remaining funds to their external wallet address. You need to change your inheritance structure.

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

Let me know if you have any questions

[jon_m]

Hi @Carlo,

Your solution works And 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.

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.
However, as selfdestruct() performs the transfer of remaining funds to its payable address agument (as well as destroying the contract), there is no need to also include the extra line of code above…

@Carlo

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

This code does transfer the contract balance to the contract owner, but it isn’t needed.

Your Destroyable.sol code is also missing an import statement, but I imagine you have included this in your own code, because otherwise it wouldn’t compile.

Let me know if you have any questions

[godi]

Ownable
pragma solidity 0.7.5;

contract Ownable {

address public owner;

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

constructor(){
    owner = msg.sender;
}

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

contract Destroyable is Ownable{

function destroy() public onlyOwner{
    selfdestruct(msg.sender);
}

}

Bank
pragma solidity 0.7.5;

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

contract Bank is Ownable,Destroyable

[decrypter]

pragma solidity 0.7.5;

contract Ownable{

address payable internal owner;

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

constructor(){
    owner=msg.sender;
}

}

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

contract Destroyable is Ownable {

function close() public onlyOwner { //Internal to allow calls only from derived contracts
    selfdestruct(owner);  
}

}

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

contract Bank is Destroyable {

mapping(address => uint) balance;

event fundsDeposited(uint amount, address indexed depositedTo);

modifier enoughBalance (uint amount) {
    require(balance[msg.sender]>= amount, "Not enough funds");
    _; //Run the function
}

function deposit() public payable returns(uint) {
    balance[msg.sender]+= msg.value;
    emit fundsDeposited(msg.value, msg.sender);
    return balance[msg.sender];
}

function  withdraw (uint amount) enoughBalance(amount) public returns(uint){
     msg.sender.transfer(amount);
     balance[msg.sender]-=amount;
     return balance[msg.sender];
}

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[recipient]+=amount;
    balance[msg.sender]-=amount;
    assert (balance[msg.sender]==previousSenderBalance);
}

function getBalance () view public returns(uint){
     return balance[msg.sender];
}

}

[decrypter]

Hi Jon,

My terminology is indeed somewhat rusty.

What I meant was not overloading but _ overriding _

Now let me rephrase my question:

• Contract B inherits contract A
• Contract A has a public function f()
• f() is overridden in contract B

My understanding is that there is no way to call A::f() in Solidity, correct?

[MattPerry]

Gonna give it another try, thanks Jon. You have been helping me a lot. I really appreciate the feedback and the tips.

[MattPerry]

i should have these done soon, had to wait till the weekend to make the last couple corrections you gave me.

[Denzel]

I am having an issue where I cannot do anything but destroy the contract, not sure what I did wrong.
Main bank file:

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

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 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);
    }
    
}