They’re here and they’re here to stay, so let’s learn to design around them.

WTF is a reentrancy attack?

I agree that the name sounds kind of dumb, I would call it recursive call attacks if I could choose a name for it. Maybe recursive withdrawal attack? How about nested function call attack?

Anyway, the name doesn't matter - let's talk about single function reentrancy and cross function reentrancy. I will not cover cross contract reentrancy yet as I lack the knowledge to cover it in depth yet.

Simple / single function reentrancy attacks

Let's look at simple reentrancy:

//This will pay the zero address.
address payable user = address(0);
uint256 balance = 100;
function withdraw(uint256 amt_){
	require(balance>0);
	user.transfer(amt_); //Send the user 1
	balance-=amt_;
}

Ignore syntax mistakes here, this is just to show you what something like this looks like.

Let's call this function 100 times.

In some cases, user.transfer() can occur multiple times before the balance is decreased (withdrawing twice from the contract). This example might be a bit poor because transfer has a fixed gas limit of 2300 which generally can't be abused by a contract which receives Eth, but user.send() could definitely have this effect. The receiving contract just needs to implement a fallback function which calls withdraw() again to abuse this. This is called a simple reentrancy attack.

Preventing this

We can work towards preventing this in a variety of ways:

1. Decrease the balance before we do the transfer. This is called the checks-effects-interactions design pattern

//This will pay the zero address.
address payable user = address(0);
uint256 balance = 100;
function withdraw(uint256 amt_){
	require(balance>0);
	balance-=amt_;
	user.transfer(amt_); //Send the user 1
}

1. Add a lock (or mutex as the solidity docs refer to it) variable so that the function literally cannot be called concurrently during the same call. Let's implement the noReentrancy modifier that the Solidity docs uses.

//This will prevent simple reentrancy attacks
bool locked;  
modifier noReentrancy() {  
	require(  
		!locked,  
		"Reentrant call."  
	);  
	locked = true;  
	_;  
	locked = false;  
}

//This will pay the zero address.
address payable user = address(0);
uint256 balance = 100;
function withdraw(uint256 amt_) noReentrancy {
	require(balance>0);
	balance-=amt_;
	user.transfer(amt_); //Send the user 1
}

OpenZeppelin has an implementation of this called ReentrancyGuard which I would recommend using over trying to build your own implementation. NoReentrant functions using this should be external.

Aside: WTF does _; do?

Good question. Solidity docs 0.8.13 doesn't really cover this in enough depth for my liking. Let me give you a tldr so you don't have to google it.

Modifier X calls function Y. _; is where function Y runs within the modifier X. Think of it like calling Y(); but in a generic (non-name-specific) fashion.

1. For extreme protection we can implement a solution proposed by @ThomasBertani's which disallow calling the function twice in the same call by comparing values of gasleft() (when the slot is warm, it cannot be called again). This is extreme since it means no implementation of your contract will be able to have anything with that modifier called twice. @transmissions11 does not recommend this for most cases but it seems to be the most comprehensive method to prevent simple reentrancy.

Cross-function reentrancy attacks

NOTE: "Cross reentrancy attack" could refer to this or cross-contract

If stopping reentrancy is that simple, then why are attacks still working?

Let me introduce you to the cross reentrancy attack. When two functions share the same state (a variable usually), this is possible. Let's imagine a withdrawEth() function for one type of user (as is being implemented in my escrow contract) and a withdrawEthWithDecision() function (which is called under a different set of circumstances).

Hypothetically (and in reality) an attacking contract can invoke a change in state by calling one function (which is not reentrancy protected) which does not update the state in time for the other (concurrent) call to drain funds. All it takes is one function which shares state and isn't reentrancy protected properly.

Making sure that functions which affect state have proper protections against reentrancy is necessary to prevent this. Using a bunch of single-function reentrancy protections to make sure that nested / recursive calls to a function which changes state can't be made is usually enough - the crux of these attacks is always the state not updating at the time the effects take place.

Where there are multiple functions, setting up a system for untrusted functions (which call unknown contracts) is good practise (this is by Consensys, however note that Consensys have their own guidelines which suggest only using call() rather than transfer() or send(), however this is not for reentrancy prevention reasons and has more to do with the 2300 gas stipend not being enough for some fallback functions to execute fully after EIP-1884 which is a separate issue altogether).

Cross-contract reentrancy

This is like the cross-function reentrancy attack, but when state is shared across contracts. I am not informed enough yet to comment on ways to prevent this type of attack, so I will refer you to better resources.

More complexity in a system leads to more surface area for attacks. Keeping things simple is often better than a complex setup for the sake of it (which can get attacked in many ways).

Transfer vs Send

Solidity Docs actually recommend using transfer() over send() as a way of making sure that transfers failing does not expose your contract to abuse (or result in loss of funds).

The 2300 gas stipend is a good feature to try and prevent reentrancy attacks but it is not enough alone.

My recommendation is literally not to use call() unless transfer() or send() cannot accomplish the purpose of the action you're trying to perform. call() is super vulnerable since it forwards all remaining gas to the contract is calls but is often needed when the 2300 gas stipend isn't enough to execute fallback functions (or just for calls which needs more gas).

Conclusion

Although some will say that the check-effects-interactions design pattern alone is enough to prevent damage from reentrancy attacks, I am a believer in mutex and lock modifiers in addition to this.

Keep your code simple and easy to mentally follow and things should generally be fine - thinking as if you are an attacker while you develop is also a good way to avoid (human-error caused) design pitfalls which could be exploited later.

Aside: do audits mean nothing?

In my opinion, pretty much - when you dangle $10m in front of math and computer nerds, they're going to outperform a $25k audit given enough effort. If an auditor had code with reentrancy vulnerabilities in there at the time when they audited it and didn't catch it, they screwed up. Everyone has screwed up - Certik, Quantstamp, Peckshield and OpenZeppelin have all had contracts they audited be exploited. Saddle Finance (who just got hit) had three of those companies audits completed.

The security industry doesn't expect pen testing to be secure over time as network and physical attacks evolve over time, though here we are talking about people's money. I am not a believer of audits being any better or worse than having your code reviewed by a developer. I am not a believer of anything certified being hack-proof whatsoever. My heart goes out to Saddle and others who have been hit by these attacks - the fact that they paid for multiple audits by big name firms shows that their intentions are good and I hope that they recover. I am personally a fan of crowdsourced-style audits which I feel give the same pool of attackers a chance to make some money (whereas giving a firm an audit contract limits who can earn money off it) but again nothing is perfect.

We can only try our best, there are no guarantees.