Ahh… yeh, I thought that might be what you’d done, but I just wanted to check you weren’t trying to do something else.
So, adminSetBalance() is your adaptation of the original addBalance() function… with the contract owner now being able to set any user’s balance, and not just their own. And getBalance() now allows any user to view any user’s balance, and not just their own. Is that correct?
Did all my other comments make sense?
Really well done on all the additional research! I just wanted to confirm what you’d found out was correct, and also fill in a few of the gaps
Apart from a mistake with an assignment operator and a missing return statement (see comments below) you’ve added the necessary lines of code 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
Also, 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. You should have got a yellow/orange compiler warning for this.
Let me know if anything unclear, or if you have any questions
You have added all of the additional lines of code needed to solve the problem with the withdraw function. The additional two lines you’ve included for the assert() statement are also well coded.
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.
The Solidity documentation doesn’t seem to pin down what exactly msg can be defined as. Instead it categorises msg.sender and msg.value as special variables in their own right, and which are available in the global namespace. This might well be because msg on its own doesn’t access or reference any value. It only has any use when appended with .sender or .value as msg.sender or msg.value , for example. However, from other material and comments I’ve read, I think it makes sense to understand msg as an object which, within a specific function, represents the data that is sent to the Ethereum network to enable that specific function to be called and executed. Specific items of data, such as the address calling the function, or an amount of wei sent to the function, can be accessed/referenced with specific, pre-defined members of the msg object: msg.sender and msg.value . In this sense, I think you are right to think of msg as a standardised object, because it is not a data structure that we can define ourselves, like we can with a struct.
Whether we use the term member, property or attribute to refer to one of the pre-defined values available on the msg object, is, in my opinion, purely academic and arbitrary. The official Solidity term does seem to be member, but, as I’ve mentioned above, the documentation also describes these values as separate variables in their own right.
Solidity is a statically-typed programming language, and uses data types to declare the type of each variable and parameter. We can define our own custom data type with a struct, and then use the name/identifier of that struct to declare what the data structure of the values stored within certain variables within our smart contract should be; but we can’t use msg like we can other pre-defined data types in Solidity, such as string , uint256 , bool and address , to declare the data type of our variables or parameters, and that’s why I think we can consider struct names/identifiers to be data types, but not msg . Although, again, these distinctions are mainly academic. The main thing is that we understand what msg.sender , msg.value and struct names/identifiers represent and how they can be used within our code to achieve the results we want.
https://docs.soliditylang.org/en/v0.8.4/types.html#members-of-addresses
As you can see from this section of the Solidity documentation, the term member seems to encompass both of the concepts that you understand as attributes and methods. When referring to the address member balance, the documentation uses the term property, and when referring to transfer, it uses the term function. I’ve also seen developers refer to transfer and send as methods.
I think the most important thing is that we understand what role <address>.balance and <address payable>.transfer(uint256 amount) perform, and what exactly they enable us to do.
I hope these comments go some way towards answering your questions, or at least give you some further food for thought
Added require function and updated balance:
function withdraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount, “Balance not sufficient”);
balance[msg.sender]=-amount;
msg.sender.transfer(amount);
}
Thanks a lot for that, that does help clear things up - or at least let me square my current understanding from Python with Solidity. Much appreciated!
Like some others, I went a bit further investigating things.
For me reading docs on the receive function of contracts was essential to understanding the docs on the reentrantcy problem, the big take-away being that payable function functions like transfer and call can execute code in the receiving contract as well as doing the transaction.
Notes:
Using solidity 0.8.4 so msg.sender must be explicitly marked payable.
The SimpleRecieve contract has no problems with withdraw function, acts the same as a Externally Owned Address.
The ComplexRecieve executes extra logic so it always runs out of gas when the transfer function is used.
How to operate the contracts in Remix:
Deploy all three contracts using the drop down to select each one in turn.
You can deposit to the Bank by putting a number in VALUE in Remix then pressing the deposit button for the contract.
You can add funds to the other 2 contracts by putting a number in the VALUE field and pressing Transact
If you enter a number in VALUE and then press Transact on the Bank it will fail, the Bank has no receive function.
Copy the address of the Bank and paste that into the functions of the contracts (followed by a comma and a number for amount).
You can check the ContractBalance on all the contracts to see the changes when using each function.
//SPDX-License-Identifier: 0BSD
pragma solidity 0.8.4;
contract Bank {
event depositDone(uint value, address sender);
mapping(address => uint) balance;
function getBalance() public view returns(uint){
return balance[msg.sender];
}
function contractBalance() public view returns(uint){
return address(this).balance;
}
function deposit() public payable returns (uint) {
balance[msg.sender] += msg.value;
emit depositDone(msg.value, msg.sender);
return balance[msg.sender];
}
// The transfer has a fixed gas limit of 2300, a contract may have additional receive logic and exceed the gas limit.
function withdrawViaTransfer(uint amount) public {
require(balance[msg.sender] >= amount, "Insufficient balance");
balance[msg.sender] -= amount;
// The require and balance update must be before the transfer for security
// Needs explicit payable modifier for solidity >= 0.8
payable(msg.sender).transfer(amount);
}
// Uses the call
function withdrawViaCall(uint amount) public {
require(balance[msg.sender] >= amount, "Insufficient balance");
balance[msg.sender] -= amount;
// The require and balance update must be before the call for security
// Needs explicit payable modifier for solidity >= 0.8
(bool success, ) = payable(msg.sender).call{value:amount}("");
if(!success) revert("transfer failed");
}
}
contract ComplexReceive {
event Received(address, uint);
uint callCounter = 1;
receive() external payable {
callCounter ++; // This should use too much gas when triggered inside a msg.sender.transfer call.
emit Received(msg.sender, msg.value);
}
function contractBalance() public view returns(uint){
return address(this).balance;
}
function depositToBank(address bank, uint amount) public {
Bank(bank).deposit{value:amount}();
}
function transferWithdawFromBank(address bank, uint amount) public {
Bank(bank).withdrawViaTransfer(amount);
}
function callWithdawFromBank(address bank, uint amount) public {
Bank(bank).withdrawViaCall(amount);
}
}
contract SimpleReceive {
event Received(address, uint);
receive() external payable {
emit Received(msg.sender, msg.value);
}
function contractBalance() public view returns(uint){
return address(this).balance;
}
function depositToBank(address bank, uint amount) public {
Bank(bank).deposit{value:amount}();
}
function transferWithdawFromBank(address bank, uint amount) public {
Bank(bank).withdrawViaTransfer(amount);
}
function callWithdawFromBank(address bank, uint amount) public {
Bank(bank).withdrawViaCall(amount);
}
}
Apart from a mistake with an assignment operator and a missing return statement (see comments below) you’ve added the necessary lines of code 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
Also, 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. You should have got a yellow/orange compiler warning for this.
Let me know if anything unclear, or if you have any questions
Yes… it ensures that the amount deducted from the contract balance using… msg.sender.transfer(amount);
… is also deducted from the individual account holder’s balance in the mapping.
After the withdrawal, the contract balance will reflect the total amount of ether held in the contract (i.e.the total of all account holders’ funds pooled together). So, adjusting the individual’s balance in the mapping by the same amount is an essential accounting adjustment that enables us to track their share of the total pooled funds, and therefore the maximum amount they are entitled to withdraw from the 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
Just one minor observation…
You only need the parentheses, here, when returning more than one value.
As I mentioned before, your withdraw function is correct. 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.
Did you manage to resolve the problem you were having with the balance not updating after calling withdraw()?
Your transfer event and corresponding emit statement are both well coded, and the emit statement will log relevant information when the function has executed successfully. I would just add, though, that in general, emit statements are probably better placed after assert statements, rather than before.
The withdrawal event declaration you’ve added is also well coded, but you aren’t emitting this event…?
function withdraw(uint amount) public returns (uint) {
// check that the function caller address has more than the amount.
require(balance[msg.sender] >= amount, "Not enough funds");
// if requirement is met, the function caller address has the amount deducted from their balance
balance[msg.sender] -= amount;
// actual transfer from function caller address
msg.sender.transfer(amount);
// updated balance is returned
return balance[msg.sender];
}
I understand the logic a bit better now, though my original had the security error @jon_m outlined previously (having my external interaction before updating the contract)
I can’t find the error message “Not enough funds” when I attempt to withdraw more than has been deposited (the withdrawal fails, just no message or I am not looking in the right place).
This is an excellent assignment solution, analysis, and piece of research into how the address members transfer() and call() interact differently with external contracts
And thank you for sharing it in such a well-documented way
You have included all of the additional code needed to solve the problem with the withdraw function, and the order of the statements within the function body also follows the recommended Checks-Effects-Interactions pattern to reduce the risk of a re-entrancy attack.
During your research, you may already have discovered that the latest advice is not to use the address members transfer() or send() after the Istanbul hard fork. This is because the fixed stipend of 2300 gas, which they forward to a calling contract along with the transfer value, used to be enough gas for a fallback function to execute successfully, but not enough gas to allow a re-entrant call (thereby reducing the risk of re-entrancy attacks); and you have demonstrated this very well with the contracts you have posted However, since the Istanbul hard fork changed the gas charges for certain operations, this fixed gas stipend is no longer guaranteed to be sufficient. As the following article explains, as well as helping to prevent re-entrancy attacks, using transfer() or send() now runs the risk of breaking some contracts.
Instead of using transfer() or send() , the recommendation now seems to be to use the address member call() . So it’s great that you’ve coded two alternatives (with transfer and call) to perform the transfer of ether from the Bank contract to either an external wallet address or an external contract address. And because call forwards all remaining gas to the calling contract, and doesn’t limit it in the same way transfer does, when using call it is especially important to implement another robust and effective guard against re-entrancy, such as the Checks-Effects-Interactions pattern.
A couple of comments about your implementation of call …
Unlike with transfer , the address doesn’t need to be payable.
I think using require() instead of if() and revert(), is cleaner, more concise, and more readable.
This is also what’s recommended in the above article I’ve linked to … and that’s been written by an advisor at ConsenSys who works on smart contract audits, so I’ll go with his advice.
You are also right that it’s not necessary to return the updated balance from the withdraw() function, and so returns(uint) can be removed from the function header. It is useful information to return, but we do already have the getBalance function which can be called separately to consult the individual account holder’s updated balance after the withdrawal.
