Inheritance Assignment

jon_m · November 23, 2021, 1:07am

Nice solution @kasunkv

Your inheritance structure is well coded, and you’ve met all of the assignment objectives

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.

You’ve also added some well-coded additional functionality:

OwnershipSet event, and corresponding emit statement in the constructor.
getContractBalance function. When testing the contract, it’s very useful to be able to check the total contract balance as well as individual user balances.

In fact, after calling your transfer() function, you can use your two getter functions to check 3 balances: (i) the total contract balance (ii) the sender’s individual balance in the mapping, and (iii) the recipient’s individual balance in the mapping. If the transfer() function was working correctly, the total contract balance would equal the sum of all of the individual user balances. However, with your current code, yours won’t, as a result of the issue I raised in my feedback for your Transfer Assignment solution.

This is actually a serious vulnerability, because the recipients of transfers will not only receive the ether in their external accounts, but their entitlement to withdraw ether from the contract (determined by their individual balances in the mapping) will also be overstated by equivalent amounts. This means these users will potentially be able to withdraw more ether from the contract (either to their own external address, or as a transfer to any other external address of their choice) than rightfully belongs to them, which effectively amounts to stealing other users’ funds.

You will see from my other feedback that this is easily fixed.

Let me know if you have any questions.

jon_m · November 23, 2021, 1:27am

Nice solution @skawal22

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 you have any questions

jon_m · November 23, 2021, 12:37pm

Nice solution @t3hmun

Your inheritance structure is well coded, and you’ve met all of the assignment objectives.

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.

Even though the Ownable and Destroyable contracts are small, it is still better practice to put each in a separate file with its own pragma statement. You would then need to add the necessary import statements to Bank.sol and Destroyable.sol

Your owner state variable does have internal visibility. You can see this when you deploy Bank in Remix: owner is available in the derived contract Destroyable — it is passed to selfdestruct so that the contract owner receives the ether remaining in the contract when it is destroyed — but no automatic getter is created to access the owner address externally while the contract is still operational. Unlike functions, the visibility of state variables defaults to internal when the visibility keyword is omitted; unlike public and private, the internal keyword is optional.

Well observed! After selfdestruct() has been executed, function calls are still successfull and don’t throw errors! We can be sure that the contract has been destroyed, because the getters now return zero values. However, as you have discovered, a clear disadvantage with using selfdestruct is that, after the contract has been destroyed, a payable function can still be called, by mistake, with an ether value and, instead of reverting, the ether will be lost. Therefore, if a smart contract deployed on the mainnet is ever destroyed with selfdestruct , all users need to be notified and informed that they must not send any more funds to that contract address. Obviously, this is hardly ideal!

The potential repercussions of selfdestruct 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. More effective alternatives do exist, such as pausable contracts. These involve more advanced techniques and are therefore out of the scope of this introductory course, but you can learn about pausable contracts and other smart-contract emergency and security features, such as proxy contracts, in the Smart Contract Security course.

Let me know if you have any questions

jon_m · November 23, 2021, 1:35pm

Hi @giomiri,

Apart from two missing import statements at the top of Bank.sol (Did you just forget to include them in your posted code?), your inheritance structure is well coded. You should also include a pragma statement at the top of every .sol file.

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.

When your contract is destroyed, the remaining contract balance is successfully transferred to the contract owner’s external address. But you don’t need to include the additional two lines of code to achieve this…

These two additional lines achieve the desired result, and from a technical point of view they are well coded. However, as well as destroying the contract, selfdestruct will also automatically transfer the remaining contract balance to the address argument it is called with (in your case, owner) …

This is all part of the pre-defined selfdestruct functionality. If you remove these two lines of code, you will see that calling the close() function produces exactly the same results.

However, even though the remaining contract balance can only be sent to the contract owner, and the funds are therefore theoretically safe (in as far as we can trust the contract owner to be a good temporary custodian), any address can call the close() function and trigger selfdestruct in the first place. This is because your onlyOwner modifier has no effect, for the same reason I mentioned in my feedback for your Transfer Assignment solution …

