Inheritance Assignment

[jon_m]

Hi @menarf,

Your Bank contract compiles, your inheritance structure “works” to a certain extent, and after being deployed and various transactions having been performed, Bank can be destroyed and the remaining Ether in the contract is transferred to the caller of selfdestruct.

However, as @kg17 has correctly pointed out …

@kg17

If you leave your code as is, then anyone can selfdestruct your contract as its open to public.

As well as destroying the contract, the selfdestruct method also automatically transfers the remaining contract balance to the payable-address argument it is called with. This is all part of the pre-defined selfdestruct functionality. Your selfdestruct argument is msg.sender, and msg.sender references whichever address has called the close() function. So, at the moment the big problem with your code is that, as well as anyone being able to call close() and destroy the contract, they can also steal the remaining funds!

The way to resolve this is to restrict access to the close() function to a trusted address, and
the assignment proposes that this should be the contract owner…

@kg17

one of the assignment’s requirements is that only the owner of the contract should be able to call the selfdestruct functionality in a situation where such a grave step is needed. For example, if your smart contract is being attacked by hackers and you can see them slowly draining Eth from your contract’s address, then in such a case, you might have to take the drastic step of selfdestructing your contract and withdrawing all the amount from it.

If only the contract owner can call close() and trigger selfdestruct, then by calling selfdestruct with msg.sender, this ensures that any remaining Ether in the contract can only be transferred to the contract owner’s external address.

@kg17 has also told you how to ensure that only the contract owner’s address can call the close() function …

@kg17

Thus the close() function will also need the onlyOwner modifier in its header

The problem here is that close() is in Destroyable and the onlyOwner modifier is defined in its child contract, Ownable. So, the onlyOwner modifier isn’t currently available within Destroyable. For the modifier to be available…

@kg17

You’d want Destroyable to be inheriting from Ownable and thus Bank to be inheriting from Destroyable.

Hopefully, you will now be able to correct your solution and understand the importance of these modifications. Make sure you deploy and spend some time testing your modified solution, so that you can see the effects of these changes in action.

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

[jon_m]

Hey @seybastiens,

Haha … I made that function external to get you to think

We could also give it public visibility, but if we wanted to only allow it to be called from outside the deployed contract and not from within, then we would restrict its accessibility to external visibility.

As @kg17 has correctly said, outside the deployed contract means that …

@kg17

… anything outside the contract can call it …

This effectively means from any external service (a web3 client e.g. Remix or the frontend of a dapp) or from any external smart contract.

I think you may be confusing the contract owner’s address (A, in our example) with the deployed contract’s address. All of the Ethereum addresses in the Account field in Remix (A, B, C…etc.) are external user addresses, which can be used to sign and send transactions to the EVM. In this respect the contract owner’s address is no different to the addresses of other users of our Bank smart contract. Address A is only our contract owner address, because this is the address that is used to initially deploy the smart contract. Due to the way the smart contract has been coded, the deploying address is assigned certain permissions that other user addresses are not.

So, in terms of accessibility based on visibility modifiers, all external addresses are treated equally i.e. they will either (i) all be able to call a function with external or public visibility, or (ii) they will all not be able to call a function with internal or private visibility. The accessibility of specific functions can then be restricted further by adding require statements, either at the beginning of the function body, or within a modifier included in the function header. In this way, even though your SelfDestructContract() function has public visibility, none of the external user addresses can access this function except for the contract owner’s address (A), as a result of the onlyOwner modifier.

I hope this clarifies things. But if not, just let me know and I’ll try to explain it in a different way

[seybastiens]

Oh yes of course, what a dumb confusion I did. I also thought in the same way that private visibility would be only visible to msg.sender…but I confused the deploying address and the contract address.
It’s very clear now thanks.

[JoriDev]

Here’s my solution. Although it looks basic at first, everything worked fine during multiple tests in Remix.

My main obstacles where getting the import line right (at first I forgot .sol) and understanding where you can verify the placing of the remaining Ether after destroying the contract. Which I later on found in the Remix panel itself right under Account behind the owners’ address.

Ownable Contract:

