Transfer Assignment

jon_m · December 30, 2020, 2:52pm

Oh dear, that’s frustrating that it still isn’t working for you Let’s see if we can sort it…

I have copied and pasted the latest code you’ve posted into my copy of the contract in Remix and it compiles and executes no problem, so there isn’t any problem with your code. The only thing I had to change was the quotes around the error message “Insufficient funds” to "Insufficient funds". But you probably have the correct quotes in your code, anyway, and that’s probably just an error when copy-and-pasting into the forum post editor.

Before you deploy your contract, I assume that the compiler isn’t giving you any red errors, is that correct? You’ll proabably always get 1 orange warning about the SPDX license identifier not being provided, but you can ignore that one. But if you get any other orange warnings then we should probably look at what those are saying.

If the compiler errors or warnings aren’t the problem, then we need to think carefully about what’s causing the transaction error you are getting when trying to call the transfer function. The transaction is being reverted so the cause should be either one of the require statements, or the assert statement. It can’t be the 1st require statement, because then the error message would also include the message "Insufficient funds". Unfortunately, without putting a personalised error message as the 2nd argument in a require statement, the complete transaction error message will be exactly the same for require() and assert(). You could try adding another personalised error message to the 2nd require statement and then you will be able to see exactly which one is throwing (2nd require, or the assert).

If the 2nd require() is throwing, then it may be because you are calling the function from the same address that you are inputting as the recipient (1st argument). This can be an easy mistake to make because you need to select the recipient address from the account dropdown, copy it (to input as the 1st argument) but then remember to change the dropdown back to the address you are transferring from before clicking to call transfer() .

If you establish that assert() is throwing, instead, then we need to think again, because from the code you have posted, there is no way assert() should throw.

If none of my above suggestions resolve the problem, then post your complete contract, so I can test that.

Let me know how you get on

jon_m · December 30, 2020, 3:01pm

Excellent @ble4h!

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 where I explain an important security modification that should be made to the order of the statements within your withdraw function body.

Andro · December 30, 2020, 8:51pm

Thanks, I started doing that.

MrSeven · December 31, 2020, 7:07am

Here is my proposed solution:

function withdraw(uint amount) public {
    require(amount <= balance[msg.sender]);
    msg.sender.transfer(amount);
}

jon_m · December 31, 2020, 5:12pm

Hi @MrSeven,

The require statement you’ve added is correct but you are missing another 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?

The incomplete withdraw function given to you for the assignment also includes returns(uint) in the function header. This is not mandatory to include and the function can still operate effectively without returning a value. Was it your intention to remove it? If you add the returns(uint) to the function header, then you would also need to include a return statement in the function body.

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 about how to correct this, or if there is anything that you don’t understand.

deanb3 · December 31, 2020, 9:42pm

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

The above is my solution.

I’ll admit I added the last two lines in the function after seeing what others had done.

I’m thinking the first of those two lines, “balance[msg.sender] -+ amount;”, is needed to update the balance mapping to what is actually in the blockchain.

Also, I’d like to be sure my understanding is correct about something. It looks to me like the transfer function we are calling with “msg.sender.transfer(amount)” is not the transfer function that we have defined in this solidity contract because that function requires two arguments. The transfer function we are calling here is some native function to all contracts, right? It only requires the amount as the argument.

Caspian1234 · January 1, 2021, 9:49am

// withdraw money - check balance of sender to ensure adequate funds, and update balance after sending
function withdraw(uint amount) public returns(uint){
require (balance[msg.sender] >= amount, “insufficient balance in sender account”);
msg.sender.transfer(amount);
balance[msg.sender] = balance[msg.sender] - amount;
}

gkassaras · January 1, 2021, 3:58pm

@jon_m

Further to your comment I made some modifications to based on your comments.
Please see below:

// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;
// Simple bank code examaple

