Transfer Assignment

[_Vincent]

Hi @jon_m,
I made a video on the error, so maybe you’ll understand what happens : https://www.youtube.com/watch?v=r1UKk8aOmwg

This is very strange, because the same actions worked well, unless I made a mistake.
I perform just a transfer, with the right number of zero. And no “payable” term to add in the official source code for transfer() function (I write that, because sometimes error message caught and it relates a missing payable term).

Thanks,
Vincent.

[jon_m]

Hi @_Vincent,

Can you post an exact copy of the full code in the contract file you are deploying and interacting with? I also want to deploy and test your actual code myself

And just to confirm, you should be doing and seeing something like the following…
(I’m going to use 2 different addresses from the Account field dropdown — let’s call them A and B)

1. Call deposit() function with Address A showing in Account field, and 5 ether in Value field. Make sure the dropdown next to the Value field is set to ether (not wei). No other inputs.
2. You see Address A’s external account balance (showing in the Account field) reduce by 5 ether (e.g. from 99.9999 to 94.9999)
3. Call getBalance() with Address A, output 5000000000000000000
4. Call getContractBalance(), output 5000000000000000000
   EFFECT: Address A’s external balance -5 ether; Contract balance +5 ether (all belongs to Address A)
5. Call transfer() function with Address A showing in Account field, and 2 arguments: recipient Address B, amount 2000000000000000000
   After you’ve copy-and-pasted Address B to input as the recipient argument, don’t forget to change the address showing in the Account field back to Address A before calling the function
   Do not input a value in the Value field. Your transfer() function should not be marked payable.
6. The external account balances of both addresses do not change.
7. Call getBalance() with Address A, output 3000000000000000000
8. Call getBalance() with Address B, output 2000000000000000000
9. Call getContractBalance(), output 5000000000000000000
   CUMULATIVE EFFECT: Address A’s external balance -5 ether; Contract balance +5 ether (3 ether belongs to Address A, and 2 ether belongs to Address B)
10. Call withdraw() function with Address B showing in Account field, and 1 argument: amount 1000000000000000000
11. You see Address B’s external account balance (showing in the Account field) increase by 1 ether (e.g. from 99.9999 to 100.9999)
12. Call getBalance() with Address B, output 1000000000000000000
13. Call getBalance() with Address A, output 3000000000000000000
14. Call getContractBalance(), output 4000000000000000000
    CUMULATIVE EFFECT: Address A’s external balance -5 ether; Address B’s external balance +1 ether; Contract balance +4 ether (3 ether belongs to Address A, and 1 ether belongs to Address B)
15. Call transfer() function with Address B showing in Account field, and 2 arguments: recipient Address A, amount 1000000000000000000
    After you’ve copy-and-pasted Address A to input as the recipient argument, don’t forget to change the address showing in the Account field back to Address B before calling the function
    Do not input a value in the Value field. This transaction should not revert, because the require statement should not fail if it is coded correctly).
16. The external account balances of both addresses do not change.
17. Call getBalance() with Address B, output 0
18. Call getBalance() with Address A, output 4000000000000000000
19. Call getContractBalance(), output 4000000000000000000
    CUMULATIVE EFFECT: Address A’s external balance -5 ether; Address B’s external balance +1 ether; Contract balance +4 ether (all belongs to Address A)

You can then try the same thing with different amounts, and then different transaction combinations.

[_Vincent]

Hi @jon_m,
many thanks for your process.

Unfortunately, I stuck at the step 5, with following output :
"Note: The called function should be payable if you send value and the value you send should be less than your current balance."
Debug infos : "Transaction mined but execution failed"

Here is my code :

OWNABLE.SOL :

pragma solidity 0.7.5;

contract Ownable
{
    address payable public owner;
    
    address public contractAddress;
    uint256 public contractBalance;
    
    constructor() payable
    {
        owner = msg.sender;
        //contractBalance = address(this).balance;
        contractAddress = address(this);
    }
    
    
     //MODIFIERS
    modifier onlyOwner()
    {
        require(msg.sender == owner);
        _;       
    } 
}

DESTROYABLE.SOL :

pragma solidity 0.7.5;

import "./Ownable.sol";

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

BANK.SOL :

pragma solidity 0.7.5;

import "./Destroyable.sol";

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

