Transfer Assignment

jon_m · January 9, 2021, 10:54pm

Nice modifications @bjamRez
It’s looking great now, and I also like how detailed your comments are. That now gives you a great reference for whenever you need to go back and check things

MindFrac · January 10, 2021, 6:04am

Thanks, no I have not read that post yet. But if memory serves me, I think the order has something to do with preventing reentrancy attacks. If the balance is not updated first, an attacker could repeatedly withdraw tokens. I will read the post now, thx.

makinitnow · January 11, 2021, 1:30pm

Based on so far acquired knowledge I’ve tried:

function withdraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount);
uint previousBalance = balance[msg.sender];
msg.sender.transfer(amount);
assert(balance[msg.sender] == previousBalance - amount);
return(balance[msg.sender]);
}

But I’ve already read through the thread below:

This does not allow to witdraw any amount and throws error (- due to assert).
I forgot the last return(balance[msg.sender]);
is this: require(balance[msg.sender] >= amount); right or
require(balance[msg.sender] > amount); - the amount should be greater because of the gas?
is this assert(balance[msg.sender] == previousBalance - amount) right or
assert(balance[msg.sender] == previousBalance + amount); - becuse we send from and to msg.sender?

I’ve tried this solution I’ve found in the forum:

function withdraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount);
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
return(balance[msg.sender]);
}Preformatted text

evasilev · January 11, 2021, 4:31pm

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

Nando · January 11, 2021, 4:54pm

pragma solidity 0.7.5;

contract Bank {
mapping(address => uint) balance;
address owner;

event depositDone(uint amount, address depositedTo);

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){
    require(balance[msg.sender] >= amount);
    uint previousBalance = balance[msg.sender];
    msg.sender.transfer(amount);
    balance[msg.sender] = previousBalance - 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"); // we can add a message ! Cool
    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); // always false
    //event logs and further checks
}

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

}

jon_m · January 12, 2021, 9:45am

Exactly @MindFrac, you’ve got it!

jon_m · January 12, 2021, 10:42am

Hi @makinitnow,

I can see you’re really working hard at understanding what’s exactly going on here — your questions are very good

Your first attempt…

… is very good. It’s just missing the line…

balance[msg.sender] -= amount;

… before the external transfer to msg.sender. But you’ve correctly included this in your final version:

You need this additional line of code, otherwise the msg.sender’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced! I’m sure you can see the critical problem with that — the same msg.sender address could continuously withdraw not only their own funds but also the funds held in the contract by other addresses. They could effectively keep withdrawing funds until the contract balance is reduced to zero.

Including your assert statement, and the associated local variable previousBalance, is perfectly fine. However, without balance[msg.sender] -= amount; assert() will always fail because, after the withdrawal, previousBalance will still be equal to balance[msg.sender] which won’t have been reduced by the withdrawal amount.

The gas is paid by the msg.sender from the ether account associated with their own external address (the one they call the withdraw function from). It isn’t deducted from the contract’s balance. The require statement is only checking whether the address making the withdrawal has enough balance in their share of the total contract balance. The apportionment of the total contract balance amongst different “account holders” is recorded, stored and tracked in the mapping. But this is purely an accounting record. So, this is why the gas cost does not have to be factored into the condition in the require statement, and why using the comparison operator >= is correct.

Yes — this is correct, because the msg.sender’s balance left in the mapping after the withdrawal should be less than their balance in the mapping before the withdrawal, the difference being the withdrawal amount. This is the invariance we would want to check in an assert statement.

So, this is incorrect. When the withdraw function is called, ether is transferred from the contract address to the msg.sender address. As mentioned above, the mapping balances are purely for accounting purposes, and record/track the apportionment of the total contract address balance amongst different “account holders” with external addresses.

I hope this makes things clearer. But do let us know if you have any further questions

Ivan_Zg · January 12, 2021, 2:00pm

We can use modifier with revert statement, to check the balance:

    modifier balanceCheck(uint amount){
        if(balance[msg.sender] <= amount) {revert("Insufficient funds!!!");}
        _;
    }
    function withdraw(uint amount)public balanceCheck(amount){
        balance[msg.sender] -=amount;
        msg.sender.transfer(amount);
    }

