Transfer Assignment

[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?

[jon_m]

Nice solution @skplunkerin

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.

[jon_m]

Hi @Sil,

Good to see you back here in forum!

Your require statement is correctly coded, but it should be placed at the very beginning of your function body. This to maximise the amount of gas refunded if require() fails. If this happens, any gas consumed executing code up to and including the require statement will not be refunded. Placing require() at the top of the function body also means that, in the condition, you will have to reference the existing balance with balance[msg.sender] instead of oldAmount.

If you deploy your contract, make a deposit of, say, 3 Ether, and then call your withdraw function with an amount argument of 2000000000000000000 wei (2 Ether), you will notice that the caller’s external address receives the requested withdrawal amount, but when you call getBalance() their balance in the mapping has been reduced to zero! This means that the user cannot withdraw or transfer their remaining 1 Ether. Whenever a user calls the withdraw() function, your code reduces their balance in the mapping to zero, irrespective of the actual amount withdrawn from the contract…

@Sil

newAmount = balance[msg.sender] - oldAmount;

At this point in the control flow  balance[msg.sender] is the same value as oldAmount. I think part of what may have led to this error is the fact that you are using the names oldAmount and newAmount to refer to balances, and not to amounts of Ether withdrawn from those balances.

@Sil

uint newAmount;
... ... ...
newAmount = balance[msg.sender] - oldAmount;
balance[msg.sender] = newAmount;

You’ve used three lines of code, here, where you can achieve the same thing in just one.
The second two lines can be condensed into…

balance[msg.sender] = balance[msg.sender] - oldAmount;

… which makes your newAmount variable declaration redundant; and based on my previous comment you will also need to modify the right-hand side of this statement, so that the new balance isn’t always set to zero.

Your assert statement is unreachable, and the compiler will have generated an orange/yellow warning notifying you of this. This is because a return statement is always the last statement to be executed, and you have placed yours before the assert statement.

Once you have modified your code for the above points, take a look at this post which explains an important security modification that should be made to the order of other statements within your withdraw function body.

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

[skplunkerin]

Thank you Jon! I have added notes about this and will be altering my code to reflect this.

[angnimasherpa]

1) Make sure that the user can not withdraw more than their balance

function withdraw(uint amount) public returns (uint){
    require(balance[msg.sender] >= amount, "Error: Balance not enough!");
    msg.sender.transfer(amount);
    return balance[msg.sender];
}

2) Correctly adjust the balance of the user after a withdrawal

function withdraw(uint amount) public returns (uint){
    require(balance[msg.sender] >= amount, "Error: Balance not enough!");
    balance[msg.sender] -= amount;
    msg.sender.transfer(amount);
    return balance[msg.sender];
}

[michael356]

@jon_m

This particular address member is a special type of function. It is pre-defined within Solidity’s syntax, and is similar to a method in JavaScript. This is the syntax…

<address payable>.transfer(uint amount)

Hi Jon:

Thank you !
I almost understand it , because it didn’t call the transfer()function.

Blockquote

This particular address member is a special type of function. It is pre-defined within Solidity’s syntax, and is similar to a method in JavaScript. This is the syntax…

<address payable>.transfer(uint amount)

Blockquote

Could help to give me some article about this function (.transfer(uint amount))

[jon_m]

Hi @angnimasherpa,

Your number 2 solution includes 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 …

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…

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

---

Your number 1 code, without the line  balance[msg.sender] -= amount;  should never be deployed, because it includes a serious bug: the caller’s balance in the mapping is not reduced, and so they will be able to make repeat withdrawals up to the total amount of ether held in the contract (the sum of all of the individual account holders’ balances), effectively allowing them to steal other users’ funds.

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.

Let me know if you have any questions.

[jon_m]

Hi @michael356,

@michael356

because it didn’t call the transfer()function.

Exactly

@michael356

Could help to give me some article about this function (.transfer(uint amount))

I’ve tried to find a suitable article, but they are either much more technical than my own explanation, out-of-date, or go into a lot of detail comparing 3 address methods: call, send, and transfer. Honestly, I think you should try to understand and practise with my explanation, because I’ve only included the basic information that you need to understand at this stage. Also, here is some further information about our code, which will hopefully help your understanding…

To transfer ether from an external address to the contract: the deposit() function

• Receiving function must be marked payable
• Sending address sends a wei value: this is done in Remix by entering a number in the Value field and selecting the uint of currency that number represents (wei, gwei, finney or ether).
• The amount of ether deposited in the contract is automatically added to the contract balance.
• If you add the following getter to your contract, you can call it whenever you want to check what the current contract balance is…

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

