Transfer Assignment

jon_m · March 20, 2022, 10:35pm

Sorry for the delay in giving you some feedback!

withdraw() function

(1) You should have found that your contract code didn’t compile, and so couldn’t be deployed…

Your pragma statement is declaring that the code should be compiled based on Solidity v0.8 syntax. That’s absolutely fine, but then you also need to bear in mind that this course material is based on v0.7 syntax. The compiler will always generate an error when your syntax needs to be changed for v0.8, and the error message will tell you why, and often what you should change it to. In this case, you will have got a red compiler error for this line of code…

In order to receive Ether, an address needs to be payable. Prior to Solidity v0.8, msg.sender is payable by default and so doesn’t need to be explicitly converted. However, from v0.8, msg.sender is non-payable by default, and so when it needs to refer to a payable address, it has to be explicitly converted. One of the ways to do this is using payable() e.g.

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

If you make this modification, you will notice that the compiler error disappears.

(2) The require statement you’ve added is correct, but your withdraw function is also missing an essential line of code …

If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the caller’s external address receives the requested withdrawal amount, but their balance in the mapping is not reduced! This means that they can make repeat withdrawals — each one no greater than their initial balance, which isn’t being reduced — up to the total amount of Ether held in the contract (the sum of all of the individual account holders’ balances). So, I’m sure you can see why this is a very serious bug!

payable(msg.sender).transfer(amount) transfers Ether from the contract address balance to the caller’s external address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total Ether balance. So, just as we increase a user’s individual balance when they deposit Ether in the contract, we need to do the opposite whenever they make a withdrawal.

(3) Notice that the withdraw function header also includes returns(uint) . This is not mandatory to include, and the function can still operate effectively without returning a value. But as it’s been included in the function header, you should also include a return statement in the function body. Once the red compiler error (described above) has been corrected, the compiler will still give you an orange warning about this issue.

(4) Once you’ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

Transfer Event (from the Events Assignment)

Your transfer event declaration is well coded, and the corresponding emit statement, which you’ve added to the transfer() function body, will log data when the transfer() function is successfully executed.

However, you are emitting the msg.sender address as the event’s to address parameter, and the recipient address as the event’s from address parameter. Can you see that you’ve got these the wrong way round? The position of each value in the emit statement must be the same as that of its corresponding parameter in the event declaration, in terms of both data type and name. If the data types don’t correspond, then the contract won’t compile, but if the names are mixed up, then the affected data will be logged against potentially misleading identifiers (which is what will happen with your solution).

But you have referenced amount in the correct position in your emit statement

Just one other observation …
In general, an emit statement is probably better placed after an assert() statement.

Let me know if anything is unclear, if you have any questions about any of these points, or if you need any further help to correct your solution code

jon_m · March 20, 2022, 11:41pm

Hi @kcoffey,

Correct

No …

The Ether is held in the contract address balance, so to perform the actual withdrawal we need to transfer an Ether value equivalent to the requested withdrawAmount from the contract address to the caller’s external address (msg.sender). We can do this with Solidity’s in-built transfer method …

<address payable>.transfer(uint amount)

So you are missing the line of code which was given to you in the assignment’s introductory video …

msg.sender.transfer(withdrawAmount);

This line of code …

… correctly reduces the withdrawer’s individual balance in the mapping by the withdrawal amount. These individual user balances in the mapping perform an internal accounting role, and record each user’s share of the contract’s total Ether balance. So, when your withdraw() function is called, the caller has their entitlement to make further withdrawals reduced by the requested withdrawal amount, but their external address doesn’t actually receive the equivalent amount of Ether from the contract!

Don’t get confused between the contract owner’s address and the contract address itself …

The contract address is the address the contract is deployed at. It is the address next to the deployed contract’s name below Deployed Contracts at the bottom of the Deploy & Run Transactions panel in Remix. You can copy this address to the clipboard by clicking on the adjacent icon.
In our contract, the contract owner’s address is the address the contract is initially deployed by — the address which is showing in the Account field near the top of the same Remix panel when you click the orange Deploy button.

Apart from this …

The require() statement and the return statement you’ve added to your withdraw() function are both correct
The LogWithdrawal event you are emitting is also a nice addition. It’s in the correct position within your withdraw() function body, and it will log relevant information when a call to the withdraw() function has executed successfully. The emit statement looks to be coded correctly. Can we also see the event declaration that it’s based on?

Once you’ve corrected your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

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

seybastiens · March 23, 2022, 8:16pm

My solution (before checking solutions) :

