Transfer Assignment

[farhan.qureshi]

pragma solidity 0.7.5;

contract Bank {

mapping(address => uint) balance;

address owner;

event deposited(uint amount, address despositedTo);

event transferred(uint amount, address beneficiary);

// seems like attribute

modifier onlyOwner {

    require(msg.sender == owner);

    _;  // run the function if above is true

}

modifier costs(uint price) {

    require(msg.value >= price);

    _;

}

constructor() {

    owner = msg.sender;

}

function deposit() public payable returns(uint) {

    balance[msg.sender] += msg.value;

    emit deposited(msg.value, msg.sender);

    return balance[msg.sender];

}



function withdraw(uint amount) public returns (uint) {

    // No One should be able to withdraw any amount from the smart contract balance

    // they should only be able to withdraw no more that their own smart contract balance

    // 1. get the balance of the sender's smart contract

    // 2. If the amount we are trying to withdraw is less than or equal to their balance, then fine

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

    msg.sender.transfer(amount);

    balance[msg.sender] -= amount;

    emit transferred(amount, msg.sender);

}

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

}

[jon_m]

Nice solution @farhan.qureshi

… and welcome to the forum! I hope you’ve been enjoying the course

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

Your withdraw event and corresponding emit statement are also both 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. However, as we also have a transfer() function in our Bank contract, I think it would be better not to name the withdraw event transferred, as this may cause confusion. What name did you give your transfer event for the Events Assignment? Did you do that assignment? It was in the Additional Solidity Concepts section of the course.

You have a costs modifier, but you haven’t used it in any of your functions. How would you do this?

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 you’ve included it in the function header, you should also include a return statement at the end of your function body. The compiler should have given you a yellow/orange warning for this.

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.

Don’t forget to post all of your assignment solutions here in the forum. It’s important to do this so we can give you some useful feedback. The other two assignments you seem to be missing are the Data Location Assignment and the Inheritance Reading Assignment.

Just let me know if you have any questions

[Jaka]

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.10;

contract EtherBank {

   

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

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

        balance[msg.sender] -= amount;

        payable(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);

        require(msg.sender != recipient);

        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;

    }

   

}

[ryan_n]

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");
        uint previousBalance = balance[msg.sender];
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        assert(balance[msg.sender] == previousBalance - 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]

Hi @Jaka,

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 …

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

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, their entitlement) 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

---

However, we should not mark the withdraw function as payable, because it doesn’t need to receive ether from an external address in order to update the contract balance. If we make it payable, then, if the caller sends 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: this is because the withdraw function does not make an equivalent accounting adjustment to the mapping 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 makes the same mistake, the function will revert, and the ether won’t be sent.

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

---

By the way, did you code a transfer event for the Events Assignment? This was in the Additional Solidity Concepts section of the course.

Let me know if you have any questions

[jon_m]

Hi @ryan_n,

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 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 balance…

balance[msg.sender] -= amount;

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

msg.sender.transfer(amount);

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, their entitlement) 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

The additional two lines you’ve included for the assert() statement are also well coded.

However, 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 at the end of the function body. The compiler should have given you a yellow/orange warning for this.

By the way, did you code a transfer event for the Events Assignment? This was in the Additional Solidity Concepts section of the course.

Let me know if you have any questions

[nbkeaton]

I used a require function to ensure the user isn’t withdrawing funds greater than their existing balance. I used an emit function to log the events of the deposit and withdraw functions. In each function the balance of the user is updated. Please let me know if any improvements can be made. Thank you!

