Transfer Assignment

[jon_m]

Hi @kHarsh,

The require statement and the return statement you’ve included are both correct But you are missing one other 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! The value which is returned from the function (the output) should be the reduced balance, but it will be the same as it was before the withdrawal. I’m sure you can see that this is another very serious bug! How would you correct this?

Once you’ve had a go at making the above modifications, have a look at this post where I explain another important security modification that should be made to the order of the statements within your withdraw function body.

Let us know if you have any questions, or if there is anything that you don’t understand.

[Aiisnotbad]

Here is my full contract…

pragma solidity 0.7.5;

contract MyContract{
    
    mapping(address => uint) balances;

    function Deposit() public payable returns (uint addressBalance) {
        require(msg.sender != address(this), "Cannot deposit from this contract to self.");
        balances[msg.sender] = balances[msg.sender] + msg.value;
        return balances[msg.sender];
    }
    
    function GetBalance() public returns(uint addressBalance) {
        return balances[msg.sender];
    }
    
    function Withdraw(uint weiAmount) public returns(uint addressBalance) {
        require(weiAmount <= balances[msg.sender], "Insufficient balance.");
        payable(msg.sender).transfer(weiAmount);
        balances[msg.sender] = balances[msg.sender] - weiAmount;
        return balances[msg.sender];
    }
}

[jon_m]

Hi @kHarsh,

A smart contract deployed on the Ethereum blockchain has it’s own Ethereum address and therefore an account balance in ether associated with that address. In this respect it’s no different to an address, and its associated account, used by a person to transact via smart contracts on the same blockchain. Both users and smart contracts are entities that transact with each other.

A smart contract’s initial balance will be zero. This balance will increase when it receives ether from another account address, and decrease when it pays ether to an account address. These other account addresses can be users’ wallet addresses or other smart contracts.

The address that initially deploys a smart contract could either:

• Pay ether into that smart contract on deployment via a constructor marked payable; or
• Pay ether into the smart contract via a payable function, once the contract has already been deployed.

If the value field is set to, say, 50 ether, then in both cases the ether balance associated with the address that deploys the contract or calls the payable function (let’s say the owner address) will reduce by 50 ether, and the contract balance will increase by the same amount. You can see this for yourself by adding either or both of the following pieces of code to a contract and deploying it with the value field set at 50 ether and/or calling the payable function fundContract with the value field set at 50 ether:

address owner;

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

// Allows you to view the current contract balance whenever called
function getContractBalance() public view returns(uint) {
    return account(this).balance;
} 

AND/OR

// Only include this state variable once if using both pieces of code together
address owner;
// Only include this function once if using both pieces of code together
function getContractBalance() public view returns(uint) {
    return account(this).balance;
} 

constructor() {            // Use the payable constructor if using 
     owner = msg.sender;   // both pieces of code together
}

modifier onlyOwner {
    require(msg.sender == owner, "Only the owner can call this function");
    _;
}

function fundContract() public payable onlyOwner returns(uint) {
    /* we would probably want to add additional functionality here,
       but for the purposes of this demonstration it's not necessary. */ 
}

It is important to note, that a smart contract could still be deployed without an initial ether balance and without receiving any subsequent funding other than the ether paid into the contract by its users. It all depends on what the contract is designed to be used for.

Any ether paid into the contract by its users will be added to the same contract balance as the one increased by funding from the owner address. This is why it is also important to keep track of the separate balances that originate from different addresses in a mapping. This should also be done for any initial or additional funding from the owner — I haven’t included this additional tracking in the code above in order to keep the example simple.

[MarkBravo]

I added a condition that test if enough funds are available, then update the balance from the mapping.

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

[Kraken]

Didnt realize that I had done the old solidity 101 course until now. Really liking the new version, cleaner and better examples and projects imo, though I loved to old one too. Here my solution, this one was pretty easy. Loving it!

