Transfer Assignment

Ethereum Smart Contract Programming 101
#1156

Hi @Giupi,

This certainly got me thinking and experimenting!!

@mcgrane5 & @thecil :  Do you agree with my conclusion, below? Or have I missed something?

Through a process of elimination, I’ve worked out that the line of code that is causing the transaction to revert in Remix (with the error message: out of gas) is the one setting the withdrawer’s individual balance in the mapping to zero …

However, as far as I could see, there was nothing wrong with this line of code. In fact, quite the opposite: you have correctly set the user’s individual balance in the mapping to zero before transferring their full Ether balance out of the contract to their external address.

I thought, maybe, one or two of the other lines of code in this function could be pushing the gas consumption over the gas limit (which can be adjusted in the Gas Limit field below the Account field in the Deploy & Run Transactions panel in Remix). But this isn’t the case, and no matter how much the gas limit is increased, this doesn’t seem to make any difference.

What’s more, I also couldn’t see any reason why setting balances[msg.sender] to 1 instead of zero should make any difference to the gas cost.

Out of interest, I tried changing …

balances[msg.sender] = 0;
// to
delete balances[msg.sender];

…which should produce the same effect. But this also triggers the out of gas error in Remix.

So, I tested your complete function (within a larger Bank contract) using Truffle and Ganache, instead of Remix … and it worked! I used Solidity compiler v.0.8.9. So, this suggests to me that there is most probably a bug in the current version of Remix.

Even the following function triggers the out of gas error in Remix. This is very strange and makes me even more convinced that there is some kind of bug causing this…

function setBalanceToZero() public {
    balances[msg.sender] = 0;
}

Once you’ve completed the section on using Truffle in the 201 course, you’ll be able to see that your withdrawAll() function works fine. However, I would suggest you also make the following amendments, which will also save a small amount of gas…

function withdrawAll() public /*returns (bool _success)*/ {
// Do you use this return value? If not, you should remove it.
    require(balances[msg.sender] > 0, "Insufficient funds");
    uint _amountToWithdraw = balances[msg.sender];
    balances[msg.sender] = 0;
    payable(msg.sender).transfer(_amountToWithdraw);
    assert(balances[/*payable(*/msg.sender/*)*/] == 0);
// msg.sender doesn't need to be payable to look up the balance in the mapping 
    emit withdrawSuccessful(/*payable(*/msg.sender/*)*/, _amountToWithdraw);
// msg.sender doesn't need to be payable to emit/log this address as event data
    // _success = true;
// Do you use this return value? If not, you should remove it.
}

Let us know if you have any questions :slight_smile:

1 Like
#1157

@jon_m this is weird, setting a users balance maping to 0 like this is standard in a lot of contracts. . so it worked when seeting to 1 and not zero. so this issue happens in sol 0.8.9? i want to check cos i want to try reproduce

2 Likes
#1158

ok so i was playing around with remix for the past 20 minutes trying anything and everything. @jon_m yeah i can confirm evrything that youve done above, setting it to any value other than zero works, but setting directly to zero causes a gasLimit error. even tried setting the gas limit stupidly high and the same.

so then i started looking around on google and came across this post from three years ago

https://ethereum.stackexchange.com/questions/11149/problem-setting-uint-value-to-zero

a guy in this post was having a similar issue which was puzzling some of the people trying to help him out. however one guy confitmed that at the time it was an issue with remix and the browser Solidity interacting with geth. he then goes on to show exactly what jon does above by deploying manually and it works. apparently at the time of that post it was an issue with like three versions

so i tried like 5 or six different versions and the same gas error presisted across all the compiler versions i tried. so this must definitely be an issue with remix and connecting to geth or something like that. i think this would be worth opening an issue. i wonder where we could do this though.

this is an insane bug as a withdrawAll function is quite common. i can say with confidence that the eroor is due to the javascript VM that remix uses because when i deployed the test contratc i wrote which users your withdraw all function with injected web3 to kovan it works fine. so its seems like the bug only presists in the local version of remix and their javascript VM, tried chaning the deafult EVM version to london berlin and a few others also and the error on the local deployment still remains. very weird indeed

3 Likes
#1159

Hey @mcgrane5 & @Giupi,