Am I right to say, that if we want to check somewhat complicated logic, it is better to use revert() to throw an error instead of require()?

ArgenFrank · January 12, 2021, 8:41pm

function withdraw(uint amount) public returns(uint){
	require(balance[msg.sender] >= amount, "You can not withdraw more than you own");
	balance[msg.sender] -= amount;
    msg.sender.transfer(amount);
	
    return balance[msg.sender];
}

jon_m · January 12, 2021, 6:07pm

Excellent @ArgenFrank

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.

jon_m · January 12, 2021, 7:12pm

Great solution @Ivan_Zg

You have included all of the additional functionality needed to solve the problem with the withdraw function, and you also have the different lines of code in the correct order for optimal security:

check inputs (require statement, or if statement + revert, as you have chosen to use instead)
effects (update the contract state for reduction in balance)
external interactions (perform the transfer of funds from the smart contract address to the external wallet address).

The original withdraw function header also included returns(uint) . This is not mandatory to include, and the function can still operate effectively without returning a value. But you may want to consider what additional line of code you would need to include in the function body if your solution did return a value.

The first thing to realise is that the require statement also has the revert() functionality built-in, and so you are still using revert() whether you use either require() or if() revert();
You are absolutely right that if, instead of a simple single condition, you need to employ a more complex conditional logic, then this would certainly be when you may want to consider whether using revert() separately, rather than built-in to require(), would be more appropriate. However, in this assignment it isn’t necessary, and in my opinion, require() provides a clearer and more concise solution.

I think you will find the following article an interesting read:
https://medium.com/blockchannel/the-use-of-revert-assert-and-require-in-solidity-and-the-new-revert-opcode-in-the-evm-1a3a7990e06e
Just be aware that it was written over 3 years ago, and while it is still provides useful background information, there have been many more updates to Solidity since then, and it should be read in the context of when it was written.

Keep up the great work!

ArgenFrank · January 12, 2021, 8:40pm

thanks for the feedback jon! i will edit my post so nobody gets it wrong then

RCV · January 13, 2021, 2:04pm

function withdraw(uint amount) public returns(uint) {
       require(balance[msg.sender] >= amount, "Withdraw Not Completed");
       msg.sender.transfer(amount);
       balance[msg.sender] -= amount;
       return balance[msg.sender];
}

jon_m · January 14, 2021, 12:49pm

Hi @RCV,

You’ve included the correct line of code to reduce the balance in the mapping Now you just need to get it in the right place:

The return statement you’ve added is correct

SirGoat · January 14, 2021, 1:32pm

pragma solidity 0.7.5;