function withdraw(uint amount)public returns(uint){
require(balance[msg.sender] >= amount, “Can’t withdraw more than your balance”);
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
return(amount);

}

[Timmy]

This is the solution I came up with. Let me know if my answer is correct or not thanks!

   function withdraw(uint amount) public returns (uint){
        require(amount <= balance[msg.sender]); // checks the balance of the sender
        balance[msg.sender] -= amount; // balance adjustment
        msg.sender.transfer(amount);

        return balance[msg.sender];
    }

[ArvidK]

@filip @jon_m

Hello! I am having trouble performing transactions in my transfer function. I am getting an error called; “VM error: invalid opcode. invalid opcode The execution might have thrown. Debug the transaction to get more information.”

My code:

mapping(address => uint) balance;

function deposit() public payable returns(uint){
    balance[msg.sender] += msg.value;
    return balance[msg.sender];
}

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

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

function transfer(address recipient, uint amount) public{
    require(balance[msg.sender] >= amount, "Insufficient funds");
    require(msg.sender != recipient);
    balance[msg.sender] -= amount;
    balance[recipient] += amount;
    
    interfaceLocation.addTransaction(msg.sender, recipient, amount);
    
    uint oldBalance = balance[msg.sender];
    
    assert(balance[msg.sender] == oldBalance - amount);
}

[Vivek20]

pragma solidity 0.7.5;