contract Bank is Destroyable
{
    GovernmentInterface govInstance = GovernmentInterface(0xd9145CCE52D386f254917e481eB44e9943F39138);
    
    constructor() payable {
    }
    
    
    mapping(address => uint) balance;
    
    //EVENTS
    event balanceAdded(uint amount, address indexed depositedTo);
    event depositDone(uint value, address indexed sender);
    event closureDone(uint value, address indexed sender);
    

    //BALANCE
    function addBalance(uint _amount) public onlyOwner returns(uint)
    {
        balance[msg.sender] += _amount;
        emit balanceAdded(_amount,msg.sender);
        
        return balance[msg.sender];
    }
    
    function getBalance(address _address) public view returns(uint)
    {
       return balance[_address];
    }
            
    //address that originally deploys the contract
    function getOwnerBalance() public view returns(uint)
    {
       return balance[msg.sender];
    }
    
    
    function getContractBalance() public view returns(uint) {
    return address(this).balance;
}
    
    //DEPOSIT 
    function depositToOwner() public payable returns(uint)
    {
        balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    //WITHDRAW
    function withdraw(uint amount) public payable onlyOwner returns(uint)
    {
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }

    
    //TRANSFER
    function transfer(address _recipient,uint _amount) public {
        //check balance of msg.sender
        require(balance[msg.sender] >= _amount, "Balance not sufficient");
        require(_recipient != owner, "Don't transfer money to yourself");
        
        uint previousBalance = balance[msg.sender];
        
        _transfer(msg.sender, _recipient, _amount);
        
        govInstance.addTransaction(msg.sender, _recipient, _amount);
        
        assert(balance[msg.sender] == previousBalance - _amount);
    } 
    
  
    function _transfer(address _from, address _to, uint _amount) private
    {
        balance[_from] -= _amount;
        balance[_to] += _amount;
    }
}

[skawal22]

My Solution:

pragma solidity 0.8.9;

contract Bank {

mapping(address => uint) balance;
address owner; 
event depositDone(uint amount, address indexed depositedTo);
//event balanceTransferred(uint amount, address indexed depositedFrom, address indexed depositedTo);

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

constructor (){
    owner = msg.sender;
}

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, "Balance not sufficient");
    payable(msg.sender).transfer(amount);
    balance[msg.sender] -= 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);
     
    // emit balanceTransferred(amount, msg.sender, recipient);
     
     assert(balance[msg.sender] == previousSenderBalance - amount);
 }
 
 function _transfer(address from, address to, uint amount) private {
     balance[from] -= amount;
     balance[to] += amount;
 }

}

[jon_m]

Hi @_Vincent,

Good job I asked to see your full code, because I didn’t realise you were including the external function call to the Government contract…

@_Vincent

govInstance.addTransaction(msg.sender, _recipient, _amount);

I think what’s probably causing your transfer() function to revert is not changing the Government contract’s address assigned to your govInstance in Bank, after you’ve deployed Government. Have a look at this post, which explains the issue and how to resolve it. It also explains why the error message you’re getting isn’t very helpful.

If that doesn’t work, post your Government contract as well, as that is part of your solution (you should be deploying it before Bank).

[jon_m]

Hi @_Vincent,

Other comments and issues with your contracts…

Ownable

(1)

@_Vincent

uint256 public contractBalance;

This code can be removed, because it serves no purpose. No value is assigned to it. Only the Bank contract needs to be deployed, because it inherits the necessary functionality from Destroyable and Ownable. So, the getContractBalance() function you’ve included in Bank is sufficient to retrieve the deployed Bank contract’s balance at any specific point in time.

(2) You don’t need to use the constructor to assign the contract address to the contractAddress state variable on deployment. The following line of code will do this more concisely…

address public contractAddress =  address(this);

Because this state variable is inherited, address(this) will capture the address at which the derived contract (which inherits this state variable) is deployed.

(3)  The constructor doesn’t need to be marked payable

Destroyable 

Bank

(1) You don’t need another constructor in Bank.

(2) This getter …

@_Vincent

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

… will return the balance of any address that calls it, and not just the contract owner’s balance. To allow anybody to check the contract owner’s balance, you would have to look up the owner 's balance in the mapping (not msg.sender 's) …

return balance[owner];

(3) The addBalance() function is not right in this contract, and should have been removed when the deposit() function was added. The addBalance() function was for adding non-ether amounts before we learnt how to send ether to a contract using a payable function. The increase to a user’s individual balance in the mapping, by the same amount as the ether sent to the function and deposited in the contract, is now being performed in the deposit() function. Having both functions allows the contract owner to inflate their individual balance in the mapping, by calling the addBalance() function, and then to withdraw more than their share of the total contract balance, effectively stealing other users’ funds.
The associated balanceAdded event and corresponding emit statement should, therefore, also be removed.

