Jon, it is actually not quite clear to me how to apply the onlyOwner modifier to the destroy_contract(). I am sorry but it is still not quite clear. Could you give me a bit more help, please?
Thanks in advance!
Hi @raphbaph,
Apologies for the delay in replying to your questions about this assignment.
Firstly, your assignment solution is 100% correct and well-coded 
Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyer, because it will implicitly inherit Ownable via Destroyer.
This doesn’t make sense, because access to your killswitch function is restricted to the contract owner by the onlyOwner modifier. I have made my own Bank contract a derived contract of your Destoyer contract, and when I deploy my Bank contract, only the address that deployed it (the contract owner) can call killswitch() and trigger selfdestruct() without killswitch() reverting.
The contract self-destructs when killswitch() successfully executes (as confirmed by the green tick next to the transaction receipt in the Remix console). You can confirm that this has happened by first performing some transactions before destroying the contract. Call getBalance() for one of the users, to prove to yourself that ether is stored in your smart contract as a result of those transactions. Then call killswitch() with the contract owner address. You will notice that one of the disadvantages of selfdestruct() is that successful calls can still be made to the destroyed contract’s functions! This is a serious problem if a user isn’t informed that the contract has been destroyed and unwittingly sends ether to the contract, because it will be lost. But you can call getBalance again for the same user, and if it returns zero (where previously it had returned a positive ether balance) then this is proof that the contract has been destroyed.
When selfdestruct() is called, in order for it to transfer the contract’s remaining ether balance to its address parameter, this address needs to be a payable address (so it can receive ether). In your posted solution, selfdestruct() is called with msg.sender, which is already a payable address by default (before Solidity v.0.8). However, owner is defined as a non-payable address in Ownable, and so if you pass owner instead of msg.sender to selfdestruct(), it will need to be either defined as a payable address in Ownable, or explicity converted to a payable address before or at the same time as being passed to selfdestruct().
In your solution, killswitch() can only be called by the contract owner’s external wallet address, as a result of the onlyOwner modifier restricting access to this function. The error message you have posted is a default one. It is unhelpful in determing the reason for the transaction reverting, because it is generated even when this has nothing to do with sending a value to a non-payable function, or sending a value that is greater than the sender’s balance. This default message will also be generated whenever an address other than the contract owner’s address tries to call killswitch().
Let me know if anything is still unclear, or if you have any further questions 
1 Like
Here is my proposed solution to the Inheritance Assignment.
pragma solidity 0.8.1;
import "./Ownable.sol";
contract banking is Ownable
{
mapping(address => uint) balance;
event depositAdded(uint amount, address indexed depositedToo);
event amountTransfered(address indexed sentFrom, uint amountSent, address sentToo);
function getContractsBalance()public view returns(uint)
{
return address(this).balance;
}
function deposit() public payable returns(uint)
{
balance[msg.sender] += msg.value;
emit depositAdded(msg.value, msg.sender);
return balance[msg.sender];
}
function withdraw(uint amount) public returns(uint)
{
require(balance[msg.sender] >= amount, "balance insufficient.");
address payable userAddress = payable(msg.sender);
balance[msg.sender] -= amount;
userAddress.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, "balance insufficient.");
require(msg.sender != recipient, "cannot money to yourself");
uint previousSenderBalance = balance[msg.sender];
_transfer(msg.sender, recipient, amount);
emit amountTransfered(msg.sender, amount, recipient);
assert(balance[msg.sender] == previousSenderBalance - amount);
}
function _transfer(address from, address too, uint amount) private
{
balance[from] -= amount;
balance[too] += amount;
}
function closeBank() public payable onlyOwner
{
address payable addr = payable(address(stash));
selfdestruct(addr);
}
}
and here is the inherited…
contract Ownable
{
address owner;
address stash = 0xdD870fA1b7C4700F2BD7f44238821C26f7392148;
uint contractValue;
constructor ()
{
owner = msg.sender;
}
modifier onlyOwner
{
require(msg.sender == owner);
_;
}
}
1 Like
Hi @Acex13,
Thanks for alerting us to this issue. It’s seems as though the website hosting the article has shut down, or something similar has happened. We’ve now found a suitable replacement article, and modified the assignment instructions accordingly. This update should be available for you to view on the Academy website within the next 48 hours.
Apologies for the inconvenience caused by this.
1 Like
Hi @ildevirdee,
Thanks for alerting us to this issue. It’s seems as though the website hosting the article has shut down, or something similar has happened. We’ve now found a suitable replacement article, and modified the assignment instructions accordingly. This update should be available for you to view on the Academy website within the next 48 hours.
Apologies for the inconvenience caused by this.
Hi @Klem,
Take your time to rest and recover… there’s no rush!
Anyway, here’s some further feedback on what you’ve posted… for whenever you’re ready…
Your Ownable contract is correct and you have defined the owner as a payable address. This means that calling selfdestruct() with owner in Destroyable is also correct
Your Destroyable contract isn’t inherited.
Where exactly are your Ownable and Destroyable contracts? From what you’ve posted, Ownable seems to be below Bank in the same file, and Destroyable seems to be below the Government contract… but that can’t be right… If that’s the case, Destroyable won’t inherit Ownable, and Bank won’t inherit Ownable (if the base and derived contract are both in the same file, the base contract must precede the derived contract). I think it will be much clearer and more appropriate if you put Ownable and Destroyable in their own separate files, and add import statements to import each of them into their respective derived contracts.
You also have a couple of issues with your code relating to the Government contract and the external function call (in the transfer function in Bank) to addTransaction(). These issues will cause the external function call to fail, and transfer() to revert. However, as the Government contract and this external function call are not part of this Inheritance Assignment, I won’t comment on these issues here. I’m also not sure whether you’re still working on them. If you need help with this next part (which comes after this assignment), then post your questions and code in the discussion topic for this section here.
Ask away, whenever you’re ready
1 Like
Thanks for the detailed feedback. I have figured it out in more detail now.
I expected the contract to close and disappear in Remix. Similar to what it does when I press the trashcan icon.
So this is very good to know, that actually the user can still call the contract at cost, but nothing happens.
I guess that means I should send an event in the case of a selfdestruct().
1 Like
*The link is not available.
*My solution:
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.7.5;
import “./Ownable.sol”;
import “./SelfDestruct.sol”;
contract Bank is Ownable, SelfDestruct {
(…)
}
contract SelfDestruct is Ownable{
function destroySmartContract(address payable _to) public onlyOwner {
selfdestruct(_to);
//all remaining funds on the address of the Smart Contract are transferred to _to.
}
}
1 Like
Great job @loso
… especially as the link no longer worked, and you have completed this assignment based on your own research 
Apologies for the delay in giving you feedback on this assignment, and also for the inconvenience caused by the link no longer working. It appears as though the website hosting the article has shut down, or something similar has happened. We’ve now found a suitable replacement article, and modified the assignment instructions accordingly. The assignment itself is still the same, but if you would like to read the new article and more comprehensive instructions, this update is now available for you to view on the Academy website.
Feedback on your solution
Well done for realising that the address passed to selfdestruct() needs to be payable, in order for any remaining ether in the contract to be paid/transferred to this address when selfdestruct() is triggered. Your method, calling destroySmartContract() with a payable address parameter, is perfectly correct. An appropriate address to send these remaining funds to would be the contract owner address. You can do this by passing either msg.sender, or owner (inherited from Ownable), to selfdestruct(), which also means destroySmartContract() no longer needs an address parameter.
You can use msg.sender because, as a result of the onlyOwner modifier, it can only ever be the contract owner address. Also, msg.sender is already a payable address by default (only before Solidity v0.8) and so doesn’t need to be explicitly converted.
However, if you pass owner you would need to:
- either define it as a payable address state variable in Ownable;
- or convert it locally within the destroySmartContract() function body, either using a local variable before it is passed to selfdestruct(), or convert the actual argument e.g.
selfdestruct(payable(owner));
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.
@Anvar_Xadja is also right about this…
Let me know if you have any questions about any of these points
Nice solution @Anvar_Xadja
Apologies for the delay in giving you feedback on this assignment, and also for the inconvenience caused by the link no longer working. It appears as though the website hosting the article has shut down, or something similar has happened. We’ve now found a suitable replacement article, and modified the assignment instructions accordingly. The assignment itself is still the same, but if you would like to read the new article and more comprehensive instructions, this update is now available for you to view on the Academy website.
Feedback on your solution
Well done for realising that the address passed to selfdestruct() needs to be payable , in order for any remaining ether in the contract to be transferred to this address when selfdestruct() is triggered. Your method, calling destroyContract() with a payable address parameter, is perfectly correct, and provides the flexibilty for the owner to transfer to any address of their choosing
If we knew in advance that any remaining funds should only be transferred to the contract owner’s address (which would be an appropriate and secure restriction), then alternative solutions would be to pass either msg.sender , or owner (inherited from Ownable), to selfdestruct(), which also means destroyContract() no longer needs an address parameter. I’ve gone into further detail about how to implement these alternatives in my feedback to @loso (which I’ve also tagged you in).
If the emit statement is placed after the call to selfdestruct(), the event won’t be emitted because the contract has been destroyed once selfdestruct() has executed, and so the emit statement is never reached. You could fix this by placing the emit statement before the selfdestruct() function call, but I don’t think this is wise, because we would be emitting the event before the actual event (contract destruction) has ocurred. On the other hand, if selfdestruct() failed, then the function would revert, including the emitted event, but I would still be reluctant to place the emit statement first. This needs some further investigation. What are your initial thoughts on the position of the emit statement?
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 have any questions
Hi Joseph,
Apologies for the late response!
Depending on the inheritance structure you’re using, the onlyOwner modifier is either already defined within your Destroyable contract, or is inherited by Destroyable from Ownable. All modifiers in a base contract are inherited by the derived contract.
Available modifiers are applied to functions by referencing the modifier’s identifier (name) in the function header, as follows:
function destroy_contract() public onlyOwner {
selfdestruct(owner);
}
It’s as simple as that 
Just let me know if anything is still unclear, or if you have any further questions.
Hi @ildevirdee,
Just to let you know, that the new article and updated assignment instructions are now available for you to view on the Academy website.
Once again, apologies for the inconvenience caused by this.
Hi Douglas,
Just to let you know, that the new article and updated assignment instructions are now available for you to view on the Academy website.
Once again, apologies for the inconvenience caused by this.
Hi @raphbaph,
Good to hear
You raise an interesting point. I’ve been discussing the issues related to emitting an event for selfdestruct() with @Anvar_Xadja …
Apart from the above points, it’s also worth bearing in mind that emitting an event doesn’t necessarily mean that users will see that infomation, and so doesn’t actually prevent them from subsequently calling the destroyed contract’s functions, incurring a gas cost, and losing any ether sent. There would still need to be some form of communication to all users informing them of the contract’s destruction, and what the consequences and implications of this are in terms of what they must and mustn’t do from now on. This is also why, in practice, other types of security mechanisms, such as proxy contracts, are implemented to deal with these kinds of emergency situations. You will learn more about the practicalities of this if you go on to take the Smart Contract Security course.
Let me know if you have any questions
Ownable Contract
contract Ownable {
address owner;
modifier onlyOwner {
require(msg.sender == owner);
_; //run the function
}
constructor(){
owner = msg.sender;
}
}
Destroyable Contract
pragma solidity 0.7.5;
import "./Ownable.sol";
contract Destroyable is Ownable {
function destroy() public onlyOwner {
selfdestruct(msg.sender);
}
}
Bank Contract’s beginning
pragma solidity 0.7.5;
import "./Ownable.sol";
import "./Destroyable.sol";
contract Bank is Ownable, Destroyable {
1 Like
Definitely would emit before self destruct. But interesting point about not being able to verify if contract then actually self destructed…
1 Like
This is the Ownable contract with the file name of InheritancePart2Ownable.sol
pragma solidity 0.7.5;
contract Ownable{
mapping(address => uint)balance;
address owner;
event depositCompleted(uint amount, address indexed depositedTo);
event transferCompleted(address indexed sentBy, uint amount, address indexed receivedBy);
event withdrawalCompleted(address indexed withdrawTo, uint amount);
event selfDestruction(address indexed balancePostDestroyedSentTo);
event selfDestructionv2(address indexed balancePostDestroyedSentTo);
modifier onlyOwner{
require(msg.sender == owner);
_; // run the function
}
constructor() {
owner = msg.sender;
}
}
This is the Destroyable contract with the file name of InheritancePart3SelfDestruct.sol that inherits Ownable
pragma solidity 0.7.5;
import "./InheritancePart2Ownable.sol";
contract Destroyable is Ownable {
function selfDestruct() public onlyOwner payable {
emit selfDestruction(msg.sender);
selfdestruct(msg.sender);
//test if emit will still run after selfdestruct invoked (it doesn't)
emit selfDestructionv2(msg.sender);
//Hypothesis: anything after selfdestruct doesn't run as the smart contract is effectively destroyed
//But the showBalance still work as stated in https://betterprogramming.pub/solidity-what-happens-with-selfdestruct-f337fcaa58a7
}
}
For the Bank contract (just for the import and contract header mainly)
import "./InheritancePart3SelfDestruct.sol";
contract Bank is Destroyable{
function deposit() public payable{
balance[msg.sender]+=msg.value;
emit depositCompleted(msg.value, msg.sender);
}
Ive structured the inheritance whereby Ownable contract is the parent to Destroyable and Bank inherits from Destroyable somewhat making the relationship of the contracts to be Ownable>Destroyable>Bank which probably defines the relationship as a multi-level inheritance?
*Another question would be:
contract Bank is Destroyable, Ownable
results in this error:
“InheritancePart1Bank.sol:7:1: TypeError: Linearization of inheritance graph impossible contract Bank is Destroyable, Ownable{ ^ (Relevant source part starts here and spans across multiple lines).”
whereas if:
contract Bank is Ownable, Destroyable
There isn’t a warning. Does that mean that the sequence of contract inheritance in the contract header is important to how the contract runs?
1 Like
@jon_m, sorry for not getting back to you sooner. I have been struggling to gain insight into other courses before my subscription is up. I will be taking a few days off from the course and in the meantime, I will be making progress elsewhere but I aim to return in a short while. Thank you very much for your time.
1 Like
No problem @JosephGarza, I totally understand — no need to apologise. Just pick things up with this course again when you’re ready. A short break, and focussing on something else can be just as beneficial as ploughing on when you get to that saturation point, which can happen at different stages with different courses. The last thing you need is to feel overly pressurised. Take care, and see you soon
I don’t think the problem is that a user can’t verify if the contract has been destroyed or not, but rather that they would need to do this pro-actively. They wouldn’t be automatically prevented from calling one of the destroyed contract’s functions. The contract owner could notify users that the contract has been destroyed, via an alert in the frontend interface. This could perhaps be triggered by an event listener which listens for an emitted event, if it is decided to emit an event before selfdestruct() is actually called.
1 Like