contract Bank {
    
    mapping (address => uint) balance;
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    event balanceTransfered(uint amount, address indexed transferedFrom, address indexed transferedTo);
    
    modifier onlyOwner {
        
        require(msg.sender == owner, "function restricted to 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 (amount <= balance[msg.sender], "insufficient balance");
        
        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{
        
        // check balance
        require(balance[msg.sender] >= amount, "Balance insufficient");
        require(msg.sender != recipient, "Transfer to self not allowed");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);

        assert(balance[msg.sender] == previousSenderBalance - amount);
        // event logs and further checks
        emit balanceTransfered(amount, msg.sender, recipient);
    }
    
    function _transfer(address from, address to, uint amount) private {
                
        balance[from] -= amount;
        balance[to] += amount;
        
    }
}

[hashmiabrar1]

function withdraw(uint amount) public returns(uint){
    require(balance[msg.sender]>=amount, "Insufficient funds to withdraw from contract");
    balance[msg.sender]-=amount;
        msg.sender.transfer(amount);
    
}

[ThreeHeadsUp]

this seems to work, only transfers to the one who put the transfer in and updates the balance:

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

[thecil]

Hi @ArvidK, hope you are well.

About that error, are you sure you are not missing the contract body and solidity version?

pragama solidity ... and contract contractName{ ... }.

Carlos Z

[thecil]

hey @hashmiabrar1, hope you are great.

You might wanna verify that function, it could drop an error when compiling the contract because you are missing the returned value.

Carlos Z

[jon_m]

Nice solution @Aiisnotbad

Just a couple of observations:

@Aiisnotbad

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

msg.sender is already a payable address by default, so you don’t need to convert it by wrapping it in  payable(). It doesn’t prevent your code from compiling, but it’s superfluous.

Have a look at this post where I explain an important security modification that should be made to the order of the statements within your withdraw function body.

Your getBalance function should also be marked view, because this function is only reading the state, and not modifying it. Remix should have notified you about this with a compiler warning.

[jon_m]

Hi @ArvidK,

You’re getting this error because your assert statement is throwing. This is because you’ve put the line…

@ArvidK

uint oldBalance = balance[msg.sender];

after you’ve reduced balance[msg.sender] for the transfer amount, but you need to assign the balance to the local variable oldBalance before it’s reduced. You should be able to now see why assert() is throwing, because in your function oldBalance is actually the new balance.

Also, the following line doesn’t make any sense in this contract, so you need to remove it.

@ArvidK

interfaceLocation.addTransaction(msg.sender, recipient, amount);

I hope that resolves your issue, but do let us know if anything is still unclear, or if you have any more questions

[jon_m]

This is excellent @Vivek20

You have included all of the additional lines of code needed to solve the problem with the withdraw function, and you’ve also put them in the correct order to reduce the security risk:

1. checks (require statement)
2. effects (update the contract state for reduction in balance)
3. interactions (perform the transfer of funds from smart contract address to wallet address).

Your Transfer event is also very well coded, and successfully emits the event upon transfer. Perhaps amountTransferred would be a better name than balanceTransferred for this event, because we can transfer any amount up to and including the balance — it doesn’t have to be the whole balance.

You’re making great progress! Keep it up

[Vivek20]

Hi Jon, thanks and you’re absolutely right amountTransferred sounds like a much better name since we’re transferring an amount and not a balance.

[gkassaras]

pragma solidity 0.7.5;


contract BankofEth {

    mapping (address => uint) private balances;


    //////////////////////////////////////////////////////////////////////////////////
    // Adding events for logging and interacting better with dapps
    ///////////////////////////////////////////////////////////////////////////////////
    
    event logUserAdded(uint indexed userId, uint indexed amountAdded, address indexed comeFromAddress);
    
    event logDepositMade(address indexed comeFromAddress, uint indexed amount);
    
    //////////////////////////////////////////////////////////////////////////////////
    // Public functions
    ///////////////////////////////////////////////////////////////////////////////////
    
    function getBalance() public view returns (uint) {
        return balances[msg.sender];
    }


    function deposit() public payable returns (uint) {
        balances[msg.sender] += msg.value;
        emit logDepositMade(msg.sender, msg.value);
        return balances[msg.sender];
    }

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

    function withdraw(uint withdrawAmount) public returns (uint remainingBal) {
        // Check enough balance available, otherwise just return balance
        if (withdrawAmount <= balances[msg.sender]) {
            balances[msg.sender] -= withdrawAmount;
            msg.sender.transfer(withdrawAmount);
        }
        return balances[msg.sender];
    }


}

[jon_m]

Great coding @gkassaras

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

1. check inputs
2. effects (update the contract state for reduction in balance)
3. interactions (perform the transfer of funds from smart contract address to wallet address).

Also, excellent use of  address(this).balance  to get the current total balance of ether held in the contract.

Just a couple of observations:

It’s better to use a require statement to check that the input meets the constraint, instead of an if statement. Apart from being cleaner and clearer code, require() will revert the transaction if it fails, meaning that the rest of the code in the function body will not be executed and the remaining gas will be refunded to the caller, meaning that the gas cost is lower.
require() also allows you return an error message so that the caller knows what’s happened.

@gkassaras

event logUserAdded(uint indexed userId, uint indexed amountAdded, address indexed comeFromAddress);

You haven’t emitted this event anywhere, and it doesn’t appear to be relevant in the context of your contract’s current functionality. It looks as though you want to log this when a user makes their initial deposit i.e. registers an account. But you aren’t currently storing a userId anywhere. Apart from this userId, all of the other event data is the same as logDepositMade.

[ArvidK]

@jon_m
thank you so much for the help, it is really appreciated. I have altered the code and made it as comparable to Filip’s contract as possible but I still can’t get around more errors. This time I am getting “transact to HelloWorld.transfer errored: VM error: revert. revert The transaction has been reverted to the initial state. Note: The called function should be payable if you send value and the value you send should be less than your current balance. Debug the transaction to get more information.”

my code:

function transfer(address recipient, uint amount) public{
require(balance[msg.sender] >= amount, “Insufficient funds”);
require(msg.sender != recipient);
uint oldBalance = balance[msg.sender];

    privateTransfer(msg.sender, recipient, amount);
    
    assert(balance[msg.sender] == oldBalance - amount);
}

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

[ble4h]

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

    return balance[msg.sender];
}