contract Bank{
address owner;

event deposited (uint amount, address indexed depositedTo);

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

modifier costs (uint price){
    require (msg.value >= price);
    _;
}

constructor (){
    owner == msg.sender;
}

mapping (address => uint) balance;

function deposit () public payable returns (uint) {
    balance[msg.sender] += msg.value; 
    //this is only for internal balance tracking, 
    //the blockchain keeps balances outside the smart contract
    emit deposited (msg.value, msg.sender);
    return balance[msg.sender];
}

function withdraw(uint amount) public returns (uint){
    // check msg.sender balance
    require(balance[msg.sender]>=amount, "Balance insufficient!");
    //transfers are denominated in Wei
    msg.sender.transfer(amount);
    //msg.sender is an address
    //address payable ToSend = 0x14723A09ACff6D2A60DcdF7aA4AFf308FDDC160C;
    // "transfer" has built in validation
    return (amount);
}


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

function transfer(address recipient, uint amount) public {
// check msg.sender balance
require(balance[msg.sender]>=amount, “Balance insufficient!”);
require(msg.sender!=recipient, “Cannot send funds 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 · January 14, 2021, 1:56pm

Hi @SirGoat,

The require statement you’ve added is correct but you are missing another very important line of code:

When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced! I’m sure you can see that this is another very serious bug! How would you correct this?

Once you’ve added a line of code for this, you need to also make sure that it is in the right place. I can see that you’ve already read this post which explains the importance of the order of the statements within the withdraw function body.

You are right to include a return statement in the function body, because the withdraw function header includes returns(uint). However, can you see that returning the balance after the amount has been withdrawn would probably be more useful than just returning the amount withdrawn (which is the input parameter anyway)?

Let us know if you have any questions about how to make these corrections, or if there is anything that you don’t understand

SirGoat · January 14, 2021, 2:29pm

hummmm…
thank you @jon_m!

what about this proposed solution for the balance update?!?

function withdraw(uint amount) public returns (uint){
    // check msg.sender balance
    require(balance[msg.sender]>=amount, "Balance insufficient!");
    //transfers are denominated in Wei
    msg.sender.transfer(amount);
    uint newBalance = balance[msg.sender] -= amount;
    balance[msg.sender]= newBalance;
    return (newBalance);
}

jiadong_han · January 14, 2021, 7:07pm

contract bank{

mapping(address => uint) balance;
address owner;
event Deposit (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 Deposit (msg.value, msg.sender);
return balance[msg.sender];

}

function transfer(address recipient, uint amount) public {
require(balance[msg.sender] >= amount);
require(msg.sender != recipient);
uint oldSenderBalance = balance[msg.sender];
_transfer(msg.sender, recipient, amount);
assert(balance[msg.sender] == oldSenderBalance - amount);
}

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

function withdraw(uint _tominus) public returns(uint){
uint oldSenderBalance = balance[owner];
require (oldSenderBalance >= _tominus, “Balance is not sufficient” );
balance[owner] -= _tominus;
msg.sender.transfer( _tominus);
assert(balance[owner] == oldSenderBalance - _tominus );
return balance[owner];
} }

okcrypto · January 15, 2021, 2:10am

function withdraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount, “insufficient Funds”);
msg.sender.transfer(amount);

Ignacio · January 15, 2021, 9:00pm

I have added the necessary require condition, also to keep easy track of the balances and address, I created the function get_currentAddrs

pragma solidity 0.7.5;

contract banking{
    
    mapping(address => uint) balance;   // maps an address to balance object, e.g. in python: balance["address"] = uint
    address owner;
    
    
    // modifier: restricts access to whatever meets conditions
    modifier onlyOwner {
        require(msg.sender == owner);
        _;   // underscore (;) means to run the function
    }
    
    modifier _cost_(uint gas_cost) {
        require(balance[msg.sender] >= gas_cost);
        _;
    }
    
    // create an event to be able to query data 
    event balanceAdded(uint amount, address depositedTo);
    event balanceTransfer(uint amount, address depositedFrom, address depositedTo);
    
    //constructor: constructs conditions for modifiers
    constructor() {
        owner = msg.sender;
    }

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

    // add balance to an address and update
    function deposit_balance() public payable returns(uint){
        balance[msg.sender] += msg.value; // this line is not needed anymore, payable functions act on the net
        emit balanceAdded(msg.value, msg.sender); // emit the event check the logs in the output
        return balance[msg.sender];
    }
    // withdraw function
    function withdraw_balance(uint amountx) public returns(uint){
        require(balance[msg.sender] >= amountx);
        msg.sender.transfer(amountx);
        balance[msg.sender] -= amountx;
        return balance[msg.sender];
    }
    
        
    // this function is PRIVATE!! can only be called from this contract,
    // substracts amount from sender and adds it to receipent
    function transfer_funds(address from, address to, uint amount) private{
        balance[from] -= amount;
        balance[to] += amount;
    }
    
    // 
    function transfer(address receipent, uint amount) public _cost_(50) {
        require(balance[msg.sender] >= amount, "balance isn't enough");   // verify that the sender has enough money
        require(receipent != msg.sender, "can't transfer to yourself");         // verify that the sender doesn't auto-transfer
        
        uint previous_balance = balance[msg.sender];
        
        // call the ohter function called: transfer_funds
        transfer_funds(msg.sender, receipent, amount);
        
        // assert: checks that conditions are met
        assert(balance[msg.sender]  == previous_balance - amount);
    }
}