function withdraw (uint amount) public returns (uint) {
         require (balance[msg.sender] >= amount);
         uint previousBalanceOnSmartContract = balance[msg.sender] ;
        payable(msg.sender).transfer(amount); // here we transfer the money to msg.sender
        // (it means we transfer the money from the SC wallet)
        // So now we need to take it from balance of msg.sender on the SC (=balance[msg.sender]) so
        // that this address cannot use more balance than it has on this SC
        balance[msg.sender] -= amount; // here we substract from the
        // balance of msg.sender on the current smart contract this amount of money
        assert (balance[msg.sender] == previousBalanceOnSmartContract - amount) ; 
        return balance[msg.sender];
     }

Though I think in reality this solution is not good as we don’t take into account gas fees. With this first solution, gas fees would be on the SC wallet and not on the balance[msg.sender] so gas fees would end up drying the SC wallet.

So, not sure yet how to integrate gas fees, I would prefer a solution like this :

 int  CurrentGasPrice; // this value could be added when calling
 // the function or taken through an Oracle maybe.

 modifier cost (int CostInGas) {
        require(msg.value >CostInGas*CurrentGasPrice); // if = it's useless
        _;
    }

 function withdrawWithGasFess (int amount) public cost(1000) returns (uint) {
// We calculate (for example) that all the operations in this function will cost 1000 gas. Also 
// but that's the gas cost and we need to current gas price to check if msg.value is over gas price *1000
        require (balance[msg.sender] > amount); // if it's = it means no space for gas fees so useless to go on.
        uint previousBalanceOnSmartContract = balance[msg.sender] ;
        payable(msg.sender).transfer(amount); // here we transfer the money to msg.sender from
        // the SC wallet, but we use the SC wallet to pay for gas fees
        balance[msg.sender] -= amount + 1000*CurrentGasPrice; // Here we also withdraw
        // the estimation in gas fees that the transaction will be
        assert (balance[msg.sender] == previousBalanceOnSmartContract - amount - 1000*CurrentGasPrice) ; 
        return balance[msg.sender];
     }

jon_m · March 25, 2022, 4:04am

Hi @seybastiens,

Your original solution

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and the additional two lines you’ve included for the assert() statement are also well coded

The only improvement that can be made is to change the order of two of the statements in your withdraw function body …

The following withdrawal pattern (order of statements) should be used in order to reduce security risk:

Check inputs (require statements)
Effects (update the contract state)
External Interactions (e.g. transfer method)

It’s important to modify the contract state for the reduction in the individual user’s balance…

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

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra info. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one. But it’s good to be aware of and to start getting into good habits.

Just another couple of important points to clarify …

It’s important to understand that this line of code transfers Ether from the smart contract’s address balance to the external account balance of msg.sender. However, by itself, it doesn’t leave us with any record of the reduced share of the total contract balance which msg.sender is now entitled to. This is achieved by also reducing the withdrawer’s individual user balance in the mapping …

seybastiens:

// So now we need to take it from balance of msg.sender on the SC (=balance[msg.sender]) so
// that this address cannot use more balance than it has on this SC
balance[msg.sender] -= amount; // here we substract from the
// balance of msg.sender on the current smart contract this amount of money

So, it’s important to also understand that it’s the individual user balances stored in the mapping, which perform an internal accounting role, and track each user’s share of the contract’s total Ether balance as this changes over time.

Your second solution

We don’t need to take into account gas fees in our code. The gas fees for each transaction, whether a call to the deposit(), withdraw() or transfer() function, will always be paid by the calling address (the address sending the transaction) and deducted from their external address balance together with any Ether value they are also sending (which in our Bank contract only happens when calling the deposit function). But while the Ether value sent to the deposit() function is added to the contract address balance, the gas fees are not.

seybastiens:

 modifier cost (int CostInGas) {
    require(msg.value >CostInGas*CurrentGasPrice); // if = it's useless
    _;

[...]
// We calculate (for example) that all the operations in this function will cost 1000 gas. Also 
// but that's the gas cost and we need to current gas price to check if msg.value is over gas price *1000
}

So, this modifier doesn’t make any sense, and will always fail, because msg.value will always be zero for the withdraw() function. This is because no Ether value is ever sent to this function, and msg.value never includes gas fees. In fact, no Ether can ever be sent to the withdraw() function because it is not marked payable, and it is right that this restriction is enforced.

The gas fees are processed by the EVM according to the Ethereum protocol. Under proof-of-work, the gas fee for each transaction (transaction fee) is paid to whichever miner successfully mines the block it is included in. Under proof-of-stake it gets more complicated, with validators instead of miners, and a proportion of the gas fees being burnt.

So, when an address calls the withdraw() function, the gas fees for executing the whole transaction — including all of the low level operations associated with the withdraw function’s code — are deducted from that address’s account balance (showing in the Account field in the Remix panel). This includes the fees for executing the transfer method, which transfers an Ether value equivalent to the withdrawal amount (input into the function as the amount argument) from the contract address balance to the account balance of the external calling address (showing in the Account field in the Remix panel).