(4) The closureDone event is not implemented anywhere in the contract, and so it’s redundant code.

(5) 

@jon_m

We should not mark the withdraw function as payable , because it does not need to receive ether from an external address in order to update the contract balance. If we made it payable , then, if the caller sent an ether value to the withdraw function by mistake, as well as calling it with a withdrawal amount, then this ether would be added to the contract balance and potentially lost to the user, because there is no accounting adjustment made to the mapping in the withdraw function for such a deposit (only for withdrawals). However, the function is much securer if we leave it as non-payable, because then, if the caller made the same mistake, the function would revert, and the ether would not be sent.

Additional information

@jon_m

A function only needs to be marked payable when it receives ether from an external address to add to the contract address balance . It might help to think of payable as the code that enables msg.value to be added to the contract address balance, without us having to add any further code for this. In the deposit function, this happens automatically because we have marked the function as payable . The line balance[msg.sender] += msg.value; then adds the same value to the individual user’s balance in the mapping, in order to keep a track of their share of the total funds held in the contract.

In contrast, the withdraw function is deducting ether from the contract balance and sending it to an external address. It does this using the address member transfer (which is like a method called on a payable address)…

<address payable>.transfer(uint256 amount);

(6) By adding the onlyOwner modifier to the withdraw() function, only the contract owner can withdraw their funds. But the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, don’t you think?
On this note, you should change the name of your deposit() function. At the moment it is depositToOwner(), which isn’t true. The function caller deposits funds to their own individual balance (not to the contract owner’s).

(7) 

@_Vincent

require(_recipient != owner, "Don't transfer money to yourself");

Any address with funds deposited in the contract can make an internal transfer, so we want this require statement to prevent the function caller from transferring funds to themselves. But your condition doesn’t do that; it prevents any function caller from transferring funds to the contract owner.

(8)  If you want to end up with a complete contract, then you are missing the transfer event from the Events Assignment. I think you may have missed it. The task was to add code, so that each successful transfer logs an event with appropriate data.

[jon_m]

Nice solution @skawal22

You have added all of the additional lines of code needed to solve the problem with the withdraw function.

Now have a look at this post which explains an important security modification that should be made to the order of the statements within your withdraw function body.

Just let me know if you have any questions

[skawal22]

@jon_m, Thanks again. And i appreciate your suggestion. I read the link post and now understand why you added suggestions there.

[_Vincent]

Hi @jon_m,
thanks for your steps.

Tried again and it works with some adjustments :
Step 5 resolved but removing government instance part (not necessary in my case, because this training happens before using governmentinstance in ETH courses).

Got issue with withdrawal and send transfer operation, but it’s logic and the issue coming from modifier onlyOwner.
This is not an issue, but we can’t withdrawal if we are not the owner, and we can’t perform a “reverse” transfer, cause to onlyOwner modifier.

[_Vincent]

Thanks again for your time @jon_m,
some feedbacks :
Ownable (3) I set the constructor with “payable”, because without that, we can’t deploy the contract with a value in “Value” fields. I notified it during my first tests.

Bank (6) Hum… it’s my new bank concept : only Owner can withdraw funds , you right ! I checked that, in my previous post with my tests

Thanks for all your taughts, missing notions are now acquired

[kasunkv]

My solution for the assignment.

pragma solidity 0.8.9;

contract Bank {
    mapping(address => uint) balance;
    address owner;
    event DepositDone(uint amount, address indexed depositedTo);
    event TransferDone(uint amount, uint balance, address indexed transferedTo);
    event WithdrawDone(uint amount, uint balance, address indexed withdrawnTo);
    
    modifier onlyOwner {
        require(msg.sender == owner, "You are not the owner");
        _;
    }
    
    constructor() {
        owner = msg.sender;
    }
    
    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, "Insufficient balance");
        require(msg.sender != _recipient, "Don't send money to yourself.");
        
        balance[msg.sender] -= _amount;
        balance[_recipient] += _amount;
        
        payable(_recipient).transfer(_amount);
        
        emit TransferDone(_amount, balance[msg.sender], _recipient);
    }
    
    function withdraw(uint _amount) public {
        require(balance[msg.sender] >= _amount, "Insufficient balance");
        
        balance[msg.sender] -= _amount;
        
        payable(msg.sender).transfer(_amount);
        
        emit WithdrawDone(_amount, balance[msg.sender], msg.sender);
    }
}

[giomiri]

pragma solidity 0.7.5;

