Are you maybe only inputting a value of, say 5 for the amount argument, instead of 5000000000000000000 ? Solidity handles values in units of wei, not ether. In Remix, when sending a value to a contract, we have the dropdown to allow us to input the value in units of ether, and it will be converted to wei for us. Unfortunately, we donât have that option when calling a function with a uint value we want to be processed as an amount in ether â we have to input our ether amount in uints of wei, with all the zeros!
If that is the issue, then the balance will be being updated, but only for a very small amount, and so you may not have noticed it if you are looking for a change in units of ether.
If that isnât the issue, then post your full contract code, and Iâll take a look.
You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are also in the correct order to reduce security risk:
check inputs (require statements)
effects (update the contract state)
external interactions
Just as you have done, itâs important to modify the contract state for the reduction in the balanceâŠ
balance[msg.sender] -= amount;
before actually transferring the funds out of the contract to the external wallet addressâŠ
msg.sender.transfer(amount);
⊠just in case there is an attack after the transfer, but before the state is modified to reflect this operation. Youâll learn about the type of attack this prevents, and how it does 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
Apologies for the delay in answering your questionâŠ
The order of the statements does matter, yes, although we wouldnât expect you to know about this yet. But seeing as youâve askedâŠ
In order to reduce security risk, we should apply the following order to the statements we place within the function body:
check inputs (require statements)
effects (update the contract state)
external interactions
Itâs important to modify the contract state for the reduction in the balanceâŠ
balance[msg.sender] -= amount;
before actually transferring the funds out of the contract to the external wallet addressâŠ
msg.sender.transfer(amount);
⊠just in case there is an attack after the transfer, but before the state is modified to reflect this operation. Youâll learn about the type of attack this prevents, and how it does it, in the courses which follow this one. But itâs great youâre already noticing these kinds of details and wanting to find out why they are as they are
Just let me know if if you have any further questions
Just be careful not to mix up your assignment (=) and equality (==) operatorsâŠ
This makes a huge difference, because your code (although it does still compile) does not reduce the userâs balance in the mapping by the withdrawal amount. Iâm sure you can see why this is a very serious bug: even though the require statement places a limit on each separate withdrawal, this limit is never reduced to reflect each withdrawal, meaning that the user can repeatedly make withdrawals of amounts up to their initial balance, until the contract balance (which includes all user account balances) is reduced to zero!
the subtraction assignment operator, which has the minus sign before the equals sign ( -= ). This will deduct the value on the right (the withdrawal amount) from the userâs current balance stored in the mapping.
and notâŠ
The âstandardâ assignment operator (=) followed by the unary minus operator ( - ). This negates the value on the right of the assignment operator (the withdrawal amount) and then assigns the resulting value as the userâs balance in the mapping (replacing their current balance). As the mapping is defined as unsigned integers mapped to addresses (address => uint) , it cannot store negative balances. So, if you input a positive withdrawal amount (as expected), the unary minus operator will convert this to a negative amount, and when this is assigned to the mapping it will be expressed as an extremely large positive balance â the largest integer which can be stored with 256 bits (a binary number with 256 1âs) less the withdrawal amount. This happens as a result of something called underflow.
Iâm sure you can see why this is a very serious bug, because the limit on withdrawals is so high, that it effectively allows the user to withdraw all of the funds held in the contract (which includes all user account balances).
Let me know if anything is unclear, or if you have any questions
Youâve done a great job at explaining these concepts to @Flippy
Your terminology is actually very good. The term used in the Solidity documentation for attributes or properties is members, but it is clear what you mean by attributes, and thatâs the main thing.
Just a couple of observations âŠ
This is correct if our mapping is defined with an address data type as the keyâŠ
mapping(address => User) users;
⊠and not with an unsigned integer data type âŠ
From your explanation, I think this is actually how you meant to define your mapping, but I just wanted to clarify this.
With the mapping defined as Userinstances (created from the struct) mapped to addresses (and not unsigned integers),âusers[msg.sender] will reference a User instance, and not an address, and so instead ofâŠ
⊠the local activeUser variable should be defined with the custom data type User (and not as an address)âŠ
User activeUser = users[msg.sender];
uint activeUserBalance = activeUser.balance;
I think itâs important to make clear that, when we are referencing a specific âentryâ in a mapping, the value within the square brackets is the key the entry is mapped to, and the data type of the key (the data type declared before the arrow in the mapping definition) is usually not the same as the data type of the value that is mapped to it (the data type declared after the arrow in the mapping definition).
Your explanation breaking down â msg.sender.transfer(amount); â is really quite excellent
Strictly speaking, msg is not a data type, but instead we can consider msg.sender as representing one value with an address data type (the address calling the function). Address data types in Solidity have many available members, some of which are what you have described as methods â which I think is a really good term to use to describe how these particular members operate.
Here is a link to the full list of Members of Address Types in the documentation:
Apologies for the delay in giving you feedback on your solution to this assignment, and in answering your question.
You have added all of the necessary code needed to solve the problem with the withdraw function
In order to receive ether, an address needs to be payable . From Solidity v0.8 msg.sender is non-payable by default, and so when it needs to refer to a payable address, it needs to be explicitly converted. One of the ways to do this is using payable() . If you are using v0.8, then this is why the compiler was generating an error when you hadâŠ
msg.sender.transfer(amount);
// instead of
payable(msg.sender).transfer(amount);
You need to bear in mind that this course material is based on v0.7 syntax. In versions prior to v0.8 msg.sender is payable by default and so doesnât need to be explicitly converted. Thatâs why the course video uses âmsg.sender.transfer(amount);â without the compiler throwing an error.
Youâve modified the withdraw function with the onlyOwner modifier, but this means that only the contract owner can withdraw funds up to their own individual balance. But the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, donât you think?
Also, have a look at this post which explains the importance of the order of the statements within your withdraw function body.
Just let me know if you have any further questions
Apologies for the delay in giving you feedback on your solution to this assignment.
You have added all of the necessary code needed to solve the problem with the withdraw function
However, youâve modified the withdraw function with the onlyOwner modifier, but this means that only the contract owner can withdraw funds up to their own individual balance. But the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, donât you think?
Iâm sure this was just a slip, but if you donât includeâsolidity , and the semi colon at the end of the statement, the compiler will throw an error:
pragma solidity 0.8.4;
Also, have a look at this post which explains the importance of the order of the statements within your withdraw function body.
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.
An excellent assignment solutuion @Nathan_Burkiewicz and, overall, a very well-coded contract
You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are also in the correct order to reduce security risk:
check inputs (require statements)
effects (update the contract state)
external interactions
Just as you have done, itâs important to modify the contract state for the reduction in the balanceâŠ
balance[msg.sender] -= amount;
before actually transferring the funds out of the contract to the external wallet addressâŠ
msg.sender.transfer(amount);
⊠just in case there is an attack after the transfer, but before the state is modified to reflect this operation. Youâll learn about the type of attack this prevents, and how it does 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
Nice solution @Julian97 and, overall, a very well-coded contract
You have added all of the necessary code needed to solve the problem with the withdraw function. Your emit statement for the transfer event is also now correct.
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.
From your explanation, I think this is actually how you meant to define your mapping, but I just wanted to clarify this.
Yeah, I did - forgot to swap it out having taken some code from the data location assignment.
And this bit:
User activeUser = users[msg.sender];
uint activeUserBalance = activeUser.balance;
âŠmakes total sense. This is what I get for not testing my code before posting!
And agreed on highlighting key vs value. Appreciate it just a particular case in this example that key is the address and the value being the custom User data type also happens to contain the address. Appreciate that wonât always be the case and makes sense to call it out.
Your explanation breaking down msg.sender.transfer(amount); is really quite excellent
Thanks, glad you think so!
Strictly speaking, msg is not a data type,
I had a sense that this must be the case, think I was trying to oversimplify and find a way to relate the concepts somehow though I have a question on this - is the description of msg as an object accurate and how does that differ from a custom data type like User?
I appreciate that msg is the message being sent and I was presuming it must be a standardised object. Iâm thinking in terms of my understanding of Python classes here:
class message:
def __init__(self, address, data, value, sig):
self.sender = address
self.data = data
self.value = value
self.sig = sig
def printSender(self):
print("The sender's address is " + self.sender)
msg = message(0x123456..., 78, 90, "xyz")
msg.sender # call attribute
msg.printSender() # perform function
>>
In Python, the User data type would be defined in a very similar way which is why I referred to msg as a data type.
Another question on this - in Solidity, is there no word other than âmemberâ to differentiate between what I consider a static attribute (ie an unchanging piece of data extracted from an instance such as msg.sender) and an active method that applies a function (such as msg.sender.transfer(amount) )?
If you are interested in these sorts of security topics then I highly recommend you take the Smart Contract Security course, either after this one or after Ethereum Smart Contract Programming 201. Itâs a really good course as it covers real case studies of famous attacks that have happened, looks at the code that caused the vulnerabilities, and then how these attacks can be prevented (or the risk posed by them seriously mitigated) by adopting specific best practices in how we code certain aspects of our smart contracts.
PSâIâm saving my reply to your other post for after a strong cup of coffee some time tomorrow, so I can give it the attention it deserves
Hey all. Banged my head against this one for quite some time but eventually figured it out. I think.
My code and notes:
pragma solidity >= 0.8.4;
contract metacontract{
constructor(){
owner = msg.sender;
}
mapping(address => uint) balance; //key value pair, address returns balance.
mapping(address => uint) ethBalance; // second KvP for tracking Eth deposits.
address owner;//if i comment this out it breaks. why?
modifier reqAdmin{ //modifier req admin priv to execute function.
require(msg.sender == owner);
_; //run the function.
}
//event listeners.
event eventTransfer(uint amount, address indexed depositedTo, address indexed sentFrom); //declares event to track with emit
event eventDeposit(uint amount, address indexed sentFrom);
event eventEthDeposit(uint amount, address indexed sentFrom);
function adminSetBalance(address target, uint value) public reqAdmin { //admin tool: set balance.
balance[target]=value;
}
function getBalance(address target) public view returns(uint) { // return address balance.
return balance[target];
}
//----------------------------------------------------
// Eth related functions
function getEthBalance() public view returns(uint){
return ethBalance[msg.sender];
}
function depositEth() public payable returns(uint){ //deposit ETH to contract.
ethBalance[msg.sender]+= msg.value;//msg.value replaces need to include function argument.
emit eventEthDeposit(msg.value,msg.sender);
return balance[msg.sender];
//assert
}
function withdrawlEth(uint amount) public payable returns(uint){
require(ethBalance[msg.sender] >= amount);//amount is used instead of msg.value
ethBalance[msg.sender]= ethBalance[msg.sender]-amount;//this line adjusts acc balance before transfer &protect against rentrancy;
(bool success, ) = msg.sender.call{value: amount} ('');
require(success, "transfer failed.");
return ethBalance[msg.sender];
}
}
You have done some excellent research to arrive at a robust solution for the withdraw function, which not only prevents users from withdrawing more than their individual account balances, but also:
uses the address memberâcall()âto perform the transfer of ether from the contract address balance to the external address, in order to take into consideration the latest advice not to use the address membersâtransfer()âorâsend()âafter the Istanbul hard fork; and
guards against the risk of re-entrancy attacks using the Checks-Effects-Interactions Pattern.
As you can see from the documentation for the latest version of Solidity (link below), the address membersâtransfer()âandâsend()âare not deprecated and still available to use.
However, it is true that the latest advice is not to use them. This is because the fixed stipend of 2300 gas, which they forward to a calling contractâs fallback function, used to be enough gas for the fallback function to execute successfully, but not enough gas to allow a re-entrant call (thereby reducing the risk of re-entrancy attacks). But since the Istanbul hard fork changed the gas charges for certain operations, this fixed gas stipend is no longer guaranteed to be sufficient.
Your implementation of âcall()â is correct. And the order you have placed each statement within the withdraw() function body is correct to guard against re-entrancy attacks, which would otherwise be more of risk as a result of usingâcall()âinstead ofâtransfer() âŠ
check inputs (require statements)
effects (update the contract state)
external interactions
Just as you have done, itâs important to modify the contract state for the reduction in the individual userâs balanceâŠ
balance[msg.sender] -= amount;
before actually transferring the funds out of the contract to the external address âŠ
⊠to prevent a malicious fallback function making successful re-entrant calls to the withdraw() function, and executing multiple withdrawals before the individual account balance is modified in the contract state to accurately reflect them, which would otherwise enable the initial require statement to fail and trigger revert.
Youâll learn more about these kinds of smart-contract security issues in the courses which follow this one. But itâs great youâre already doing your own research on the subject, and with some impressive results to show for it
⊠because this state variable owner is referenced in two places:
In the constructor â to set the contract ownerâs address on deployment
In the onlyOwner modifierâs require() statement â to check whether the function caller is the contract owner
If you remove the owner state variable, the compiler will throw errors in both of these places where it is referenced. To be able to remove it, you would have to also remove the constructor and the modifier, which, as far as the deposit(), withdraw() and transfer() functions are concerned, you could do.
Is this, perhaps, because you hadnât converted msg.sender to a payable address, as follows?
payable(msg.sender).transfer(amount);
In order to receive ether, an address needs to be payable . You need to bear in mind that this course material is based on Solidity v0.7 syntax. In versions prior to v0.8 msg.sender is payable by default and so doesnât need to be explicitly converted when transfer() is called on it. Thatâs why the course video uses â msg.sender.transfer(amount); â without the compiler throwing an error. However, you have chosen to use Solidity v0.8, and from v0.8 msg.sender is non-payable by default, and so when it needs to refer to a payable address, it needs to be explicitly converted. One of the ways to do this is with payable()
What follows is a list of issues which need addressing:
(1)âIâm not really sure why youâve added the extra mappingâethBalance. When users deposit ether into, or withdraw ether from, the contract, the ether itself is added to, or deducted from, the contract address balance. This contract balance represents the total ether belonging all account holders. The individual account balances in the mapping perform an internal accounting role, by keeping track of each individual account holderâs share of the total pooled funds. As a result of this, we can continue to use the same balance mapping for this purpose, and we can also leave the function names as deposit() withdraw() and transfer().
(2)âI also donât understand the purpose of your adminSetBalance() function or your modified getBalance() function. The only balances we need to manipulate within the contract are the individual account balances. Could you explain your thinking here?
(3)âYou have two deposit event declarations, but youâre only emitting one.
(4)
Let me know if anything is unclear, or if you have any further questions
I had some leftover code from previous exercices in here so the contract as is handles both an ethBalance tracker and a Balance tracker which isnt used in this instance of value transfering. going forward ill try to keep the code cleaner/more organized.
