Hi @MightyYak,
You have added the additional lines of code needed to solve the problem with the withdraw function
You are also right that it’s not necessary to return the updated balance, 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.
Yes, it does… This is good question, and a very important one in terms of smart contract security. Best practice is for the order of the statements within the function body to follow the Checks-Effects-Interactions Pattern:
- check inputs (require statements)
- effects (update the contract state)
- external interactions
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…
payable(msg.sender).transfer(amount);
… to guard against re-entrancy attacks. In this type of attack a malicious fallback function (in a receiving contract) would be able to make successful re-entrant calls to the withdraw() function, thereby allowing it to execute multiple withdrawals before its individual account balance is modified in the contract state to accurately reflect these operations (which would otherwise enable the initial require statement to fail and trigger revert).
In fact, the latest advice is not to use the address members transfer()
or send()
. 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.
Instead of transfer()
we can use the address member call()
to perform the transfer of ether from the contract address balance to the external address …
(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Transfer failed");
Using call()
would normally pose a greater risk of re-entrancy attacks than using transfer()
or send()
, because it will forward all of the remaining gas (up to the gas limit set by the caller) to the fallback function. However, implementing the Checks-Effects-Interactions Pattern will guard against this, as I’ve described above.
You will find more background information about these issues in the following articles:
https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
https://medium.com/coinmonks/protect-your-solidity-smart-contracts-from-reentrancy-attacks-9972c3af7c21
You’ll learn more about re-entrancy and other types of attacks, and some of coding techniques and smart-contract best practices we can implement to guard against them and reduce security risk, in the courses which follow this one: Ethereum Smart Contract Programming 201, and the Ethereum Smart Contract Security course. But it’s great that you’re already starting to consider these types of issues
Just let me know if you have any further questions.