clean up

patterns/flash-loans/FlashLoanPool.sol

@@ -24,9 +24,9 @@ interface IBorrower {
     ) external;
 }
 
-
+// A simple flash loan protocol with a single depositor/withdrawer (OWNER).
 contract FlashLoanPool {
-    uint16 public constant FEE_BPS = 0.001e4;
+    uint16 public constant FEE_BPS = 0.001e4; // 0.1% fee.
     address public immutable OWNER;
 
     constructor(address owner) { OWNER = owner; }
@@ -61,7 +61,7 @@ contract FlashLoanPool {
         );
         // Check that all the tokens were returned + fee.
         uint256 balanceAfter = token.balanceOf(address(this));
-        require(balanceAfter == balanceBefore + fee, 'not repaid');
+        require(balanceAfter >= balanceBefore + fee, 'not repaid');
     }
 
     // Withdraw tokens from this contract to the contract owner.

patterns/flash-loans/README.md

@@ -10,18 +10,20 @@ Here we'll explore creating a basic flash loan protocol to illustrate the concep
 ## Anatomy of a Flash Loan
 
 At their core, flash loans are actually fairly simple, following this typical flow:
-    1. Transfer loaned assets to a user-specified borrower contract.
-    2. Call a handler function on the borrower contract.
-        1. Let the borrower contract perform whatever actions it needs to do with those assets.
-    3. After the borrower's handler function returns, verify that all of the borrowed assets have been returned + some extra as fee.
+
+1. Transfer loaned assets to a user-provided borrower contract.
+2. Call a handler function on the borrower contract to hand over execution control.
+    1. Let the borrower contract perform whatever actions it needs to do with those assets.
+3. After the borrower's handler function returns, verify that all of the borrowed assets have been returned + some extra as fee.
+
 
 ![flash loan flow](./flash-loan-flow.drawio.svg)
 
-The entirety of the loan occurs inside of the call to the loan function. If the borrower fails to return the assets (+ fee) by the time their logic completes, the entire call frame reverts and it will be as if the loan and the actions performed with it never happened, exposing no assets to risk. It's this lack of risk that drives the fee associated with flash loans down.
+The entirety of the loan occurs inside of the call to the loan function. If the borrower fails to return the assets (+ fee) by the time their logic completes, the entire call frame reverts and it will be as if the loan and the actions performed within never happened, exposing no assets to any™️ risk. It's this lack of risk that helps drive the fee associated with flash loans down.
 
 ## A Simple FLash Loan Protocol
 
-Let's write a simple ERC20 pool contract owned and funded by a single entity. Borrowers can come along and take a flash loan against the pool's tokens, earning a small fee along the way and increasing the total value of the pool. For additional simplicity, this contract will only support [compliant](../erc20-compatibility/) ERC20 tokens that don't take fees on transfer.
+Let's write a super simple ERC20 pool contract owned and funded by a single entity. Borrowers can come along and take a flash loan against the pool's tokens, earning a small fee along the way and increasing the total value of the pool. For additional simplicity, this contract will only support [compliant](../erc20-compatibility/) ERC20 tokens that don't take fees on transfer.
 
 We're looking at the following minimal interfaces for this protocol:
 
@@ -40,7 +42,7 @@ interface IFLashLoanPool {
         bytes calldata data
     ) external;
 
-    // Withdraw tokens to the contract owner.
+    // Withdraw tokens held by this contract to the contract owner.
     function withdraw(IERC20 token, uint256 amount) external;
 }
 
@@ -61,23 +63,15 @@ interface IBorrower {
 }
 ```
 
-Let's flesh out `floashLoan()`, which is really all we need to have a functioning flash loan protocol. It needs to track the token balances, transfer tokens to the borrower, hand over execution control to the borrower, then verify assets were returned. We'll use the constant `FEE_BPS` to define the flash loan fee in BPS.
+Let's immediately flesh out `flashLoan()`, which is really all we need to have a functioning flash loan protocol. It needs to 1) track the token balances, 2) transfer tokens to the borrower, 3) hand over execution control to the borrower, then 4) verify all the assets were returned. We'll use the constant `FEE_BPS` to define the flash loan fee in BPS (e.g., `1% == 0.01e4`).
 
 ```solidity