Only an external address, via a web3 client at the frontend, can be used to sign and send a transaction which incurs a gas fee. This transaction may trigger a smart contract to send/transfer funds (whether Ether or other tokens) to other smart contracts, other external addresses or, as happens with the withdraw() function, to itself. But at no point do smart contract addresses ever pay gas fees, and neither are gas fees ever added to smart contract address balances or stored within them.

So, this won’t happen and isn’t even an issue.

I hope that makes sense now, but just let me know if anything is still unclear, or if you have any further questions

menarf · March 26, 2022, 6:43pm

pragma solidity 0.7.5;

contract Bank {

    mapping(address => uint) balance;

    address owner;

    event depositDone(uint amount, 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"); //check that sender's balance is bigger than the amount to be withdrawn
        msg.sender.transfer(amount);
    }

    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;
        

    }

}

jon_m · March 26, 2022, 11:33pm

Hi @menarf,

The require statement you’ve added is correct, but your withdraw function is missing an essential line of code …

If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the caller’s external address receives the requested withdrawal amount, but their balance in the mapping is not reduced! This means that they can make repeat withdrawals — each one no greater than their initial balance, which isn’t being reduced — up to the total amount of Ether held in the contract (the sum of all of the individual account holders’ balances). So, I’m sure you can see why this is a very serious bug!

msg.sender.transfer(amount) transfers Ether from the contract address balance to the caller’s external address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total Ether balance. So, just as we increase a user’s individual balance when they deposit Ether in the contract, we need to do the opposite whenever they make a withdrawal.

Notice that the withdraw function header also includes returns(uint) . This is not mandatory to include, and the function can still operate effectively without returning a value. But if you do include it in the function header, you should also include a return statement in the function body. The compiler will have given you an orange warning about this.

Once you’ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

By the way, I think you may have missed out the Events Assignment, because you are missing a Transfer event, and a corresponding emit statement, in your contract. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.

Let me know if anything is unclear, if you have any questions, or if you need any more help with correcting your solution

seybastiens · March 28, 2022, 6:36pm

Ok well understood.
Thanks for the deep explanation.

menarf · March 28, 2022, 7:45pm

Hi @jon_m,

Thanks a lot for your reply.

I guess I didn’t think through the whole task very well. I realized it straight away when I saw Filip’s solution in the following video.

As for Events Assignment, you are right, I missed it completely

I added transfer event to my code and corrected withdrawal function as per your suggestions and Filip’s solution video.

pragma solidity 0.7.5;

contract Bank {

    mapping(address => uint) balance;

    address owner;

    event depositDone(uint amount, address indexed depositedTo);

    event amountTransferred (uint amount, address sender, address recipient);

    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"); 
        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); 

        emit amountTransferred(amount, msg.sender, recipient);

    }

    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount; 
        balance[to] += amount;
        

    }

}

jon_m · March 29, 2022, 1:10am

Nice modifications @menarf, … your whole contract’s looking great now!

The transfer event and corresponding emit statement you’ve added 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

Don’t worry! … As long as you carefully review and check your solution afterwards, in order to understand anything you missed or should be coding differently, you will make good progress. Obviously, there are often several alternative solutions, and we can help you to review and check these kinds of differences when you post your code here in the forum.

Thunarson7999 · March 29, 2022, 2:59pm

See below - add require(balance[msg.sender] >= amount,"Balance not sufficient"); and update balance with balance[msg.sender] -= amount;

pragma solidity 0.7.5;

contract Bank {

    constructor(){

        owner = msg.sender;

    }

    mapping(address => uint) balance;

    address owner;

    modifier onlyOwner {

        require(msg.sender == owner);

        _; //run the function

    }

    event depositDone(uint amount, address 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 returns (uint){

        require(balance[msg.sender]>=amount,"Balance not sufficient");

        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;

    }
}

jon_m · April 1, 2022, 5:03am

Great solution @Thunarson7999

You have added all of the 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:

Check inputs (require statements)
Effects (update the contract state)
External Interactions (e.g. transfer method)

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

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

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent 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

By the way, I think you may have missed out the Events Assignment, because you are missing a Transfer event, and a corresponding emit statement, in your contract. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.

Just let me know if you have any questions

Thunarson7999 · April 4, 2022, 7:36am

Thx @jon_m for your feedback, highly appreciated! Oh yes I forgot a withdraw event.
I’d probably add sth like this in the declaration

event withdrawalCompleted(address withdrawnTo);

and then emit the event in the function body

emit withdrawalCompleted(msg.sender);

JoriDev · April 4, 2022, 2:11pm

Here’s the latest version of my bank contract. I added a withdrawal function and a withdrawal event, along with some corrections after the previous assignment. Just for clarity I’ll post all of the code again.

pragma solidity 0.7.5;

contract Bank {

  mapping(address => uint) balance;

  address owner;

  event DepositDone(uint amount, address depositedTo);

  event WithdrawalDone(address withdrawnTo, uint newBalance)

  event TransferLog(address indexed sender, address indexed recipient, uint transferValue);

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

  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){
     //make sure the balance of the sender is sufficient
    require (balance[msg.sender] >= amount, "the amount is larger than your balance");
    
     balance[msg.sender] -= amount;
     msg.sender.transfer(amount);
     return balance[msg.sender];

     emit WithdrawalDone(msg.sender, balance[msg.sender]);  
  }

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

  function transfer(address recipient, uint amount) public {
    //check balance of msg.sender
    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);

    //event logs and further checks
    assert(balance[msg.sender] == previousSenderBalance - amount);
    emit TransferLog (msg.sender, recipient, amount);
  }

  function _transfer(address from, address to, uint amount) private {
    balance[from] -= amount;
    balance[to] += amount;
  }
    
}