pragma solidity 0.7.5;

    contract EthBank {

        mapping(address => uint) balance;

        event depositDone(uint amount, uint balance, address indexed depositedTo);

        function deposit() public payable returns (uint) { 
            balance[msg.sender] += msg.value; 
            emit depositDone(msg.value, balance[msg.sender], msg.sender);  //executing an event to log the amount of balance added and the address which it was deposited to
            //emit will continue to be used to log the addresses of those the deposited money into this EthBank
            return balance[msg.sender];
        }
        
        function withdraw(uint amount) public returns(uint) { 
            balance[msg.sender] -= amount;
            require(balance[msg.sender] >= amount, "Balance insufficient"); //using require function to make sure that the address/wallet balance is greater than the amount requested
            msg.sender.transfer(amount);
            emit depositDone(amount, balance[msg.sender], msg.sender);
        }

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

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

        function transfer(address recipient, uint amount) public {
            require(balance[msg.sender] >= amount, "Balance insufficient"); //using require function to make sure that the address/wallet balance is greater than the amount requested
            require(msg.sender != recipient, "Don't transfer money to yourself"); //requires that the sender is not also the receiver 
            uint previousSenderBalance = balance[msg.sender];

            _transfer(msg.sender, recipient, amount);

            emit depositDone(amount, balance[msg.sender], msg.sender);

            assert(balance[msg.sender] == previousSenderBalance - amount); //asserting that the output of the function should be the original balance minus the amount inputted 
            //and will return an error if its anything else
        }
        
    }

[jon_m]

Hi @nbkeaton,

You have added both of the essential lines of code needed to solve the problem with the withdraw function, and you have correctly adjusted the user’s individual Bank balance…

balance[msg.sender] -= amount;

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

msg.sender.transfer(amount);

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, their entitlement) 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.

Your require statement is correctly coded, but it should be placed at the very beginning of your function body, before the user’s individual Bank balance is adjusted. To reduce security risk in smart contracts, the following order of statements should be used…

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

Executing the require statement first will also maximise the amount of gas refunded if it fails and revert is triggered, because if this happens any gas consumed executing code up to and including the require statement will not be refunded.

---

Your event declaration and corresponding emit statements within the deposit() and withdraw() functions are accurately coded. Both emit statements are in the correct position within their respective function bodies, and they will log the data when these functions are successfully executed.

I can see that your use of the same event declaration to emit events for all 3 functions is a logical attempt to re-use code. However, each function performs a transaction which is different from the others in important ways, and by emitting all events with the name depositDone, we risk losing this disinction and causing confusion. Furthermore, the third event parameter name depositedTo incorrectly describes the sender’s address (msg.sender), which is emitted for this parameter in the transfer() function. The funds are transferred from msg.sender and “deposited to” the recipient.

Where the deposit() and withdraw() functions each execute a transaction that only involves 1 user address (the depositer or withdrawer), the transfer() function executes a transaction that involves 2 user addresses (the sender and the recipient). So, it would be useful to include the recipient’s address, as well as the sender’s address, within the data emitted for this particular transaction event.

So, I think we need to have three separate event declarations, each with a name that clearly describes the specific type of transaction it is associated with: (i) ETH transferred from an external address and deposited into the Bank contract, (ii) ETH withdrawn from the Bank contract and transferred to an external address, or (iii) an internal transfer of funds within the Bank contract between two users, which is effectively a reallocation of each user’s share of the Bank contract’s total ETH balance without any change to the actual Bank contract balance itself.

Including a user’s new balance within the event data is a good idea, but for the data emitted by the transfer() function, you need to make it clear whether this is the sender’s new balance or the recipient’s, because this isn’t clear from the parameter name balance.

Also, in the transfer() function it is probably better to place the emit statement after the assert statement.

---

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 you’ve included it in the function header, you should also include a return statement at the end of your function body. The compiler should have given you a yellow/orange warning for this.

---

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

[DNC]

pragma solidity 0.7.5;  