Once you place the condition within a require statement, this modifier will work as it should, and only the contract owner will be able to call close() and successfully trigger selfdestruct.

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

unwise_guy · November 23, 2021, 4:01pm

pragma solidity 0.7.5;

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

pragma solidity 0.7.5;

import "./Ownable.sol";

contract destroyable is Ownable{
    
    
    
    function destructo()public onlyOwner{
        selfdestruct(owner);
    }
    
}

pragma solidity 0.7.5;

import "./destroyable.sol";

contract Bank is destroyable {

skplunkerin · November 26, 2021, 5:46pm

I wasn’t sure if it’s possible (or even good practice) to have contracts be both a Parent and Child to each other, so I made Destroyable be an Abstract Contract to enforce the Child Bank contract to override the Destroy() function derived from Destroyable. Please let me know if there’s a more appropriate way to do what’s needed in this assignment, as well if there’s anything I can do better at in my code.

Thank you for taking time to review my solution!

[update]

I learned from reading above feedback that “selfdestruct(owner)” will do the funds transfer automatically… I added lots of extra code here in order to know the available “TotalFunds” to know how much to transfer to the owner… looks like I don’t need to do that. I’m going to re-submit with a more streamlined solution.

see new solution here

File: destroyable.sol:

pragma solidity 0.8.7;

abstract contract Destroyable {
    function Destroy() virtual public payable;
}

File: ownable.sol:

pragma solidity 0.8.7;

contract Ownable {
    address payable public owner;
    
    constructor() {
        owner = payable(msg.sender);
    }
    
    modifier isOwner {
        require(msg.sender == owner, "Must be the owner to run this function");
        _;
    }
}

File: bank.sol:

pragma solidity 0.8.7;

import "./destroyable.sol";
import "./ownable.sol";

contract Bank is Ownable, Destroyable {
    //// EVENTS ////
    event deposited(uint _amount, address indexed _sender);
    event transfered(uint _amount, address indexed _sender, address indexed _receiver);
    event withdrawn(uint _amount, address indexed _receiver);

    //// STATE VARIABLES ////
    mapping(address => uint) Balance;
    uint TotalFunds;

    //// PUBLIC/VISIBLE FUNCTIONS ////
    
    // Deposit transfers funds from the sender to this smart contract;
    // the Balance state variable is then updated for visibility.
    function Deposit() public payable{
        _addBalance(msg.value, msg.sender);
        emit deposited(msg.value, msg.sender);
    }

    // GetBalance returns the balance of the caller address (msg.sender).
    function GetBalance() public view returns(uint){
        return Balance[msg.sender];
    }

    // Transfer will send tokens from msg.sender to a _recipient address
    // if they have the funds to do so and the _recipient isn't the sender.
    function Transfer(uint _amount, address _recipient) public {
        require(Balance[msg.sender] >= _amount, "Insufficient tokens.");
        require(msg.sender != _recipient, "Cannot send tokens to self.");

        // NOTE: is it safer to handle this manually?
        _subtractBalance(_amount, msg.sender);
        _addBalance(_amount, _recipient);
        emit transfered(_amount, msg.sender, _recipient);
    }

    // Withdraw transfers funds out of the contract to the callers
    // address, if they have enough funds deposited.
    function Withdraw(uint _amount) public returns(uint){
        require(Balance[msg.sender] >= _amount, "Insufficient funds");

        address payable msgSender = payable(msg.sender);
        _subtractBalance(_amount, msgSender);
        msgSender.transfer(_amount);

        emit withdrawn(_amount, msgSender);
        return Balance[msgSender];
    }

    // Destroy transfers all contract ETH to the owner, then calls
    // selfdestruct(owner).
    function Destroy() public override isOwner payable {
        owner.transfer(TotalFunds);
        selfdestruct(owner);
    }

    //// PRIVATE/HIDDEN FUNCTIONS ////

    // _addBalance adds the _amount to the _forAddress Balance, and adds the _amount
    // to TotalFunds.
    function _addBalance(uint _amount, address _forAddress) private {
        uint prevTotal = TotalFunds;
        uint prevBalance = Balance[_forAddress];

        Balance[_forAddress] += _amount;
        TotalFunds += _amount;

        assert(Balance[_forAddress] == prevBalance + _amount);
        assert(TotalFunds == prevTotal + _amount);
    }

    // _subtractBalance removes the _amount from the _forAddress Balance, and
    // subtracts the _amount from TotalFunds.
    function _subtractBalance(uint _amount, address _forAddress) private {
        uint prevTotal = TotalFunds;
        uint prevBalance = Balance[_forAddress];

        Balance[_forAddress] -= _amount;
        TotalFunds -= _amount;

        assert(Balance[_forAddress] == prevBalance - _amount);
        assert(TotalFunds == prevTotal - _amount);
    }
}

skplunkerin · November 26, 2021, 5:47pm

Here’s my streamlined approach after I learned that selfdestruct(owner) automatically transfers all Contract funds to the owner; the approach is now a clean-and-simple Multi-Level Inheritance structure:

File: ownable.sol:

pragma solidity 0.8.7;

contract Ownable {
    address payable public owner;
    
    constructor() {
        owner = payable(msg.sender);
    }
    
    // isOwner requires that msg.sender is the contracts owner.
    modifier isOwner {
        require(msg.sender == owner, "Must be the owner to run this function");
        _;
    }
}

File: destroyable.sol:

pragma solidity 0.8.7;

import "./ownable.sol";

contract Destroyable is Ownable {
    // Destroy transfers all contract ETH to the owner and
    // self-destructs.
    function Destroy() public isOwner payable {
        selfdestruct(owner);
    }
}

File: bank.sol:

pragma solidity 0.8.7;

import "./destroyable.sol";

contract Bank is Destroyable {
    //// EVENTS ////
    event deposited(uint _amount, address indexed _sender);
    event transfered(uint _amount, address indexed _sender, address indexed _receiver);
    event withdrawn(uint _amount, address indexed _receiver);

    //// STATE VARIABLES ////
    mapping(address => uint) Balance;

    //// PUBLIC/VISIBLE FUNCTIONS ////
    
    // Deposit transfers funds from the sender to this smart contract;
    // the Balance state variable is then updated for visibility.
    function Deposit() public payable{
        _addBalance(msg.value, msg.sender);
        emit deposited(msg.value, msg.sender);
    }

    // GetBalance returns the balance of the caller address (msg.sender).
    function GetBalance() public view returns(uint){
        return Balance[msg.sender];
    }

    // Transfer will send tokens from msg.sender to a _recipient address
    // if they have the funds to do so and the _recipient isn't the sender.
    function Transfer(uint _amount, address _recipient) public {
        require(Balance[msg.sender] >= _amount, "Insufficient tokens.");
        require(msg.sender != _recipient, "Cannot send tokens to self.");

        _subtractBalance(_amount, msg.sender);
        _addBalance(_amount, _recipient);
        emit transfered(_amount, msg.sender, _recipient);
    }

    // Withdraw transfers funds out of the contract to the callers
    // address, if they have enough funds deposited.
    function Withdraw(uint _amount) public returns(uint){
        require(Balance[msg.sender] >= _amount, "Insufficient funds");

        address payable msgSender = payable(msg.sender);
        _subtractBalance(_amount, msgSender);
        msgSender.transfer(_amount);

        emit withdrawn(_amount, msgSender);
        return Balance[msgSender];
    }

    //// PRIVATE/HIDDEN FUNCTIONS ////

    // _addBalance adds the _amount to the _forAddress Balance and asserts the
    // action worked as intended.
    function _addBalance(uint _amount, address _forAddress) private {
        uint prevBalance = Balance[_forAddress];
        Balance[_forAddress] += _amount;
        assert(Balance[_forAddress] == prevBalance + _amount);
    }

    // _subtractBalance removes the _amount from the _forAddress Balance and
    // asserts the action worked as intended.
    function _subtractBalance(uint _amount, address _forAddress) private {
        uint prevBalance = Balance[_forAddress];
        Balance[_forAddress] -= _amount;
        assert(Balance[_forAddress] == prevBalance - _amount);
    }
}

jon_m · November 28, 2021, 12:16am

This is an excellent piece of work @skplunkerin

Your final solution meets all of the assignment objectives, and you’ve documented really well how you’ve improved your initial version, and the reasons for your modifications

Even though your final version is an improvement, your initial version does still demonstrate some great knowledge of abstract contracts and function overriding. I’ll give you some additional feedback on this first version in a separate post.

Final version – a couple of additional observations

(1)

The destroy() function doesn’t need to be marked payable. Only functions that receive ether into the contract (adding it to the contract balance) should be marked payable. Functions that only send ether out of the contract (deducting it from the contract balance) should remain non-payable.

Marking the destroy() function as payable doesn’t prevent your code from compiling, or the function from executing, but it is bad practice and poor risk management not to restrict a function to non-payable if it doesn’t need to receive ether. This is because if someone sends ether by mistake, this amount will still be transferred to the contract. As we are programming money, we need our code to be as risk-averse as possible.

In actual fact, because the destroy() function transfers the remaining contract balance to the contract owner, if any ether was sent by mistake, it would immediately be returned together with the rest of the contract balance anyway; so in this particular case there can be no adverse effect from making the function payable. However, in other cases it could have adverse effects, and so it’s best not to get into bad habits.

(2)

In Solidity, the convention is to start the names of mappings and functions with a lower case letter — the same as with the names of variables, arrays, arguments, parameters and modifers. The names of events, structs, contracts and interfaces usually start with a capital letter. Doing the opposite will not prevent your code from compiling, but following this generally accepted convention does apply a consistency to our code, which can make it more readable.

As always, just let me know if you have any questions

skplunkerin · November 28, 2021, 2:01am

Thanks again Jon, this feedback is very helpful!

jon_m · November 28, 2021, 2:14am

As you found out, in this particular contract there isn’t actually any need to track the total contract balance, because selfdestruct's pre-defined functionality will automatically transfer whatever the contract balance is to the payable address argument it is called with, anyway.

However, if we do need to reference or retrieve the contract balance at any time, Solidity provides us with some simple syntax for this…

address(this).balance

It’s actually quite useful (especially for testing purposes) to include a getter such as the following in our contract …

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

If you ever needed to transfer the whole contract balance to an external address, you could do it as follows…

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

Instead of an abstract contract, you could make Destroyable an interface…

interface Destroyable {
    function destroy() external;
}

Function definitions in interfaces have to have external visibility, but their overriding implementations can change external visibility to public.
Also, function definitions in interfaces are implicitly virtual, and so this keyword doesn’t need to be included in the function header.
Here are links to the relevant Solidity documentation for abstract contracts and interfaces, in case you haven’t already seen it…

https://docs.soliditylang.org/en/latest/contracts.html#abstract-contracts
https://docs.soliditylang.org/en/latest/contracts.html#interfaces

However, the destroy() function itself is only a “wrapper” for selfdestruct(), and because a function definition without implementation can’t include a modifier, having just a definition of destroy() without the isOwner modifier and selfdestruct(), is actually pretty meaningless in either an interface or an abstract contract. Instead, it’s much more appropriate to have the destroy() function fully implemented within Destroyable, so that the contract destruction functionality can be inherited by derived contracts as a “complete package” to be used “as is” and not overridden. This more prescriptive approach is also a safer one, because destroy() has the potential to cause a lot of damage if it isn’t implemented securely.

Let me know if you have any questions.

skplunkerin · November 28, 2021, 2:19am

Thank you Jon, this is also very helpful feedback!

angnimasherpa · November 28, 2021, 5:57pm

pragma solidity 0.7.5;

contract Ownable {
    address internal owner;
    constructor() {
        owner = msg.sender;
    }

    modifier onlyOwner() {
        require(owner == msg.sender, "Ownable: Only owner can call it!");
        _;
    }
}

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

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

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

contract Bank is Ownable, Destroyable {
}

jon_m · November 30, 2021, 12:57am

Nice solution @angnimasherpa

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 you have any questions

angnimasherpa · November 30, 2021, 7:17am

ahh right!
As the Destroyable function already has the inheritance of Ownable. It’s possible to use the Ownable contracts function through Destroyable by using the multi-level inheritance!
Nice, will keep it in mind from next time.
Thank you @jon_m!

Austaino · December 3, 2021, 12:49pm

Bank Contract:-

pragma solidity 0.7.5;

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

contract Bank is ownable, destroyable{

Ownable Contract:-

pragma solidity 0.7.5;

import “./Destroyable.sol”;

contract ownable{

   address public  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 taminateContract()public onlyOwner {
    selfdestruct (msg.sender);

}

}

jon_m · December 4, 2021, 2:00pm

Hi @Austaino,

Unfortunately there’s just one import statement which is preventing the inheritance structure from working, and Bank from compiling…

Ownable is the parent of Destroyable, and so you don’t need to import Destroyable.sol into Ownable.sol. This is confusing the compiler and throwing an error in Bank. If you remove this import statement, Bank will compile and deploy successfully, and the rest of your code will then work as it should, meeting all of the assignment objectives

We can also make the inheritance structure more streamlined 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 you have any questions

Austaino · December 5, 2021, 10:15am

pragma solidity 0.7.5;

import “./Ownable.sol”;

contract destroyable is ownable{

function taminate()public onlyOwner {
    address payable receiver = msg.sender;
    selfdestruct (receiver);

}

}

pragma solidity 0.7.5;

contract ownable{

   address public  owner;

   modifier onlyOwner{
       require(msg.sender == owner);
       _; // run the function.
   }

   constructor() {
       owner = msg.sender;
   }

}

Thanks for the info, I can see that Destroyable is Ownable, since Ownable is the parent contract. Therefore Bank and Destroyable can inherit Ownable contract. But this level of inheritance is not explain. however the Multi Level inheritance in which the Bank or inheritance contract will directly inherit the destroyable and destroyable inherit the ownable contract is more straight forward.

Thanks a lot, I get it now, But I think I would rader use the multi lever inheritance for this assignment its much better.

thanks again.

pragma solidity 0.7.5;

import “./Destroyable.sol”;

/*import "./internalVisibility.sol";**/

contract Bank is ownable,destroyable{

   mapping(address => uint) balance;

Austaino · December 6, 2021, 2:44pm

sorry I meant this.
ragma solidity 0.7.5;

import “./Destroyable.sol”;

/*import "./internalVisibility.sol";**/

contract Bank is destroyable{

   mapping(address => uint) balance;

jon_m · December 6, 2021, 4:46pm

Hi @Austaino,

I think you’ve got it now…

Your 2nd post will give you multi-level inheritance…

i.e

contract Ownable { ... }
contract Destroyable is Ownable { ... }
contract Bank is Destroyable { ... }

… and this is what I was suggesting was the best approach …

contract Bank is Ownable, Destroyable { ... } also works but is unnecessary… is this the version that you say wasn’t explained? …

One other observation …

This is also correct, but I think your original solution for this function is better, because it’s cleaner and more concise …

Have a look at this post for further details about the use of the additional local variable receiver in the model solution.

In Solidity v.0.7 msg.sender is a payable address by default. However, from v.0.8 it is non-payable by default, and so would need to be converted to a payable address in order to pass it to selfedestruct() as its argument.

farhan.qureshi · December 7, 2021, 5:44am

1.1 Full Code from Ownable Contract

pragma solidity 0.7.5;

contract Ownable {

address public owner;

modifier onlyOwner {

    require(msg.sender == owner);

    _;  // run the function if above is true

}

constructor() {

    owner = msg.sender;

}

}

1.2 Full Code from Destroyable Contract

pragma solidity 0.7.5;

contract Destroyable {

function destroy(address payable owner) internal {

    selfdestruct(owner);

}

}

2 Code (new method only) from the main Bank Contract

pragma solidity 0.7.5;

import “./Ownable.sol”;

import “./Destroyable.sol”;

contract Bank is Ownable, Destroyable {

mapping(address => uint) balance;

event deposited(uint amount, address despositedTo);

event transferred(uint amount, address beneficiary);

function destroy() public onlyOwner {

    withdraw(balance[msg.sender]);

    destroy(msg.sender);

}