contract Bank{
    
    mapping(address => uint) balance;
    
    address owner;
    
    event depositeDone(uint amount, address indexed depositedTo);
    event withdrawDone(uint amount, address indexed withdraw);

    
    modifier onlyOwner {
        msg.sender == owner;
        _;
    }
    
    constructor(){
        owner = msg.sender;
    }
    
    function deposit() public payable returns(uint){
        balance[msg.sender] += msg.value;
        emit depositeDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public{
        require(amount <= balance[msg.sender], "You haven't sufficient balance");
        msg.sender.transfer(amount);
        emit withdrawDone(amount, msg.sender);
        balance[msg.sender] -= amount;
    }
    
    function getBalance() public view returns(uint){
        return balance[msg.sender];
    }
    
    function transferTo(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't send 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;
    }
}

[jon_m]

Hi @_Vincent,

@_Vincent

Step 5 resolved but removing government instance part (not necessary in my case, because this training happens before using governmentinstance in ETH courses)

Yes, it will be easier to get it all working first without the extra complication of the Government instance and external function call. You can add this in afterwards.

@_Vincent

Got issue with withdrawal […] but it’s logic and the issue coming from modifier onlyOwner.
This is not an issue, but we can’t withdrawal if we are not the owner […] cause to onlyOwner modifier.

You should remove onlyOwner from the withdraw function header for the following reason …

@jon_m

(6) By adding the onlyOwner modifier to the withdraw() function, only the contract owner can withdraw their funds. But the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, don’t you think?
On this note, you should change the name of your deposit() function. At the moment it is depositToOwner(), which isn’t true. The function caller deposits funds to their own individual balance (not to the contract owner’s).

@_Vincent

Got issue with […] send transfer operation …

Do you mean the transfer of ether out of the contract to the external address, which happens as part of the withdraw transaction, i.e. msg.sender.transfer(amount) ?

@_Vincent

… and we can’t perform a “reverse” transfer, cause to onlyOwner modifier.

By “reverse” transfer do you mean step 15? …

@jon_m

15. Call transfer() function with Address B showing in Account field, and 2 arguments: recipient Address A, amount 1000000000000000000

There is no reason why this shouldn’t work, if your first transfer now works in step 5. The onlyOwner modifier isn’t applied to the transfer() function. Make sure you have Address A in the recipient argument field, and Address B showing in the Account field, before calling the function. Or is the issue something else?

[_Vincent]

Hello @jon_m,

@jon_m

Do you mean the transfer of ether out of the contract to the external address, which happens as part of the withdraw transaction, i.e. msg.sender.transfer(amount) ?

Yes, it was not possible for I doing a transfer as a non Owner, but logic regarding the onlyOwner modifier.

@jon_m

By “reverse” transfer do you mean step 15? …

Exactly, even it’s not the right term to use “reverse”.

@jon_m

There is no reason why this shouldn’t work, if your first transfer now works in step 5. The onlyOwner modifier isn’t applied to the transfer() function. Make sure you have Address A in the recipient argument field, and Address B showing in the Account field, before calling the function. Or is the issue something else?

Issue something else finally, but to be honest I don’t remember cause I’m working on the final project for ETH 101.
Sure, it was a little trick :).

Many thanks Jon

[jon_m]

@_Vincent

Ownable (3) I set the constructor with “payable”, because without that, we can’t deploy the contract with a value in “Value” fields. I notified it during my first tests.

The idea isn’t for the owner to send ether to the contract on deployment. If you do that, you will also need to include a line of code in the constructor which increases the owner’s individual balance in the mapping by the same amount, otherwise the owner won’t be able to withdraw those funds (unless they destroy the contract) or transfer them internally to another user. We have the deposit() function for all users (including the owner) to deposit ether in the contract. You should use this for deposits, because it contains all of the correct functionality for this to be done correctly and securely.

So, you can deploy the contract with Address A (which will be the owner). Then Address A can immediately call the deposit() function to deposit the same amount of ether you were previously depositing via the constructor.

@_Vincent

Bank (6) Hum… it’s my new bank concept : only Owner can withdraw funds , you right ! I checked that, in my previous post with my tests

 Ahhh … OK … so, does this now clear up the following issue you raised in your first post?

@_Vincent

Got issue with withdrawal and send transfer operation, but it’s logic and the issue coming from modifier onlyOwner.
This is not an issue, but we can’t withdrawal if we are not the owner […] cause to onlyOwner modifier.

[_Vincent]

Thanks for first tip about preventing to deploy with an Ether value and, instead of use deposit() function.

All is right now

Thanks @jon_m.

[jon_m]

Hi @kasunkv,