contract EtherBank {

    mapping(address => uint) balance;

    address owner;

    constructor() {

        owner = msg.sender;

    }

    modifier onlyOwner {

        require(msg.sender == owner);

        _;

    }

    modifier costs(uint price) {

        require(msg.value >= price);

        _;

    }

    event balanceAdded(uint _amount, address indexed _depositedTo);

    event transferDetails(uint _amount, address indexed _from, address indexed _to);

    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 {

        // we check that msg.sender personally has the amount to be withdrawn
        require(balance[msg.sender] >= _amount, "You don't have enough funds");

        msg.sender.transfer(_amount);

        // we update the balance of msg.sender after teh withdrawl
        balance[msg.sender] -= _amount;

    }

    function getBalance() public view returns(uint) {

        return balance[msg.sender];

    }

    function transfer(address _recipient, uint _amount) public {

        require(balance[msg.sender] >= _amount, "You do NOT have enough funds");

        require(msg.sender != _recipient, "You can NOT send money to yourself");

        uint previousSenderBalance = balance[msg.sender];

        _transfer(msg.sender, _recipient, _amount);

        emit transferDetails(_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;

    }

}

[Davelopa]

Hi! My solution:

function withdraw(uint amount) public returns(uint){

        // error handling

        require(balance[msg.sender] >= amount, "Not enough funds.");

        balance[msg.sender] -= amount;

        msg.sender.transfer(amount);

        return balance[msg.sender];

    }

[jon_m]

Hi @DNC,

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

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 a user can call separately to consult their own individual updated balance after a withdrawal.

Your comments explaining why you’ve added each line of code are also good, except that we should adjust the user’s individual balance in the mapping …

@DNC

/* we update the balance of msg.sender after teh withdrawl */
balance[msg.sender] -= _amount;

… before (and not after) actually transferring the funds out of the contract to the user’s external address…

@DNC

msg.sender.transfer(_amount);

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, their entitlement) 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.

To reduce security risk in smart contracts, the following order of statements should be used within functions …

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

---

Redundant code (which isn’t used) should be removed from the contract:

@DNC

modifier costs(uint price) {

        require(msg.value >= price);

        _;

    }

    event balanceAdded(uint _amount, address indexed _depositedTo);

The balanceAdded event is not needed in this contract because we have the deposit() function and the depositDone event instead.

However, you could implement the costs modifier. Where and how would you do that?

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

[jon_m]

An excellent solution @Davelopa

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…

balance[msg.sender] -= amount;

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

msg.sender.transfer(amount);

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, their entitlement) 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

Keep up the great coding!

[DNC]

Thanks again Jon.
Very useful info overall, especially the immediately actionable feedback about the order of statements within functions (to reduce the security risk); the same with the redundant code.
The costs modifier would be placed in the transfer function in order to pay for the transfer. I’m not quite clear how this works “under the hood”; yet

[jrobbins]

function withdraw(uint amount) public returns (uint){
    //check to ensure person calling the function has a sufficient balance
    require(balance[msg.sender] >= amount, "Insufficient Balace"); 
    //withdraw funds
    msg.sender.transfer(amount);  
    //update balance
    balance[msg.sender] -= amount;
    return balance[msg.sender];
}

[jon_m]

Hey Dan,

@DNC

The costs modifier would be placed in the transfer function in order to pay for the transfer.

Yes, that would work. You could actually use it in any of the functions where you wanted the contract to charge a price for performing the transaction.

@DNC

I’m not quite clear how this works “under the hood”; yet

You would need to place the costs modifier in the function header, and because it is checking that a minimum amount of wei has been transferred into the contract (i.e. the price) the function would also need to be marked payable (if it wasn’t already) e.g.

function transfer(address recipient, uint amount) public payable costs(100) {...}

In this example, a fixed price of 100 wei is hard-coded into the transfer() function. If the caller of the function doesn’t send a value of at least 100 wei, then the code in the function body will not be executed. If a valid amount of wei is sent, then this wei value will be added to the contract balance. This happens automatically because the function has been marked payable.

If we also add the following function to our contract, we can call it to check what the contract’s total ether balance is at any point in time. This should be the sum of all the individual user balances in the mapping, plus the total of all the wei amounts paid in fees to the contract.

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

Just let me know if you have any questions.

[jon_m]

Nice solution @jrobbins

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

[Davelopa]

Thanks for the feedback @jon_m!

This kind of information is very useful and is good to have the feedback right after learning and trying out new things!

Cheers

[DanDD]

Modified the withdraw routine:

    function withdraw(uint amount) public returns(uint){
        require(balance[msg.sender] >= amount, "You don't have enough to withdraw");
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
    }

[jon_m]

Hi @DanDD,

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

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 at the end of the function body. The compiler should have given you a yellow/orange warning for this.

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.

Let me know if you have any questions.

[Kunanon_Jarat]

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, "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;
    }
    
}