contract BankofEth {

   mapping (address => uint) private balances;
   
   address owner;

   constructor(){
       
       owner = msg.sender;
       
   }

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

   //////////////////////////////////////////////////////////////////////////////////
   // Adding events for logging and interacting better with dapps
   ///////////////////////////////////////////////////////////////////////////////////
   
   event logDepositMade(address indexed comeFromAddress, uint indexed amount);
   
   event logWithdrawMade(uint indexed amount);
   
   //////////////////////////////////////////////////////////////////////////////////
   // Private functions
   ///////////////////////////////////////////////////////////////////////////////////
   
   function _withdraw(uint _withdrawAmount) private {
       // Check enough balance available, otherwise just return balance
       require(_withdrawAmount <= balances[msg.sender]);
       balances[msg.sender] -= _withdrawAmount;
       msg.sender.transfer(_withdrawAmount);
       
   }
   
   //////////////////////////////////////////////////////////////////////////////////
   // 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  onlyOwner returns (uint) {
       emit logWithdrawMade(address(this).balance);
       return address(this).balance;
   }

   function withdraw(uint withdrawAmount) public returns (uint remainingBal) {
       _withdraw(withdrawAmount);
       return balances[msg.sender];
   }


}

It now looks more secure and splits the withdraw function to two functions. The interesting part I get while checking the Gas fees warnings is the one below. I wonder if an attacker can use this to run a buffer overflow attack.

See below:
screenshot

gkassaras · January 1, 2021, 4:17pm

Now uploading all my code to github https://github.com/supremeLame/Solidity_Require_Use/blob/main/README.md

gkassaras · January 1, 2021, 4:24pm

It is a lot of hard work required to do for security is there a guide about security logic on smart contracts we can look into?

I found this guide, also good for evaluating contracts for investments:

https://www.smartcontractaudits.com/audit-provider/zeppelin

DUUKA_SMASH · January 1, 2021, 11:45pm

So i didnt get it quite the way i thought of it based off what i understood for this project. However i got some help when i looked at the disccusion only to find out that i had two characters swapped the whole time. My “>=” was “=>”…which i swore was correct and refused to even try it the otherway. So much for me to learn. Hopefully ill look back on these days and laugh at my noobish ways. However learning this stuff is challenging.

function withdraw(uint amount) onlyOwner public returns (uint){
//msg.sender is an address
require(balance[msg.sender] >= amount);
msg.sender.transfer(amount);
}

DUUKA_SMASH · January 1, 2021, 11:57pm

So i see that the onlyOwner wasnt needed. That was my desperate attempt to keep each owner able to take what they put in. However i guess that the owner is actually the person who deployed the contract lol. Otherwise what helped clean up my withdraw function was the “balance[msg.sender] -= amount” because i believe that is the reason i get the revert when i didnt send as much and attempt to take more. Also it helped alot with having the return balance at the end of the body since it was annoying not seeing a difference in the balance after completing withdrawal.

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

someday ill get good at this.
DuuKa Duuka!!!

RCV · January 2, 2021, 4:23am

function withdraw(uint amount) public returns (uint){
        require(balance[msg.sender] >= amount, "Funds Withdrawn");
        msg.sender.transfer(amount);
    }

lucas_pace · January 3, 2021, 12:34am

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

Richard_Bond · January 3, 2021, 12:54am

While I was taking care of validating the transfer, I went ahead and added an event for good measure… hopefully not jumping ahead here

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

saf · January 3, 2021, 3:03am

function withdraw(unit amount public returns (uint)){
        require(balance.[msg.sender] >= amount, "You do not have enough funds");
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
        return balance[msg.sender]
    }

Stequal · January 3, 2021, 5:41pm

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

Depth-Hoar · January 3, 2021, 10:06pm

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

CC123 · January 4, 2021, 7:36am

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

CC123 · January 4, 2021, 7:37am

    balance[msg.sender] -= amount;
    return balance[msg.sender];