why is it that when you deposit Ether into a payable function, it sometimes the funds dont get subtracted from your address and added to the contract, but decides to work when you try it again a few times at random. Also, when depositing say 20 ether into a contract, sometimes less than 20 ether ends up being deducted from my address and added to the smart contract. Is this a feature within remix?
Hi @Marta_Tofan,
(1) Withdraw function
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.
(2) Transfer Event (from the Events Assignment)
Your transfer event and corresponding emit statement are both accurately coded, and the emit statement will log the data when the transfer() function is successfully executed.
A couple of observations…
-
An emit statement should be placed at the end of a function, after the event itself has occurred, but before a return statement if there is one. Generally speaking, it’s probably also better to place an emit statement after an assert statement if there is one.
-
Where the deposit() function executes a transaction that only involves 1 user address (the depositor), the transfer() function executes a transaction that involves 2 user addresses (the sender and the recipient). So, it would be useful to include the sender’s address, as well as the recipient’s address, within the data emitted for the transfer event.
Please, format all your code when posting it, instead of just parts of it. This will make it clearer and easier to read. It will also make it easier to copy and paste if we need to deploy and test it. You just need to make sure that all your code is placed between the two sets of 3 back ticks i.e.
```
all your code
```
Follow the instructions in this FAQ: How to post code in the forum
Let me know if you have any questions
Hi @Adrian1,
The same amount of Ether, Finney, Gwei or Wei that you enter into the Value field will always be deducted from the sender’s address balance that is showing in the Account field, near the top of the Deploy & Run Transactions panel in Remix. Make sure you set the dropdown next to the Value input box to the appropriate currency unit for the amount you want to send e.g.
20 Ether … will deduct 20.0 Ether from the sender’s address balance
2 Finney … will deduct 0.002 Ether from the sender’s address balance
25 Gwei … will deduct 0.000000025 Ether from the sender’s address balance
275 Wei … will deduct 0.000000000000000275 Ether from the sender’s address balance
If you are sending units of Wei instead of Ether, this could be why you aren’t noticing the change in the address balance, because it’s only affecting the last decimal places.
When you call the deposit() function, the amount of Ether that is sent to the contract will be added to the contract address balance. If you add the following function to your contract, you can call it to retrieve the current contract address balance (instead of the individual user balances stored in the mapping).
function getContractBalance() public view returns(uint) {
return address(this).balance;
}
The value returned from this function, and also from the getBalance() function, will always be in units of Wei e.g.
Return value of …
20000000000000000000  =  20 Ether
   2000000000000000  =   2 Finney
        25000000000  =  25 Gwei
               275  =  275 Wei
Let me know if anything is still unclear, or if you have any further questions.
function withdraw(uint amount) public returns (unit)
{
require(balance[msg.sender] >= amount);
msg.sender.transfer(amount);
balance[msg.sender] -= amount;
return balance[msg.sender];
}
pragma solidity 0.8.7;
contract Bank {
mapping(address => uint)balance;
address owner;
event depositDone (uint _amount, address _depositedTo);
event amountWithdrawn (uint _amount, address _withdrawnFrom);
constructor(){
owner = msg.sender;
}
modifier onlyOwner {
require(msg.sender == owner);
_;
}
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, "You cannot withdraw more than what you have!");
balance[msg.sender] -= amount;
payable(msg.sender).transfer(amount);
emit amountWithdrawn(amount, msg.sender);
return balance[msg.sender];
}
function getBalance () public view returns(uint) {
return balance[msg.sender];
}
function transfer(address recipient, uint amount) public {
require(msg.sender != recipient, "You cannot transfer to yourself!");
require(balance[msg.sender] >= amount, "Balance insufficient.");
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] = balance[from] - amount;
balance[to] = balance[to] + amount;
}
}
Hi @oioii1999,
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.
And don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.
Let me know if you have any questions
Great solution @karendj and, overall, a very well coded contract
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.
Your withdraw event and corresponding emit statement are also both well coded, and a nice addition. 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.
Keep up the great coding!
Thanks for your advice! @jon_m
I have updated the code for the withdraw function as below, and submitted the event assignment in this post. Please check it at your convenience.
function withdraw(uint amount) public returns (unit)
{
require(balance[msg.sender] >= amount);
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
return balance[msg.sender];
}
// SPDX-License-Identifier: GPL-3.0
pragma solidity >= 0.8.0;
contract bank{
mapping(address => uint) balance;
address owner;
event depositDone(uint amount, address indexed to);
event transferencedone(uint amoun, address indexed to, address indexed from);
constructor(){
owner = msg.sender;
}
modifier onlyOwner{
require(msg.sender == owner);
_;
}
function deposit() public payable returns(uint){
balance[msg.sender] = 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, "Saldo insuficiente");
msg.sender.transfer(amount);
}
function getBalance() public view returns(uint){
return balance[msg.sender];
}
function transfer(address recipient, uint amount) public {
//check balance del msg.sender
require(balance[msg.sender] >= amount, "Saldo insuficiente");
require(msg.sender != recipient, "No envies dinero a ti mismo");
//creando variable que permite revisar el estado de la transaccion al finalizar la misma
uint previousSenderBalance = balance[msg.sender];
_transfer(msg.sender, recipient, amount);
//emitiendo events logs cuando alguien haga una transferencia
emit transferencedone(amount, msg.sender, recipient);
//usando assert para verificar si el codigo esta correcto
assert (balance[msg.sender] == previousSenderBalance - amount);
}
//implementando visibilidad, funcion privada
function _transfer(address from, address to, uint amount) private {
balance[from] -= amount;
balance[to] += amount;
}
}
To make sure that the user can not withdraw more then their balance
you check the balance for message sender but withdraw from owner.
function withdraw(uint withdrawAmount) public returns (uint) {
require(balances[msg.sender] >= withdrawAmount);
balances[msg.sender] -= withdrawAmount;
emit LogWithdrawal(msg.sender, withdrawAmount, balances[msg.sender]);
return balances[msg.sender];
}
Hi @valleria,
Sorry for the delay in giving you some feedback!
withdraw()
 function
