Is there something I did not understood or any smart contract developer can exit with the money of everyone thx to that selfdestruct function?
1 Like
pragma solidity 0.7.5;
import â./Ownableâ;
contract Destroyable is ownable{
function close() public onlyowner{
selfdestruct(msg.sender);}
}
// that was my answer, and I checked later your solution. I am not sure to understand perfectly you declare a âreceiverâ payable address variable. Didnât you mention that to withdraw an address doesnât not need to be âpayableâ. also isnât everything already included in the âselfdestructâ predefined function?
Thx a lot for your lights
1 Like
Nice solution @AustinBurks 
Can we also see what modifications youâve made to the start of your Bank.sol file, and Bank contract header, for the inheritance?
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 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 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;
}
}
`
`contract Ownable {
address internal owner;
modifier onlyOwner {
require(msg.sender == owner);
_;
}
constructor(){
owner = msg.sender;
}
}```
pragma solidity 0.7.5;
import â./Ownable.solâ;
contract Destroyable is Ownable {
function destroye() public onlyOwner {
address payable receiver = msg.sender;
selfdestruct(receiver);
}
}
Contract compiles but does not deploy
The only message says that all transactions (deployed contracts and function executions) in this environment can be saved and replayed in another environment. eg Transactions created in Javascript vm, can be replayed in the Injected Web3.
They are all three in the same folder labeled contracts. Any advice?
PROBLEM SOLVED
Was using Brave. I opened firefox, navigated to the website, created three folders in the contract folder, deleted everything else, then pasted the code in the respective contracts, it worked!! I think I just needed a fresh start with remix.
1 Like
Hi @m_Robbie,
Your code compiles, but if you deploy it and then try calling closeContract() with different addresses, you will see that it is not only the contract owner that is able to trigger selfdestruct(). We obviously wouldnât want anybody to be able to do this. The reason why this is possible is because the address which calls the closeContract function (msg.sender) is passed to the inherited onlyOwner modifier as the owner parameterâŚ
This means that require() will never fail, because msg.sender will always be equal to msg.sender.
To restrict closeContract to the contract owner, you need to first set the contract owner address (to whichever address you want to have this capability). You canât do that without a state variable. The owner state variable and constructor in Ownable provided this functionality, but youâve removed them. You could have a setter instead of a constructor, but the constructor allows the contract owner to be set immediately on deployment. By setting the owner state variable as msg.sender, the address that deploys the contract becomes the contract owner. However, you could add an address parameter to the constructor which allows the deployer to set owner as a different address.
As well as any address being able to call closeContract() and destroy the contract, the caller can choose to transfer the remaining contract balance to any address of their choosing. Obviously, we donât want that to happen either. The problem is that the input parameter owner (which can be any address, not just the contract ownerâs) is referenced as the argument in the selfdestruct() function call. Once you have ensured that access to this function is restricted to the contract ownerâs address, you can either pass (as the argument)âŚ
- msg.sender (appropriate caller validation will ensure that msg.sender is always the contract owner); or
- the name of the owner address state variable (inherited from Ownable).
msg.sender would not need to be explicitly converted to payable (see below for the reason why). Whether or not the name of the variable (which stores the ownerâs address) needs to be converted, depends on whether it has already been defined and stored as a payable or non-payable address (see below for further details).
After deploying Bank, you will see that your closeContract() function is already available in Bank via inheritance, and can be called from Remix; so, there is no need to add an additional function (endAll) in Bank to be able to call closeContract() and trigger selfdestruct(). The fact that you donât need this additional function in the derived contract, highlights an important advantage of inheritance: it helps to avoid code duplication, while maintaining modularity.
In addition, closeContract() does not need to be marked payable, because the function itself doesnât need to receive any ether. Only the owner address needs to be payable because itâs receiving ether (the remaining contract balance). Making closeContract() payable does not prevent it from executing, but it is better practice (and more secure) to restrict a function to non-payable if it doesnât need to receive ether.
If you define an address state variable as payable, then it will be payable whenever it is referenced (so, thereâs no need to explicitly convert it). However, if it only needs to be payable in specific situations, it is better practice (and more secure) to store it in the contract state as a non-payable address, and then convert it locally whenever it needs to be payable.
In v.0.7 msg.sender is automatically payable by default, but from v.0.8 it is non-payable by default.
Have a go at modifying your code to address these points. You could also put each contract in a separate file to practise using import . Let us know if anything is unclear, or if you have any questions. If you get stuck, you can always have a look at other studentsâ solutions, and the feedback and suggestions theyâve received, which have been posted here, in this discussion topic 
Nice solution @juanmcba
closeSC() does not need to be marked payable , because the function itself doesnât need to receive any ether. Making it payable does not prevent the code from compiling, or the function from executing, but it is better practice (and more secure) to restrict a function to non-payable if it doesnât need to receive ether. Only the owner address needs to be payable because itâs receiving ether (the remaining contract balance). Youâve correctly used msg.sender, which in Solidity v0.7 is a payable address by default, and so doesnât need to be explicitly converted to payable. Just so youâre aware, from v0.8 msg.sender is non-payable by default.
Just to make the code more interesting in terms of the inheritance, you can also try separating your ownership functionality and contract destruction functionality into 2 separate files, giving you a total of 3 files (instead of just 2) in your inheritance structure.
Let us know if anything is unclear, or if you have any questions
Hi @1984,
Yes, if itâs not implemented correctly or securely! The address argument that is passed to selfdestruct() determines which address the contractâs remaining ether balance is sent to when it is destroyed. So, if we hadâŚ
function close() public {
selfdestruct(msg.sender);
}
⌠without the onlyOwner modifier (restricting access to the contract owner), then anyone could call selfdestruct(), destroy the contract and, as the caller is msg.sender, the remaining funds would be transferred to their external wallet address.
If we hadâŚ
function close() public {
selfdestruct(payable(owner));
}
âŚwhere, owner is an accessible state variable, defined as a non-payable address, and set as the contract ownerâs addressâŚ
âŚthen the transfer of the remaining funds would now be restricted to the contract ownerâs address, but anyone could still call selfdestruct(), destroy the contract, and trigger this transfer in the first place. The remaining funds are now secure, but the fact that anyone can destroy the contract is still a huge problem. So thatâs why, as well as ensuring that the address passed to selfdestruct() is a secure destination (from where the remaining funds can be appropriately managed), we also need to restrict who can trigger selfdestruct(). In our example this is done using the onlyOwner modifier inherited from contract Ownable. The onlyOwner modifier applies a require statement which restricts access to the contract ownerâs address (set with a constructor when the contract is deployed)âŚ
// YOUR SOLUTION
function close() public onlyOwner{
selfdestruct(msg.sender);
}
If we restrict access to close() to the ownerâs address, then msg.sender can also only be the ownerâs address. Thatâs why, in this case, we can call selfdestruct() with either msg.sender or owner. In Solidity v.0.7 msg.sender is payable by default, and so there is no need to explicitly convert it to a payable address. However, from v.0.8 msg.sender is non-payable by default, so we would have to do something like the following:
function close() public onlyOwner{
selfdestruct(payable(msg.sender));
}
Whether this âsecureâ implementation is actually secure in practice, obviously depends on whether users can or should trust the contract owner! To understand the implications of this, we need to consider the reasons why we would actually want to include selfdestruct() in our smart contract. This is discussed here.
I hope that answers your question. Let us know if anything is still unclear, or if you have any further questions.
Hi @1984,
Your solution is essentially correct ⌠but if what you have posted here is exactly the same as what you have in Remix, then you should have got a compiler error for this lineâŚ
The imported file will only be recognised if it includesâ.sol
Is your base contract called ownable or Ownable?
Is your modifier called onlyowner or onlyOwner?
The base contract and the modifier will not be inherited unless their names are written exactly the same (including capital v lower case letters) as they are in the base contract. Normally we always start the names we give to contracts with a capital letter. It is easier to read and manage code when these conventions are applied consistently.
Can we also see what modifications youâve made to the start of your Bank.sol file, and Bank contract header, for the inheritance?
To be able to receive ether, an address must be payable. The address passed as an argument to the selfdestruct function must be payable because this is the address that any ether remaining in the contract will be transferred to on destruction.
Note that the close() function doesnât need to be payable. A function only needs to be payable when it receives ether. Thatâs why we marked our deposit function in Bank as payable, but not our withdraw function.
We are using Solidity v0.7 in this course. Prior to v.0.8,âmsg.senderâisâpayableâby default, and so thatâs why you donât have to explicitly convert it here:
selfdestruct(msg.sender);
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));
Another option is to pass the owner variable (inherited from Ownable) to selfdestruct(). However, this is a non-payable address in Ownable:
address owner;
So we either need to define it as a payable state variable in Ownable:
address payable owner;
⌠or explicity convert it locally whenever we need to use it as a payable address e.g.
selfdestruct(payable(owner));
address payable receiver = msg.sender;
selfdestruct(receiver);
The use of the additional local variable receiver is not actually necessary. It was done like this to break down what is happening into more easier-to-understand steps (although this can obviously confuse some students more than it helps!) However, it does highlight that the address passed to selfdestruct() needs to be a payable address. Have a look at this post where you can read more about the use of this receiver variable.
Yes, the functionality that destroys the contract, and transfers any remaining funds in the contract to a specified address, all comes predefined and ready-made within the Solidity language. However its implementation requirements are:
- it takes one argument, which must be a payable address (a non-payable address will not automatically be converted to a payable address);
- wrap it within another function â selfdestruct() will then be called when this âwrapperâ function is called.
Let us know if anything is unclear, or if you have any further questions
Hi Donnie! 
Iâm glad it worked in the end! But why have you suddenly jumped to using v.0.7.5 and the new Solidity courseâs Bank contract? Have you already done the rest of the new Solidity course as well? The reason I ask is because the code looks suspiciously like the model solution provided to check your work after completing the assignment yourselfâŚ
The idea with the new course is to gradually build up your own Bank contract yourself, and then use that in this Inheritance Assignment. There is some very important code missing in your withdraw() function, which you would only know about if you do the Transfer Assignment in the new course â I think it may have been missed out of the model answer.
Also, why have you suddenly started usingâŚ
address payable receiver = msg.sender;
selfdestruct(receiver);
âŚin your destroy() function? ⌠and not one of the 3 methods you worked so hard at understanding before?
This other alternative also works, but do you know why?
First of all, thanks for all your help. I appreciate your taking the time to help me with this.
I finished the first course the same day that I worked through the contract inheritance and started the second class (ETH 101 0.7.5). I donât know why, but when I started the 0.7.5, the first lesson that it gave me was inheritance, skipping ahead. I found the code as best I could around Filipâs teaching. I watched the same video a couple of times but felt that I had missed some code. Jon, I would rather have started from the beginning, but I have learned from times past, that if I ask your team to reset the lesson, the answer I have received is that I can take the class as many times as I like. I think that means that you canât or have been instructed not to reset the lessons. So, I read the assigned article and copied the self destruct code that was there. I have now finished the government contract, and hope to spend more time on it today.
Donât worry, I saved the other contracts in atom as references.
âŚin your destroy() function? ⌠and not one of the 3 methods you worked so hard at understanding before?
This other alternative also works, but do you know why?
-Yes,
- because it is the code from the reading assignment.
- I remembered that in a post somewhere you told me that 0.7 had different rules. I didnât want to
copy what worked before. My goal was to take this class as is as best I can.
sooo if you can reset the course to #1, I would gladly start over.
With what I have put together, it all works. Canât say I fully understand everything, but it works. Here is my code?
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 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 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;
}
}
contract Ownable {
address payable internal owner;
modifier onlyOwner {
require(msg.sender == owner);
_;
}
constructor(){
owner = msg.sender;
}
}
pragma solidity 0.7.5;
import "./Ownable.sol";
contract Destroyable is Ownable {
function close() public onlyOwner{
selfdestruct(owner);
//option 1
//address payable receiver = address(uint160(owner));
// selfdestruct(receiver);
// option2
// function close() public onlyOwner {
// selfdestruct(address(uint160(owner)));
// selfdestruct(Owner);
}
}
I think I am on target, following along at least for now.
1 Like
contract Destructible is Ownable {
function destruct() onlyOwner {
selfdestruct(owner);
}
}
So, I can deposit Eth into my Bank.sol contract. I can withdraw with no errors, but when I transfer, I get an error. To transfer, I have gone to the top![Screen Shot 2021-05-14 at 4.18.21 PM|517x499]
(upload://lruqF1rkA9FzOz0GE4RKeD2crV3.png)
From what I have gathered, the first address is the Bank.sol contract. The others are available to transfer money to. So, I have copied some of the others, and tried to send money to them. After I hit copy, I moved the first back to the field, so that the window shows only the first address, but I paste another into the address bar as recipient. and I am getting an error. The code looks identical to me, what am I missing?
The error message says that the call function should be payable. I tried payable between âamount)â and âpublicâ, it didnât work.
Thank you so much for your reply! I will address your tips.
Also, I checked the solution code and I noticed that in Destroyable contract, there is a line of code stating the msg.sender as payable. Is this 100% necessary? Iâm not sure why is it so.
import "./Ownable.sol";
pragma solidity 0.7.5;
contract Destroyable is Ownable {
function destroy() public onlyOwner {
address payable receiver = msg.sender;
selfdestruct(receiver);
}
}
1 Like
import â./ownable.solâ;
pragma solidity 0.5.12;
contract SelfDestruct is Ownable {
function toSuicide() public onlyOwner{
selfdestruct (msg.sender);
}
}
1 Like
Seems like I keep on running into a
âcreation of Destroyable errored: Cannot convert undefined or null to objectâ
errorâŚ
Just tried out the solution posted on GitHub.
Anything I might be missing?
Oh dear, that does sound frustrating! I think I understand the problem you are having. If you enter the course from the My Courses page (which I think is the default page when you log in to the Academy) then the algorithm will automatically take you to what it thinks you should be watching next. But if you want to start from the beginning, there are 2 ways to override this:
-
Navigate to the All Courses page (from the menu at the top of the page) and find the course. Then, when you enter it, it should take you to the course contents page, where you can click on the very first lesson and start from there;
-
Enter the course as normal from My Courses, and on the lesson page it takes you to, go to the âindex pathâ (like a file path) just above the video, and navigate to the course contents page from there.
You can also use either the course contents page, or the âindex pathâ on each page, to navigate to any lesson in the course you want to watch again (again, overriding the automatic selection algorithm). I think this is what was meant byâŚ
If all, or some of the lessons in the new course have already been marked as completed, then in each individual lesson you can click on the blue button below the video that says Complete. It should then turn back to white and say Mark As Complete. If you want to start the course from the beginning and work your way through from lesson to lesson, then you could go into all the lessons it says youâve completed and reset this button â that way the âContinue where you left offâ algorithm should work again as it should for someone starting the course from the very beginning, and so you should then be able to just continue with the course from the default My Courses page (probably what you usually use).
So⌠we canât do that for you⌠but you should be able to 
Let me know if that makes sense, and if it works for you
For this Inheritance Assignment, the code youâve posted looks good, Donnie
Iâve deleted all of your code relating to the Government contract and the external function call, because that comes in the next section, so you need to post any code related to that here. I donât want to over-complicate this discussion post with code students wonât have seen yet when they do this assignment
In terms of the code in the close() function bodyâŚ
The method you have used in your code is correct: define the owner address as payable within OwnableâŚ
address payable owner;
⌠and call selfdestruct() with owner . This still works in v0.7 using the same syntax.
Itâs only in v0.8 where msg.sender changes from being payable to non-payable by default. So, you can use â selfdestruct(msg.sender); â for v.0.7 as well.
So, instead of having to explicitly convertâownerâto a payable address usingâŚ
address(uint160(nonpayableAddress));
âŚwe can now perform this conversion using the simplerâŚ
payable(nonpayableAddress);
So, you need to modify your 2 options (comments in your close() function) for this change.
Hopefully you can go back and complete this course from the beginning. If you do, then, when you come to do the Transfer Assignment you will notice that you have an important line of code missing in your withdraw() function. If you test your code as it is at the moment, you should be able to already see what the serious bug isâŚ
Hi Donnie,
This could well be an error thrown because you havenât previously deployed your Government contract and/or havenât replaced the Government address in your Government instance definition (in Bank) with the new one. You should be deploying your Government contract before deploying Bank. Then, if you redeploy Government at any stage, it will have a different contract address, which needs to be updated in Bank, otherwise the external contract call to addTransaction() will fail and cause the whole transfer() function to revert.
If this is what is causing the error, the problem is that the error message you get is just a default one, and so thatâs why it isnât helpful in pinpointing the error. If itâs a default message then the error may not have anything to do with the function not being marked payable. The withdraw() function does not need to receive ether (it only sends it) so it doesnât need to be marked payable. This is why adding payable to the function header didnât solve the problem.
Ideally, I think you should go back and complete the earlier part of the course, because if you havenât seen the lectures where the transfer function is introduced and explained, it will be difficult to follow and fully understand the additional changes we make with the external function call (within the transfer function) to the Government contract.
Also, I think you are missing some key principles from the earlier lessons and assignments:
No, all of the addresses in the dropdown are simulating external wallet addresses for different users. The one you deploy the contract with (by default the top one) will become the Bank contract owner â as a result of our constructor and owner state variable.
The contract address is the one next to the deployed contract where you can open up all of the function calls you are able to make from Remix. You only need to copy and paste the Government contract address whenever you need to replace the one in the contract instance definition (explained above).
The Bank contract address automatically receives ether when the deposit() function is called. Thatâs why we make it a payable function.
<address payable>.transfer(uint256 amount) â automatically transfers ether from the contract address, to whichever external wallet address (from our dropdown) calls the withdraw() function.
The transfer() function that you are talking about (and which is throwing an error and reverting) involves no ether being transferred between an external wallet address and the contract address. When transfer() is executed the total amount of ether held in the contract remains the same â it is only an internal Bank transfer between two bank account holders, reflecting the change in each of their shares in the total funds.
However, we do identify each account holderâs internal Bank balance (stored in the mapping) by their external wallet address, so doing the following is correctâŚ
⌠except that, after copying the recipientâs address, you can then select any address that has a bank account balance > 0, because, as I have just mentioned, you arenât transferring from the contract address, but simply making an internal transfer between 2 account holders. When youâve got this transfer() function working, you will see the change in the senderâs and recipientâs bank account balance by calling the getBalance() function with their respective addresses. But, you wonât see a change in the ether balance in their external wallets, because this will only change when a deposit or withdrawal is made to/from the contract address.
⌠but try depositing amounts from several addresses, and then get one of those addresses to withdraw what it deposited. If the contract works as it should, if that same address then tries to withdraw another amount from the contract, the transaction should revert ⌠but what actually happens with your code? âŚWhy?.. the answer will tell you what this missing line is that I keep mentioningâŚ
if Destroyable is Ownable then you only need
Bank is Destroyable not Bank is Destroyable, Ownable right?
https://github.com/filipmartinsson/solidity-0.7.5/blob/main/inheritance-assignment/Bank.sol#L5
1 Like
Correct
Explicitly inheriting both Destroyable and Ownable, is not wrong *⌠but as you have correctly observed we can also streamline the inheritance by having Bank just inherit Destroyable, because it will then indirectly inherit Ownable via Destroyable. This will give you a multi-level inheritance structure.
* but then it must beâŚ
Bank is Ownable, Destroyable {...}
// not ...Destroyable, Ownable...