mjbam93 · April 6, 2022, 11:56am

Hi,
i tried for a while now, but i cant even compile my code.
Then i looked up Filip’s solution and even his code doesnt work for me.

in line 18 it says:

ypeError: “send” and “transfer” are only available for objects of type “address payable”, not “address”.
–> payable_functions_solution.sol:18:9:
|
18 | msg.sender.transfer(amount);
| ^^^^^^^^^^^^^^^^^^^

is this outdated or so? i dont get what is actually wrong…

mcgrane5 · April 6, 2022, 11:59am

no you need to make the address of type payable. change the lin eof code to

payable(msg.sender).transfer(amount)

mjbam93 · April 6, 2022, 12:11pm

Awesome! Thanks a lot! It worked!

jon_m · April 6, 2022, 1:59pm

Hi @Thunarson7999,

This code is correct, but we would probably want to log the withdrawal amount as another parameter e.g.

event withdrawalCompleted(address withdrawnTo, unit amount);

emit withdrawalCompleted(msg.sender, amount);

By the way, where in the function body would you place the emit statement? Order/position is important, so it would be good to see what you think

jon_m · April 6, 2022, 2:48pm

Hi @JoriDev,

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

All of your statements are in the correct order within the withdraw() function body, except for the emit statement. Once you’ve resolved the compiler error by adding a semi colon to the end of your WithdrawalDone event you will get an orange compiler warning for the emit statement. The warning message tells you that this line of code is unreachable. This is because function execution stops after a return statement, so at the moment your WithdrawalDone event data won’t be emitted or logged. You can test this for yourself by calling the withdraw() function and then having a look at logs in the transaction receipt (the one with the green tick) in the Remix terminal at the bottom of the screen.

You are right that an emit statement should only be executed after the event itself has occurred. However, it needs to be placed before a return statement if there is one. And generally speaking, it’s probably better to place an emit statement after an assert statement if there is one, which is what you’ve done with your TransferLog emit statement in the transfer() function body.

Apart from its position, your WithdrawalDone event declaration and corresponding emit statement are well coded. But don’t you think it would be important to log the amount withdrawn as well as the user’s updated balance?

Your other statements are in the correct order within the withdraw() function body, in order to reduce security risk

Check inputs (require statements)
Effects (update the contract state)
External Interactions (e.g. transfer method)

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

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

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent 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!

Your other modifications related to the previous Events Assignmnet are also well done

Just let me know if you have any questions.

jon_m · April 6, 2022, 3:15pm

Hi @mjbam93,

You need to bear in mind that this course material is based on Solidity v0.7 syntax. It’s perfectly fine to use Solidity v0.8 and, in fact, it’s better to use this latest version. But if you do, you will need to make some occasional adjustments to your code for the one v0.8 syntax change which affects the code for this 101 course …

In order to receive Ether, an address needs to be payable . Prior to Solidity v0.8, msg.sender is payable by default and so doesn’t need to be explicitly converted. However, from v0.8, msg.sender is non-payable by default, and so when it needs to refer to a payable address, it has to be explicitly converted. One of the ways to do this is using payable() in the way that @mcgrane5 has already shown you …

You should find that the next time you get a compiler error due to this syntax change, you now know how to fix it. But just let either of us know if you have any more questions

JoriDev · April 8, 2022, 12:08pm

Well I had a question on your reply, but after reading it once again the answer was already in it.

You wrote:

And I was like okay, but why is the order of emit before return so important?

Then there was the light bulb moment, as you were telling earlier:

I’ll definitely keep this in mind while writing future contracts to prevent parts of my function executions from being unreachable. Thanks for clearing up this accidental error!