-function onFlashLoan(
-    // Who called `flashLoan()`.
-    address operator,
-    // Token borrowed.
+function flashLoan(
     IERC20 token,
-    // Amount of tokens borrowed.
-    uint256 amount,
-    // Extra tokens (on top of `amount`) to return as the loan fee.
-    uint256 fee,
-    // Arbitrary data passed into `flashLoan()`.
+    uint256 borrowAmount,
+    IBorrower borrower,
     bytes calldata data
-)
-    external
-{
+) external {
     // Snapshot our token balance before the transfer.
     uint256 balanceBefore = token.balanceOf(address(this));
     require(balanceBefore >= borrowAmount, 'too much');
@@ -95,20 +89,20 @@ function onFlashLoan(
     );
     // Check that all the tokens were returned + fee.
     uint256 balanceAfter = token.balanceOf(address(this));
-    require(balanceAfter == balanceBefore + fee, 'not repaid');
+    require(balanceAfter >= balanceBefore + fee, 'not repaid');
 }
 ```
 
-The `withdraw()` function is trivial to implement so we'll omit it from this guide, but you can see the full, functional contract [here](./FlashLoanPool.sol).
+The `withdraw()` function is trivial to implement so we'll omit it from this guide, but you can see the complete contract [here](./FlashLoanPool.sol).
 
 ## Security Considerations
 
-Implementing flash loans might have seemed really simple but usually flash loans are added on top of an existing, more complex product. For example, Aave, Dydx, and Uniswap all have flash loan capabilities added to their lending and exchange products. The transfer-and-call pattern used by flash loans creates a huge opportunity for [reentrancy](../reentrancy/) and price manipulation attacks when in the context of even low complexity protocols. 
+Implementing flash loans might have seemed really simple but usually they're added on top of an existing, more complex product. For example, Aave, Dydx, and Uniswap all have flash loan capabilities added to their lending and exchange products. The transfer-and-call pattern used by flash loans creates a huge opportunity for [reentrancy](../reentrancy/) and price manipulation attacks when in the setting of even a small protocol.
 
-For instance, let's say we took the natural progression of our toy example and allowed anyone to deposit assets, granting them shares that entitles them to a proportion of generated fees. Now we would have to wonder what could happen if the flash loan borrower re-deposited borrowed assets into the pool. Without proper safeguards, it's very possible that we could double count these assets and the borrower would be able to inflate the size/value of their own shares and then drain all the assets out of the pool after the flash loan operation!
+For instance, let's say we took the natural progression of our toy example and allowed anyone to deposit assets, granting them shares that entitles them to a proportion of generated fees. Now we would have to wonder what could happen if the flash loan borrower re-deposited borrowed assets into the pool. Without proper safeguards, it's very possible that we could double count these assets and the borrower would be able to unfairly inflate the number of their own shares and then drain all the assets out of the pool after the flash loan operation!
 
 Extreme care has to be taken any time you do any kind of arbitrary function callback, but especially if there's value associated with it.
 
 ## Test Demo: DEX Arbitrage Borrower
 
-Check the [tests](../../test/FlashLoanPool.t.sol) for an illustration of how a user would use our flash loan feature. There you'll find the a borrower contract designed to perform arbitrary swap operations across different DEXes to capture a zero-capital arbitrage opportunity, with profits going to the operator.
+Check the [tests](../../test/FlashLoanPool.t.sol) for an illustration of how a user would use our flash loan feature. There you'll find a fun borrower contract designed to perform arbitrary swap operations across different DEXes to capture a zero-capital arbitrage opportunity, with profits split between the operator and fee.

patterns/reentrancy/README.md

@@ -3,20 +3,20 @@
 - [📜 Example Code](./AppleDAO.sol)
 - [🐞 Tests](../../test/AppleDAO.t.sol)
 
-Virtually all protocol contracts will make some form of an external call, either directly or indirectly, to an untrusted, uncontrolled address. Any time an external call is made, execution control is lost to another party, which may be untrusted or unknowable. There is no concept of parallelism in Ethereum contracts, so when a protocol loses execution control in the middle of an operation that has yet to finish, it must wait for the executor to return and opens itself up to the notorious reentrancy attack.
+Virtually all protocol contracts will make some form of an external call, either directly or indirectly, to an untrusted, uncontrolled address. Any time an external call is made, execution control is lost to another party. There is no concept of parallelism in Ethereum contracts, so when a protocol loses execution control in the middle of an operation that has yet to finish it must wait for the executor to return and opens itself up to the notorious reentrancy attack.
 
-External calls can come in the obvious form of a simple call to a function on a contract, the less obvious transfer of ETH to an address (which is just an empty call), or the extra sneaky call to a trusted contract that implements a callback or hook mechanism (such as ERC777 and ERC721 tokens).
+External calls can come in the obvious form of a simple call to a function on a contract, the less obvious transfer of ETH to an address (which is just an empty call), deploying a contract, or the transfer of tokens with a callback or hook mechanism (such as ERC777 and ERC721).
 
 ## Losing All Your Apples
 
-Imagine your protocol is Alice, who is handing out apples to people, but only one per person. Unfortunately, Alice is also suffering from short-term memory loss and must write everything down to remember it. Now Bob comes along, who is greedy and actually wants *2 apples*! How can he trick Alice into getting them?
+Imagine your protocol is Alice, who is handing out apples to people, but only one per person. Unfortunately Alice is also suffering from short-term memory loss and must write everything down to remember it. Now greedy Bob comes along, who actually wants *2 apples*! How can he trick Alice into getting them?
 
 1. Bob asks Alice for an apple.
 2. Alice checks her notebook to see if she's already given Bob an apple, sees that she hasn't, and hands over an apple to Bob.
-    1. Before Alice can write down that she has given Bob an apple (!), Bob immediately asks Alice for another apple.
+    1. *Before* Alice can write down that she has given Bob an apple, Bob immediately asks Alice for another apple.
     2. Alice again checks her notebook and sees Bob hasn't received an apple, so she hands him another.
-    3. Alice now checks Bob's name off in her notebook, indicating he's received an apple.
-3. Alice (again) checks Bob's name off in her notebook, indicating he's received an apple.
+    3. Alice now crosses Bob's name off in her notebook, indicating he's received an apple.
+3. Alice (again) crosses Bob's name off in her notebook, indicating he's received an apple.
 
 Taking it further, if Bob wanted ALL of Alice's apples, he could simply keep nesting requests for apples before Alice gets a chance to record the exchange until she ran out. This is exactly how the infamous [DAO hack](https://www.immunebytes.com/blog/an-insight-into-the-dao-attack/) was carried out.
 
@@ -57,22 +57,23 @@ contract Bob {
 
 ```
 
-*⚠️ Note that this is just one, relatively simple, example of what a reentrancy attack could look like. The actual topology can vary greatly, involving more or less intermediary contracts/actors. It's also important to be aware that reentrancy is often spoken about at the individual contract level, but it can manifest itself at the protocol level when an operation spans multiple contracts (or even multiple protocols). Reentrancy attacks can also (and often does) exploit multiple functions/operations that expect the same shared state to be consistent.*
-
+*⚠️ Note that this is just one, relatively simple example of what a reentrancy attack could look like. The actual topology can vary greatly, involving more or less intermediary contracts/actors. It's also important to be aware that reentrancy is often spoken about at the individual contract level, but it can also manifest itself at the protocol level when an operation spans multiple contracts (or even multiple protocols). Reentrancy attacks can also (and often does) exploit multiple functions/operations that rely on some shared state.*
 
 ## Protecting Your Apples
 
-Let's see how we can apply two common patterns/mechanisms to help keep Alice from being exploited.
+Let's see how we can apply two common patterns/mechanisms to help keep Alice from getting exploited.
 
 ### Checks-Effects-Interactions Pattern
-The "Checks-Effects-Interactions" (abbreviated to just "CEI") pattern is basically a mantra for organizing the logic in your operation to minimize reentrancy opportunities. It also often has a nice side effect of making your code easier to follow, which helps with security reviews, so you should considering applying the technique even when you're confident the impact of a reentrancy attack is negligible. Most seasoned solidity devs do it by reflex.
+The "Checks-Effects-Interactions" (abbreviated to just "CEI") pattern is a mantra for organizing the logic in your operation to minimize reentrancy opportunities. It also often has a nice side effect of making your code easier to follow, so you should strongly consider applying the technique even when you're confident that the impact of a reentrancy attack is negligible. Most seasoned solidity devs do it by reflex now.
 
 The sequence goes as follows:
 
 1. **Checks**: Verify inputs, access control, and initial state for the function/operation.
 2. **Effects**: Perform any internal accounting and commit any state changes to storage that would be affected by the operation.
 3. **Interactions**: Make untrusted/external calls and asset transfers.
 
+By placing the external call at the end of your logic, you can avoid being in an incomplete state when you hand over execution control.
+
 If we look at the Alice example, she actually does all these things but in the wrong order! Instead of C-E-I, she does C-I-E. Correcting the order removes the reentrancy vulnerability because she will have already recorded that Bob received an apple before he gets the opportunity to request another.
 
 ```solidity
@@ -93,7 +94,9 @@ contract Alice {
 ### Reentrancy Guards (Mutex)
 Sometimes you can't organize your code according to CEI. Maybe you depend on the output of an external interaction to compute the final state to be committed. In these cases, you can use some form of a reentrancy guard.
 
-Reentrancy guards are essentially temporary state that indicates an operation is ongoing, which you can check to prevent two mutually exclusive operations (or the same operation) from occurring before the first one has completed. Many contracts use a dedicated storage variable as this mutex (see the [standard OpenZeppelin implementation](https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard)) and share it across any at-risk functions, usually in the form of a convenient modifier that asserts the reentrancy flag, toggles on the flag, executes the function body, then resets the flag.
+Reentrancy guards are essentially temporary state that indicates an operation is ongoing, which you can check to prevent two mutually exclusive operations (or the same operation) from occurring before the first one has completed. Many contracts use a dedicated storage variable as this mutex (see the [standard OpenZeppelin implementation](https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard)) and share it across any at-risk functions. Often reentrancy guards are wrapped in a modifier that asserts the state flag, toggles on the flag, executes the function body, then resets the flag.
+
+Here is Alice with a reentrancy guard:
 
 
 ```solidity
@@ -119,13 +122,13 @@ contract Alice {
 }
 ```
 
-Now if Bob attempts to call `claimApple()` again inside of another, the modifier will see that the reentrancy guard is activated and the call will revert.
+Now if Bob attempts to call `claimApple()` again before it has completed the modifier will see that the reentrancy guard is activated and the call will revert.
 
-The reentrancy guard approach is pretty convenient and easy to reason about, which makes it a very popular solution. However, it comes with some considerations.
+The reentrancy guard approach is pretty convenient and takes much less thought to apply, which makes it a very popular solution. However, it comes with some considerations.
 
-- Writing to a new storage slot (especially an empty one) introduces significant gas cost. Even though the majority of it will be refunded (because the slot is reset by the modifier), it raises the execution gas limit of the transaction which causes some extra sticker shock to users.
+- The reentrancy flag usually occupies its own storage slot. Writing to a new storage slot (especially an empty one) introduces significant gas cost. Even though the majority of it will be refunded (because the slot is reset by the modifier), it raises the execution gas limit of the transaction which causes some extra sticker shock to users.
     - Sometimes you can avoid using a dedicated reentrancy guard state variable. Instead you can reuse a state variable that you would write to during the operation anyway, checking and setting it to some preordained invalid value that would act the same way a dedicated reentrancy guard would.
-- The naive version of a reentrancy guard can only protect reentrancy within a single contract. Protocols are often composed of several contracts with mutually exclusive operations across them. In these situations, you may need to come up with a way to share reentrancy guard state with the rest of the system.
+- The naive version of a reentrancy guard can only protect reentrancy within a single contract. Protocols are often composed of several contracts with mutually exclusive operations across them. In these situations, you may need to come up with a way to surface the reentrancy guard state across the rest of the system.
 
-## The Demo
+## Demo
 The [demo](./AppleDAO.sol) is the complete implementation of the scenario and solutions described here. An abridged and simplified version of an ERC721 style token contract is used for brevity. You can inspect the traces of the [tests](.../../test/AppleDAO.t.sol) with `forge test -vvvv --match-path test/AppleDAO.t.sol` to get a better understanding of the flow of execution.