• We keep a record of each individual user’s share of the total contract balance in a mapping. Each individual user’s share (or balance) is mapped to their external Ethereum address. So when a user calls the getBalance() function, this returns their current share of the total contract balance. The sum of all of the individual user balances stored in the mapping should always be equal to the contract balance. This is ensured by always updating the mapping whenever ether is transferred either into or out of the contract. When ether is transferred into the contract the individual user balance is updated with…

      balance[msg.sender] += msg.value;
/*
    msg.value references the ether value sent to the payable function from
    the external address and automatically added to the contract balance
*/ 

To transfer ether out of the contract to an external address: the withdraw() function

• Sending function is not marked payable
• The function caller’s individual share of the contract balance is updated with…

      balance[msg.sender] -= amount;
/*
    "amount" references a parameter value input into the function by the caller
     and which is the amount of wei requested to be withdrawn from the contract
*/

• The transfer address method is used to transfer ether (an amount in uints of wei) from the contract balance to an external address balance…

      <address payable>.transfer(uint amount);
/*           (A)                     (B)

    (A) External receiving address
        msg.sender if the receiving address is the caller of the function
    (B) A uint value which is the amount of wei to be transferred out 
        (withdrawn) from the contract. This can reference a parameter  
        value input into the function by the caller.
*/

• The amount of ether withdrawn from the contract is automatically added to the external address balance.

The transfer() function

• No ether enters the contract, so this function it isn’t marked payable.
• And no ether leaves the contract, so we don’t need the transfer address method either.
• What this function does is reduce the sender’s individual share of the contract balance, and increase the recipient’s individual share of the contract balance by the same amount. This is effectively a reallocation of the ether held in the contract, and neither the contract balance nor any external address balance changes.
• The function caller’s individual share of the contract balance is updated (reduced) with…

      balance[msg.sender] -= amount;

• And the recipient’s individual share of the contract balance is updated (increased) with…

      balance[recipient] += amount;

To help with your understanding of what the code in the different functions is actually doing, I also recommend you experiment and test your Bank contract with the steps listed in this post.

[angnimasherpa]

Thank you for the review and response @jon_m.
In the Question no.1 I excluded the part to deduct and only answered as per the question asked and that was to create a function to not let user withdraw more than their balance. So, I resolved it all along with the possibility of re-entrency attack in Question no.2.

Yes, I did have a couple of months of prior experience and research in solidity before deciding to take the course in Moralis to broden my knowledge on solidity, security, and blockchain overall.

Also, I just got into the re-entrency attack part as well today. It’s getting interesting!

I do have a question, say that the “transfer” function failed after deducting user’s balance. The whole transaction gets reverted and the users’ balance returns to the previous state, right?
I saw the code in re-entrency video on the academy which had the following code:

uint toTransfer = balance[msg.sender];
balance[msg.sender] = 0;
bool success = msg.sender.send(toTransfer);
if(!success) {
    balance[msg.sender] = toTransfer;
}

If the “send/transfer” part fails and reverts the transaction. There’s no need for further code, isn’t it?

[Sil]

Thanks a lot. Took some time off, but just can’t let go of programming on Ethereum.

Really appreciate all the tips & tricks. You’ll hear a lot from me going forward.

[jon_m]

Hi @angnimasherpa,

@angnimasherpa

In the Question no.1 I excluded the part to deduct and only answered as per the question asked and that was to create a function to not let user withdraw more than their balance.

But to prevent the user from withdrawing more than their balance whenever they call the withdraw function (and not just the first time they call it), we need to add both lines: the require statement and the individual balance adjustment. This is because, unless the user’s individual balance in the mapping is reduced to reflect the withdrawal, the require statement won’t check the user’s next withdrawal amount against their correct balance.

@angnimasherpa

So, I resolved it all along with the possibility of re-entrency attack in Question no.2.

… which is the correct solution to the problem

@angnimasherpa

Yes, I did have a couple of months of prior experience and research in solidity before deciding to take the course in Moralis to broden my knowledge on solidity, security, and blockchain overall.

Also, I just got into the re-entrency attack part as well today. It’s getting interesting!

That’s great! I’m sure you will learn a lot from these courses

@angnimasherpa

I do have a question, say that the “transfer” function failed after deducting user’s balance. The whole transaction gets reverted and the users’ balance returns to the previous state, right?

Correct… If the transfer method fails, revert is triggered automatically, and so no additional error handling is required. The send method effectively performs the same operation as transfer but, in the event of failure, instead of revert being triggered, send returns false. So, that’s why with send we need to add the additional code to reset the individual user balance in the mapping if an error occurs…

@angnimasherpa

bool success = msg.sender.send(toTransfer);
if(!success) {
    balance[msg.sender] = toTransfer;
}