(1) You should have found that your contract code didn’t compile, and so couldn’t be deployed…
Your pragma
statement is declaring that the code should be compiled based on Solidity v0.8 syntax. That’s absolutely fine, but then you also need to bear in mind that this course material is based on v0.7 syntax. The compiler will always generate an error when your syntax needs to be changed for v0.8, and the error message will tell you why, and often what you should change it to. In this case, you will have got a red compiler error for this line of code…
In order to receive Ether, an address needs to be payable
. Prior to Solidity v0.8, msg.sender
is payable by default and so doesn’t need to be explicitly converted. However, from v0.8, msg.sender
is non-payable by default, and so when it needs to refer to a payable address, it has to be explicitly converted. One of the ways to do this is using  payable()
 e.g.
payable(msg.sender).transfer(amount);
If you make this modification, you will notice that the compiler error disappears.
(2) The require statement you’ve added is correct, but your withdraw function is also missing an essential line of code …
If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the caller’s external address receives the requested withdrawal amount, but their balance in the mapping is not reduced! This means that they can make repeat withdrawals — each one no greater than their initial balance, which isn’t being reduced — up to the total amount of Ether held in the contract (the sum of all of the individual account holders’ balances). So, I’m sure you can see why this is a very serious bug!
payable(msg.sender).transfer(amount)
  transfers Ether from the contract address balance to the caller’s external address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total Ether balance. So, just as we increase a user’s individual balance when they deposit Ether in the contract, we need to do the opposite whenever they make a withdrawal.