Thanks for confirming that @mcgrane5 ! … I agree that it’s very weird indeed. But at least we’ve come to the same conclusion.

Yeh … I tried compiler v0.5 and v0.7 and still had the same issue. I also tried using the call method instead of transfer, but it’s not the line of code that sends the Ether which is causing the problem. It’s the assignment of zero to the mapping … so weird!!

Yeh, that’s exactly what I couldn’t understand. For ages, I thought I was just missing something obvious due to brain fog!

Funnily enough, I was just looking for where to do this, and the following issue opened earlier today in the remix-project repository looks like it relates to the same bug …

https://github.com/ethereum/remix-project/issues/2320#issue-1207367110

I haven’t looked at it in detail yet … but this seems like further evidence that we are dealing with a bug, which will hopefully soon be fixed by their developer team :sweat_smile:

1 Like
#1160

Thank you @jon_m and @mcgrane5 I really appreciate your responses.
I’m not sure how to open an issue with Remix, but I’ll do some research and see what I can find.

In the meantime thanks again. I love Moralis and this forum!

2 Likes
#1161

No problem @Giupi … thanks for giving us a good challenge :sweat_smile:

Have a look at what I’ve posted in the last few minutes … I’ve found an issue opened in the Remix Project’s GitHub repository that looks like it may be caused by the same bug. I haven’t looked in detail yet, but you may want to start there :wink:

1 Like
#1162

aww thats good @jon_m, it must be something that is only recent especillay if the issue was opened today. and @Giupi no worries my man glad you love the fourms

2 Likes
#1163

@jon_m Thanks for sharing these links. As a matter of fact, I had a similar issue with another function that I wrote to basically remove a struct from an array of structs. It seems related to the issue pointed out in the link you shared, I’m guessing since, when using delete on a struct, it would revert all values to zero. Maybe. Not sure, but it’s an interesting learning experience.

2 Likes
#1164

hahaha glad to give you guys a nice challenge :smile:

2 Likes
#1165

If you look up or reference a value in a mapping or a fixed-size array, and the value has either previously been deleted using delete, or you use a key or index value which hasn’t had a value mapped or assigned to it yet, then you will retrieve the equivalent zero value according to the data type…

  • uint and int  …  0
  • bool  … false
  • address  … 0x000...000  (zero address)
  • string  … ""  (empty string)

If the value you look up or reference, as above, is a struct instance, its properties will also be equivalent zero values according to each property’s data type.

If you try to retrieve a value from a dynamically-sized array at an index value which hasn’t had a value assigned to it yet, then this results in an error.
I’ll have to check whether it’s possible to use delete to delete a previously assigned value at a specific index within a dynamically-sized array. Unfortunately, I haven’t been able to quickly test this in Remix because of the bug we’ve discovered! :grimacing: If this can be done, I’d assume that the index value would subsequently hold an equivalent zero value (or a struct instance with properties holding their equivalent zero values), and that it would be these zero values which would be retrieved when referencing this index value (as with the fixed-size array).

I’ll have a look in a bit more detail to see if this relates to the issue I linked to in the Remix Project repository in GitHub … and I’ll get back to you.

#1166 
    function withdraw(uint amount) public {
        require(msg.sender.balance >= amount, "Not enough funds.");
        require(balance[msg.sender] >= amount, "Not enough funds.");

        uint sourceAccountBalance = msg.sender.balance;
        payable(msg.sender).transfer(amount);

        uint sourceInternalBalance = balance[msg.sender];
        balance[msg.sender] -= amount;

        assert(msg.sender.balance - amount == sourceAccountBalance);
        assert(balance[msg.sender] + amount == sourceInternalBalance);
    }
1 Like
#1167

hey @xenmayer nice solution but theres a few things. why do you sue msg.sender.balance and balance[msg.sender]. you dont need both. msg.sender.balance holds the wei denominated ether balance of your wallet address. however balance[msg.sender] holds the wei denominated ether balance of the user in the SMART CONTRACT, so since your funds have been deposited into the smary contarct there is no need to make a check on the users wallet balance theough msg.sender.balance.