pragma solidity 0.7.5;

contract Ownable {

    address owner;

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

    constructor(){
        owner = msg.sender;
    }

}

Destroyable Contract:

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable{

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

}

Contract header of Bank Contract:

pragma solidity 0.7.5;

import "./Destroyable.sol";

contract BankContract is Destroyable{

[jon_m]

Great solution @JoriDev … and nice self-evaluation!

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

@JoriDev

Although it looks basic at first, everything worked fine during multiple tests in Remix.

The simpler, the better! In my opinion this is the optimum solution.

Good to hear that you carried out extensive testing

@JoriDev

My main obstacles where getting the import line right (at first I forgot .sol)

Yeh … that’s easily forgotten, and is a common mistake.

@JoriDev

… and understanding where you can verify the placing of the remaining Ether after destroying the contract. Which I later on found in the Remix panel itself right under Account behind the owners’ address.

Well done, for working it out! … Yes … the contract’s remaining Ether is now “safe” in the contract owner’s external account … as long as they are a trustworthy custodian!

You’re making great progress!

[menarf]

Thanks. It makes much more sense than my attempt.

Cheers

[menarf]

Thanks a lot for the explanation. I will have a deeper look at this and correct my code.

Cheers

[Baz]

Ownable.sol

pragma solidity 0.7.5;

contract Ownable {

    address public owner;

    modifier onlyOwner {
        require(msg.sender == owner, "You are not owner!!");
        _;
    }

    constructor() {
        owner = msg.sender;
    }

}

Destroyable.sol

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable {
    // THIS ACTION SHOULD ONLY BE AVAILABLE FOR THE CONTRACT OWNER.

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

}

Bank.sol

pragma solidity 0.7.5;

import "./Destroyable.sol";

contract BankContract is Destroyable {
...
}

[pappas_kellyn]

//Ownable contract

pragma solidity 0.7.5;
contract Ownable {
address payable public owner;
modifier onlyOwner (){
require (msg.sender == owner, “must be contract owner”);
_;
}
constructor() {
owner = msg.sender;
}
}

//Destroyable contract

pragma solidity 0.7.5;

import “./ownable.sol”;

contract Destroyable is Ownable {
function destroy() public onlyOwner {
selfdestruct(payable(msg.sender)); //‘payable’ not necessary here, but is for pragma 8.0 and above
}
}

//Bank contract

pragma solidity 0.7.5;

import “./destroyable.sol”;

contract Bank is Destroyable{ …

[martina]

Ownable:

pragma solidity 0.7.5;

contract Ownable {

address owner;

modifier onlyOwner {

  require(msg.sender == owner);

    _;

}

constructor(){

 owner == msg.sender;

}

}

Destroyable:
import “./ownable.sol”;

pragma solidity 0.7.5;

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 {

[stanmation]

pragma solidity 0.7.5;

contract Ownable {
    address payable owner;

    uint remainingBalance;

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

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

pragma solidity 0.7.5;

import "./Ownable.sol";

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

pragma solidity 0.7.5;

import "./Destroyable.sol";

contract Bank is Destroyable {
    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 destroyMe() public {
        remainingBalance = balance[owner];
        close();
    }
}

[nathanls78]

// Here is my Destroyable.sol contract
pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable {

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

}

// Here is my Ownable.sol contract
pragma solidity 0.7.5;

contract Ownable {

    address owner;

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

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

// here is the inheritance syntax on hellowolrd.sol contract (which is the bank contract)
pragma solidity 0.7.5;

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

contract Bank is Ownable, Destroyable {

[gunnarseay]

Destroyable Contract Solution:
Ownable Contract:

pragma solidity 0.5.12;

contract Ownable{
    address public owner;

    modifier onlyOwner {
        require(msg.sender == owner, "Caller needs to be owner"        _; /
        _; //continues exec in function
    }

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

import “./Ownable.sol”;
pragma solidity 0.5.12;
contract Destroyable is Ownable {

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

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

contract HelloWorld is Destroyable{
…

[Heyns]

I didn’t modify anything in my ownable.sol file.

this is my Destroyable.sol file:

pragma solidity 0.7.5;

import "./Ownable.sol";

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

and then I changed the import in my bank.sol file to the destroyable.sol file, because in destroyable.sol the Ownable.sol file is already integrated.

[Heyns]

After looking at the solution I came across a piece of code and therefore I have one question:

govermentInstance.addTransaction(msg.sender, recipient, amount);

Can you please explain to me what this piece of code functionality is in the bank.sol file?

Thanks!

[Heyns]

ok further in the course

[jon_m]

Hi @martina,

You are nearly there, apart from a couple of errors …

Based on the code you’ve posted, your Bank contract won’t compile because of an error in the contract header …

@martina

contract Bank is ownable, Destroyable {

All of the names of your three contracts start with a capital letter, and so you need to ensure you are consistent with this when coding your inheritance structure. These kinds of mistakes are easy to make, but the compiler will flag them for you, and the error messages generated by the compiler will help you to identify and correct them. If you haven’t already, turn on Auto compile by checking the first box under Compiler Configuration in the Solidity Compiler panel in Remix. The compiler will then compile your code as you write it. This will enable you to see and check any errors as soon as they arise.

Apart from this error, your inheritance structure is well coded. However, Bank only needs to explicitly inherit Destroyable, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

---

Once you’ve corrected your code for the above error, you will be able to compile and deploy Bank. However, you will now find that Bank cannot be destroyed…

All of the code in your Destroyable contract is correct. The problem is that, on deployment, the address that deploys Bank isn’t assigned to the owner state variable in Ownable. Can you see why?

At the moment the owner variable remains empty. So, when the contract owner calls destroy() the condition in the onlyOwner modifier’s require() statement is comparing the contract owner’s address (msg.sender) with a zero address (the default value of an empty address variable). This means that the condition will always evaluate to false and the require() statement will always fail and trigger revert.

See if you can correct this yourself. But let me know if you need any further help, or if you have any questions

---

Just one final point …

When posting your code, format it as one single block. That way you won’t end up with bits of your code formatted, and other bits unformatted. This will make it easier to read and easier to spot any copy-and-paste errors.
Follow the instructions in this FAQ: How to post code in the forum

[jon_m]

Hi @stanmation,

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 some code which isn’t necessary. The same objectives can be met in a more concise way …

(1) By giving your close() 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 (destroyMe) to Bank which in turn calls close(). However, an important reason for using inheritance is to reduce code duplication, and if you change the visibility of the close() 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 destroyMe() function in Bank.

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

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) 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 means there is no need to add any additional code for this.

In any case, the additional code you’ve added wouldn’t save the contract’s remaining Ether balance, or transfer it to the contract owner’s external address, anyway …

@stanmation

remainingBalance = balance[owner];

This line of code assigns the contract owner’s individual balance to the remaingBalance state variable in Ownable. But the Ether which needs to be transferred on destruction of the contract is the total amount of Ether held in the contract, and not just the contract owner’s share.

The individual user balances stored in the mapping perform an internal accounting role and record each user’s share of the contract’s total Ether balance. The contract address balance can be accessed with…

address(this).balance

… and can be retrieved at any point in time by adding a getter which contains this code e.g.

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

@stanmation

uint remainingBalance;

Whatever value is assigned within either the destroyMe() or the close() function to the remainingBalance state variable in Ownable will be immediately lost when selfdestruct is then triggered and destroys the contract in the same transaction. So, even if we did need to reference and store an amount during this final transaction (which we don’t), we would only need to do this temporarily using a local variable, and not with a state variable.

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

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

---

A couple of additional observations about the withdraw() function in Bank …

(3) 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 has ended up being left in the withdraw() function in the solution code for this assignment by mistake!

(4) Your withdraw() function now includes the missing return statement, but don’t forget to also add a line of code to adjust the individual user’s balance in the mapping for the withdrawal amount, as I’ve explained in my feedback for your Transfer Assignment.

---

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

[jon_m]

Nice solution @nathanls78

In fact, Bank only needs to explicitly inherit Destroyable, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

Let me know if you have any questions

[mcgrane5]

john you are now free @jon_m