(3) 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 in the function body. Once the red compiler error (described above) has been corrected, the compiler will still give you an orange warning about this issue.
(4) Once you’ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.
Transfer Event (from the Events Assignment)
Your transfer event declaration is well coded, and the corresponding emit statement, which you’ve added to the transfer() function body, will log data when the transfer() function is successfully executed.
However, you are emitting the msg.sender
address as the event’s to
address parameter, and the recipient
address as the event’s from
address parameter. Can you see that you’ve got these the wrong way round? The position of each value in the emit statement must be the same as that of its corresponding parameter in the event declaration, in terms of both data type and name. If the data types don’t correspond, then the contract won’t compile, but if the names are mixed up, then the affected data will be logged against potentially misleading identifiers (which is what will happen with your solution).
But you have referenced amount
in the correct position in your emit statement
Just one other observation …
In general, an emit statement is probably better placed after an assert() statement.
Let me know if anything is unclear, if you have any questions about any of these points, or if you need any further help to correct your solution code
Hi @kcoffey,
Correct
No …
The Ether is held in the contract address balance, so to perform the actual withdrawal we need to transfer an Ether value equivalent to the requested withdrawAmount
from the contract address to the caller’s external address (msg.sender
). We can do this with Solidity’s in-built transfer
method …
<address payable>.transfer(uint amount)
So you are missing the line of code which was given to you in the assignment’s introductory video …
msg.sender.transfer(withdrawAmount);
This line of code …
… correctly reduces the withdrawer’s individual balance in the mapping by the withdrawal amount. These individual user balances in the mapping perform an internal accounting role, and record each user’s share of the contract’s total Ether balance. So, when your withdraw() function is called, the caller has their entitlement to make further withdrawals reduced by the requested withdrawal amount, but their external address doesn’t actually receive the equivalent amount of Ether from the contract!
Don’t get confused between the contract owner’s address and the contract address itself …
-
The contract address is the address the contract is deployed at. It is the address next to the deployed contract’s name below Deployed Contracts at the bottom of the Deploy & Run Transactions panel in Remix. You can copy this address to the clipboard by clicking on the adjacent icon.
-
In our contract, the contract owner’s address is the address the contract is initially deployed by — the address which is showing in the Account field near the top of the same Remix panel when you click the orange Deploy button.
Apart from this …
-
The require() statement and the return statement you’ve added to your withdraw() function are both correct
-
The LogWithdrawal event you are emitting is also a nice addition. It’s in the correct position within your withdraw() function body, and it will log relevant information when a call to the withdraw() function has executed successfully. The emit statement looks to be coded correctly. Can we also see the event declaration that it’s based on?
Once you’ve corrected your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.
Let me know if anything is unclear, or if you have any questions
My solution (before checking solutions) :
function withdraw (uint amount) public returns (uint) {
require (balance[msg.sender] >= amount);
uint previousBalanceOnSmartContract = balance[msg.sender] ;
payable(msg.sender).transfer(amount); // here we transfer the money to msg.sender
// (it means we transfer the money from the SC wallet)
// So now we need to take it from balance of msg.sender on the SC (=balance[msg.sender]) so
// that this address cannot use more balance than it has on this SC
balance[msg.sender] -= amount; // here we substract from the
// balance of msg.sender on the current smart contract this amount of money
assert (balance[msg.sender] == previousBalanceOnSmartContract - amount) ;
return balance[msg.sender];
}
Though I think in reality this solution is not good as we don’t take into account gas fees. With this first solution, gas fees would be on the SC wallet and not on the balance[msg.sender] so gas fees would end up drying the SC wallet.
So, not sure yet how to integrate gas fees, I would prefer a solution like this :
int CurrentGasPrice; // this value could be added when calling
// the function or taken through an Oracle maybe.
modifier cost (int CostInGas) {
require(msg.value >CostInGas*CurrentGasPrice); // if = it's useless
_;
}
function withdrawWithGasFess (int amount) public cost(1000) returns (uint) {
// We calculate (for example) that all the operations in this function will cost 1000 gas. Also
// but that's the gas cost and we need to current gas price to check if msg.value is over gas price *1000
require (balance[msg.sender] > amount); // if it's = it means no space for gas fees so useless to go on.
uint previousBalanceOnSmartContract = balance[msg.sender] ;
payable(msg.sender).transfer(amount); // here we transfer the money to msg.sender from
// the SC wallet, but we use the SC wallet to pay for gas fees
balance[msg.sender] -= amount + 1000*CurrentGasPrice; // Here we also withdraw
// the estimation in gas fees that the transaction will be
assert (balance[msg.sender] == previousBalanceOnSmartContract - amount - 1000*CurrentGasPrice) ;
return balance[msg.sender];
}
Hi @seybastiens,
Your original solution
You have added all of the additional lines of code needed to solve the problem with the withdraw function, and the additional two lines you’ve included for the assert() statement are also well coded
The only improvement that can be made is to change the order of two of the statements in your withdraw function body …
The following withdrawal pattern (order of statements) should be used in order to reduce security risk:
- Check inputs (require statements)
- Effects (update the contract state)
- External Interactions (e.g.
transfer
method)
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. We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra info. 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 good to be aware of and to start getting into good habits.
Just another couple of important points to clarify …
It’s important to understand that this line of code transfers Ether from the smart contract’s address balance to the external account balance of msg.sender
. However, by itself, it doesn’t leave us with any record of the reduced share of the total contract balance which msg.sender
is now entitled to. This is achieved by also reducing the withdrawer’s individual user balance in the mapping …
So, it’s important to also understand that it’s the individual user balances stored in the mapping, which perform an internal accounting role, and track each user’s share of the contract’s total Ether balance as this changes over time.
Your second solution
We don’t need to take into account gas fees in our code. The gas fees for each transaction, whether a call to the deposit(), withdraw() or transfer() function, will always be paid by the calling address (the address sending the transaction) and deducted from their external address balance together with any Ether value they are also sending (which in our Bank contract only happens when calling the deposit function). But while the Ether value sent to the deposit() function is added to the contract address balance, the gas fees are not.
So, this modifier doesn’t make any sense, and will always fail, because msg.value
will always be zero for the withdraw() function. This is because no Ether value is ever sent to this function, and msg.value
never includes gas fees. In fact, no Ether can ever be sent to the withdraw() function because it is not marked payable
, and it is right that this restriction is enforced.
The gas fees are processed by the EVM according to the Ethereum protocol. Under proof-of-work, the gas fee for each transaction (transaction fee) is paid to whichever miner successfully mines the block it is included in. Under proof-of-stake it gets more complicated, with validators instead of miners, and a proportion of the gas fees being burnt.
So, when an address calls the withdraw() function, the gas fees for executing the whole transaction — including all of the low level operations associated with the withdraw function’s code — are deducted from that address’s account balance (showing in the Account field in the Remix panel). This includes the fees for executing the transfer
method, which transfers an Ether value equivalent to the withdrawal amount (input into the function as the amount
argument) from the contract address balance to the account balance of the external calling address (showing in the Account field in the Remix panel).
Only an external address, via a web3 client at the frontend, can be used to sign and send a transaction which incurs a gas fee. This transaction may trigger a smart contract to send/transfer funds (whether Ether or other tokens) to other smart contracts, other external addresses or, as happens with the withdraw() function, to itself. But at no point do smart contract addresses ever pay gas fees, and neither are gas fees ever added to smart contract address balances or stored within them.
So, this won’t happen and isn’t even an issue.
I hope that makes sense now, but just let me know if anything is still unclear, or if you have any further questions
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);
_;
}
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"); //check that sender's balance is bigger than the amount to be withdrawn
msg.sender.transfer(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;
}
}
Hi @menarf,
The require statement you’ve added is correct, but your withdraw function is missing an essential line of code …
If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the caller’s external address receives the requested withdrawal amount, but their balance in the mapping is not reduced! This means that they can make repeat withdrawals — each one no greater than their initial balance, which isn’t being reduced — up to the total amount of Ether held in the contract (the sum of all of the individual account holders’ balances). So, I’m sure you can see why this is a very serious bug!
msg.sender.transfer(amount)
 transfers Ether from the contract address balance to the caller’s external address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total Ether balance. So, just as we increase a user’s individual balance when they deposit Ether in the contract, we need to do the opposite whenever they make a withdrawal.
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 have given you an orange warning about this.
Once you’ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.
By the way, I think you may have missed out the Events Assignment, because you are missing a Transfer event, and a corresponding emit statement, in your contract. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.
Let me know if anything is unclear, if you have any questions, or if you need any more help with correcting your solution
Ok well understood.
Thanks for the deep explanation.
Hi @jon_m,
Thanks a lot for your reply.
I guess I didn’t think through the whole task very well. I realized it straight away when I saw Filip’s solution in the following video.
As for Events Assignment, you are right, I missed it completely
I added transfer event to my code and corrected withdrawal function as per your suggestions and Filip’s solution video.
pragma solidity 0.7.5;
contract Bank {
mapping(address => uint) balance;
address owner;
event depositDone(uint amount, address indexed depositedTo);
event amountTransferred (uint amount, address sender, address recipient);
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");
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);
emit amountTransferred(amount, msg.sender, recipient);
}
function _transfer(address from, address to, uint amount) private {
balance[from] -= amount;
balance[to] += amount;
}
}
Nice modifications @menarf, … your whole contract’s looking great now!
The transfer event and corresponding emit statement you’ve added are both correct and well coded. The emit statement is in the correct position in the transfer() function body and will log appropriate data when the transfer() function is successfully executed
Don’t worry! … As long as you carefully review and check your solution afterwards, in order to understand anything you missed or should be coding differently, you will make good progress. Obviously, there are often several alternative solutions, and we can help you to review and check these kinds of differences when you post your code here in the forum.
See below - add require(balance[msg.sender] >= amount,"Balance not sufficient");
and update balance with balance[msg.sender] -= amount;
pragma solidity 0.7.5;
contract Bank {
constructor(){
owner = msg.sender;
}
mapping(address => uint) balance;
address owner;
modifier onlyOwner {
require(msg.sender == owner);
_; //run the function
}
event depositDone(uint amount, address 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 not sufficient");
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;
}
}