this is actually a big risk. you wont have came across this because all remix accounts have 100 ether which takes a long time to burn through. but if you some how managed to get your wallet balance to zero, for example you could deposit 100 ether into the contract with your deposit function. then if you try to withdraw that 100 ether back into you wallet with your implementation of the function above this would flag yur revert at line

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

this is because this check isw checking whether you wallet address has funds greater than the amount your trying to withdraw which will always be false in thi scase because yuour wallet address now has 0 balance after we deposuted the full 100 ether. so the only way to get this money back would be to fund your wallet address with an amount of ether eqaul to or greater than the amount your trying to withdraw. so in this case to get back the 100 ether you would need to get another 100 eth into you wallet address. so this is a big securoty risk. below ive optimised yur function

 function withdraw(uint amount) public {
        require(balance[msg.sender] >= amount, "Not enough funds.");
        uint balanceBefore = balance[msg.sender];
        balance[msg.sender] -= amount;
        payable(msg.sender).transfer(amount);
        assert(balanceBefore - amount == balance[msg.sender]);
    }

we can get rid of the first require statement. and the diffeence here is we store the previous balance in a temp variable so that we can reduce the balance of msg.sender before the actual transfer. not that its a big risk here, bu this just makes sure our function is safe from being exploitable in a callback exploit (reentrancy attack), but since we stroed their previous balance in the tep variable we can do the assertio check after the transfer is done.

so we can see that the new code is a lot more consise and efficent. not at bad attempt at all keep it up

2 Likes
#1168

you could use msg.sender.balance in the deposit function but should use it here

1 Like
#1169

you should chexk this youyrself with your code. try deposit 100 ether or the max amount you wallet has, then try to withdraw even 1. it will flag an error due to the first revert statement

2 Likes
#1170

Hey @mcgrane5 , thank you for clarification. It seems that I’ve misunderstood the wallet and smart contracts balances representation. It’s now clear for me, thank you.

2 Likes
#1171

no worries i actually made a mistake in my first post i noticed you seen that as you were replying. lol i actually missed this until i double checked by taking your code into remix. its funny because msg.sender.balance is something I never usually use because the evm will handle that for you if you try to deposit an amount greater than your walket balance. and if your making a frontend down the line and use ethers.js, ethers wont even allow metamsk to popup if checks like these are not met. so you will very rarely find yourself using msg.sender.balance.

the only siutation i could find it useful in is say if you wanted to know the users wallet balance for some calulcation within a function. what this calculation could be may vary, but you will never need to use this for actual safety checks

2 Likes
#1172

Thank you @mcgrane5 :pray:

1 Like
#1173

What am i missing here @jon_m

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

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

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

    balance[msg.sender] -= amount;

    msg.sender.transfer(amount);

}`Preformatted text`
#1174

This seems to have done the trick :grin:

pragma solidity 0.7.5;

contract Bank {

     mapping(address => uint ) balance ;

     address owner;

     event depositDone(uint amount, address indexed depositedTo);

     modifier onlyOwner {

         require(msg.sender == owner );

         _; //underscore makes the function run

     }

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

         return balance[msg.sender];

     }

     function withdraw(uint amount) public  {

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

         msg.sender.transfer(amount);

         balance[msg.sender] -= amount;

     }

     function transfer(address recipient, uint amount ) public  {

       // check balance of sender

        require(balance[msg.sender]>= amount, "Balance not sufficient");

        require(msg.sender!= recipient);

       _transfer(msg.sender, recipient, amount);

       //event logs and further checks

     }

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

        balance[from] -= amount;

        balance[to] += amount;

     }

}
1 Like
#1175

Hi @Rebe,

You need to remove your first require() statement …

This will fail and revert the transaction whenever the user’s individual balance is greater than the requested withdrawal amount… which is the opposite of what we want. Your second require() statement contains the correct condition, and so you only need this one.

Once you’ve removed the incorrect require() statement, you will then have all of the lines of code needed to solve the problem with the withdraw function :ok_hand: And your statements are also in the correct order within the function body to reduce security risk:

  • Check inputs (require statement)
  • 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…

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

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, the share of the total contract balance they are entitled to withdraw) 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 :+1:

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 if you do include it in the function header, you should also include a return statement in the function body. The compiler will give you an orange warning about this.

Let me know if you have any further questions.

1 Like