You have added both of the essential lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk …

1. check inputs (require statements)
2. effects (update the contract state)
3. external interactions

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external address…

payable(msg.sender).transfer(amount);

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. You’ll learn about the type of attack this prevents, and how it does it, in the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security

You are also right that it’s not necessary to return the updated balance, and so returns(uint) can be removed from the function header. It is useful information to return, but we do already have the getBalance function which can be called separately to consult the individual account holder’s updated balance after the withdrawal.

Your withdraw event and corresponding emit statement are both correct and well coded. The emit statement is in the correct position in the withdraw() function body and will log appropriate data when the withdraw() function is successfully executed

---

The transfer() function was initially implemented to perform internal transfers between two users, without any ether actually leaving the contract. In this scenario, the total contract balance remains the same. Only the sender’s and the recipient’s individual balances in the mapping need to be adjusted to reflect a change in each user’s share of the total contract balance. The sender’s share (individual balance) is reduced, and the recipient’s share is increased by the same amount …

@kasunkv

balance[msg.sender] -= _amount;
balance[_recipient] += _amount;

However, by including …

@kasunkv

payable(_recipient).transfer(_amount);

… your transfer() function does transfer ether out of the contract, to the recipient’s external address. In this case, only the sender’s individual balance in the mapping should be adjusted, because the recipient’s share of the contract balance shouldn’t be affected by the increase to their external address balance. In this respect, your transfer() function performs similar operations to the withdraw() function. Both reduce the total contract balance by the transaction amount. The only difference is that calling your transfer() function increases the recipient’s external address balance by the transaction amount, whereas calling the withdraw() function increases the caller’s own external address balance by this amount.

So, to perform an internal transfer within the contract, you need to remove …

@kasunkv

payable(_recipient).transfer(_amount);

And to perform an external transfer, you need to remove…

@kasunkv

balance[_recipient] += _amount;

Your transfer event and corresponding emit statement are both correct and well coded. The emit statement is in the correct position in the transfer() function body and will log appropriate data when the transfer() function is successfully executed. However, whereas deposit and withdraw transactions only involve one user address, a transfer involves two: the sender and the recipient. So, I think you can make two improvements to your transfer event:

1. Log both addresses (the sender’s as well as the recipient’s)
2. Make it clear whose new balance is being logged (the sender’s not the recipient’s). You can do this easily by changing the parameter name e.g. from balance to senderBalance

Let me know if anything is unclear, or if you have any questions about any of these points

[jon_m]

Hi @giomiri,

You have added both of the essential lines of code needed to solve the problem with the withdraw function

Your withdrawDone event and corresponding emit statement are also both correct and well coded, and the emit statement will log relevant information when the function has executed successfully. However, events should only be emitted once all of the associated operations have been completed. This can only be assured if the emit statement is positioned at the end of the function body (but before the return statement, if there is one).

You are right that it’s not necessary to return the updated balance, and so returns(uint) can be removed from the function header. It is useful information to return, but we do already have the getBalance function which can be called separately to consult the individual account holder’s updated balance after the withdrawal.

@giomiri

modifier onlyOwner {
    msg.sender == owner;
    _;
}

Can you see what’s missing here? … as it is, this modifier won’t have any effect.

Also, have a look at this post which explains an important security modification that should be made to the order of the statements within your withdraw function body.

Just let me know if you have any questions

[skplunkerin]

contract Bank {

    // abbreviated...

    event withdrawn(uint _amount, address _toAddress);
    
    function Withdraw(uint _amount) public returns(uint){
        require(balance[msg.sender] >= _amount, "not enough funds");
        // msg.sender.transfer() doesn't work on normal address types
        // need to make msg.sender to address payable type:
        address payable sender = payable(msg.sender);
        
        // Transfer to the sender:
        // NOTE: if transfer fails, it will refend just like with require()
        sender.transfer(_amount);
        
        uint oldBalance = balance[msg.sender];
        balance[msg.sender] -= _amount;
        assert(balance[msg.sender] == oldBalance - _amount);
        
        emit withdrawn(_amount, sender);
        return balance[msg.sender];
    }
}

[Sil]

    function withdraw(uint amount) public returns(uint){
        uint oldAmount = balance[msg.sender];
        uint newAmount;
        require(amount <= oldAmount);
        msg.sender.transfer(amount);
        newAmount = balance[msg.sender] - oldAmount;
        balance[msg.sender] = newAmount;
        return newAmount;

        assert(balance[msg.sender] == oldAmount - amount);
    }

I think I could compile it more, but it does the job. Any tips?