… although I think a better way to code this is with a require statement, because this will perform revert and avoid the need to explicitly code the reset of the individual user balance e.g.

bool success = msg.sender.send(toTransfer);
require(success, "Transfer failed");

// or

require(msg.sender.send(toTransfer), "Transfer failed");

In fact, the latest advice is not to use the address members transfer() or send() . The fixed stipend of 2300 gas, which these methods forward to a calling contract’s fallback function, used to be enough gas for the fallback function to execute successfully, but not enough gas to allow a re-entrant call (thereby reducing the risk of re-entrancy attacks). But since the Istanbul hard fork changed the gas charges for certain operations, this fixed gas stipend is no longer guaranteed to be sufficient.

Instead of transfer() we can use the address member call() to perform the transfer of ether from the contract address balance to the external address …

(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Transfer failed");

Using call() would normally pose a greater risk of re-entrancy attacks than using transfer() or send() , because it will forward all of the remaining gas (up to the gas limit set by the caller) to the fallback function. However, implementing the Checks-Effects-Interactions Pattern will guard against this.

You will find more background information about this issue in the following article:

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Let me know if you have any further questions

[Joeyj007]

pragma solidity 0.7.5;

contract Bank {

  

     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 returns (uint){

        require(balance[msg.sender] >= amount;

        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, "Transferring funds to yourself is prohibited.");

       

        uint previousSenderBalance = balance[msg.sender];

            

        _transfer(msg.sender, recipient, amount);

                

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

        

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

        // event logs and further checks

    }

    

    function _transfer(address from, address to, uint amount) private {

                balance[from] -= amount;

                balance[to] += amount;

    }

}

[angnimasherpa]

@jon_m

Correct… If the transfer method fails, revert is triggered automatically, and so no additional error handling is required. The send method effectively performs the same operation as transfer but, in the event of failure, instead of revert being triggered, send returns false . So, that’s why with send we need to add the additional code to reset the individual user balance in the mapping if an error occurs…

Ahh So the send methods does not revert the transaction that’s why hmm. Thanks a lot for making it clear! I thought send and transfer were a similar functionality.

@jon_m

… although I think a better way to code this is with a require statement, because this will perform revert and avoid the need to explicitly code the reset of the individual user balance e.g.

True that. And I guess with the require statement. It will cost less gas than the one implemented there with conditions.

@jon_m

Instead of transfer() we can use the address member call() to perform the transfer of ether from the contract address balance to the external address …

Yes, I reached that part as well. And I’ll be implementing it starting my next contract. Also a question, is the call() function only working on transferring the ethers or we can transfer other tokens as well?
Through the use of openzeppelin can we write the following code for ERC20 token?

IERC20(tokenAddress).call{ value: amount }("")

Is the above function valid and functional or does it only work for the native token like ether/BNB/Matic?

Thank you!

[jon_m]

Hi @Joeyj007,

Your solution includes 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 …

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…

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

---

Be careful when you’re copy-and-pasting: you have a closing bracket after the deposit event declaration, which shouldn’t be there; it closes the Bank contract too early and throws a compiler error for the code which comes after it.

You are also missing a transfer event declaration. You’ve only included the emit statement, which will also throw a compiler error without an event declaration.

Let me know if you have any questions.

[jon_m]

Hi @angnimasherpa,

@angnimasherpa

Also a question, is the call() function only working on transferring the ethers or we can transfer other tokens as well?

ERC20 tokens only “exist” as balances within their respective ERC20 contracts. To transfer ownership of tokens we would need to call the function within the ERC20 contract which performs that operation. If we wanted another contract to call that function using the call method we would need to include a payload as the argument, instead of just a zero value (""). Without going into too much detail, the payload is an encoding of the function to be called and the arguments passed to it, expressed as a bytes value (data). If no payload is included, then the contract’s receive() function or fallback() function will be called, in that order of priority. It’s either the receive() function or fallback() function which is called by the transfer and send methods, neither of which include a payload argument.

Solidity Documentation for the Receive Ether function and the Fallback function
https://docs.soliditylang.org/en/latest/contracts.html#special-functions

The predefined syntax for the call method is …

<address>.call(bytes memory) returns (bool, bytes memory)

Unlike the transfer and send methods, sending an ether value is optional with the call method. With any external function call, we can send ether (in uints of wei) from one contract address to another by prepending the syntax {value: uint amount} to the parentheses containing the argument(s), or to the empty parentheses if there aren’t any arguments. This syntax is only for sending ether with an external function call.

This is a complex area of Solidity and smart contract programming, so don’t expect to fully understand everything I’ve just explained above straight away. Use this information to point you in the right direction for your own research.