From 7b2ab0a158b363acd8be8d3b01117d13d4e3d05d Mon Sep 17 00:00:00 2001
From: Igor Yalovoy <to@ylv.io>
Date: Mon, 5 Jun 2023 14:56:28 -0600
Subject: [PATCH] Avoid wasting gas on vault reentrancy check (#2467)

---
 .../contracts/lib/VaultReentrancyLib.sol           | 14 ++++++++------
 pkg/pool-utils/test/VaultReentrancyLib.test.ts     |  8 ++++++++
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/pkg/pool-utils/contracts/lib/VaultReentrancyLib.sol b/pkg/pool-utils/contracts/lib/VaultReentrancyLib.sol
index 9f8336d868..f3b5c2be98 100644
--- a/pkg/pool-utils/contracts/lib/VaultReentrancyLib.sol
+++ b/pkg/pool-utils/contracts/lib/VaultReentrancyLib.sol
@@ -45,12 +45,14 @@ library VaultReentrancyLib {
         // view.
         //
         // This staticcall always reverts, but we need to make sure it doesn't fail due to a re-entrancy attack.
-        // Staticcalls consume all gas forwarded to them on a revert. By default, almost the entire available gas
-        // is forwarded to the staticcall, causing the entire call to revert with an 'out of gas' error.
+        // Staticcalls consume all gas forwarded to them on a revert caused by storage modification.
+        // By default, almost the entire available gas is forwarded to the staticcall,
+        // causing the entire call to revert with an 'out of gas' error.
         //
-        // We set the gas limit to 100k, but the exact number doesn't matter because view calls are free, and non-view
-        // calls won't waste the entire gas limit on a revert. `manageUserBalance` is a non-reentrant function in the
-        // Vault, so calling it invokes `_enterNonReentrant` in the `ReentrancyGuard` contract, reproduced here:
+        // We set the gas limit to 10k for the staticcall to
+        // avoid wasting gas when it reverts due to storage modification.
+        // `manageUserBalance` is a non-reentrant function in the Vault, so calling it invokes `_enterNonReentrant`
+        // in the `ReentrancyGuard` contract, reproduced here:
         //
         //    function _enterNonReentrant() private {
         //        // If the Vault is actually being reentered, it will revert in the first line, at the `_require` that
@@ -73,7 +75,7 @@ library VaultReentrancyLib {
         // `manageUserBalance` even gets called), any other error would generate non-zero revertData, so checking for
         // empty data guards against this case too.
 
-        (, bytes memory revertData) = address(vault).staticcall{ gas: 100_000 }(
+        (, bytes memory revertData) = address(vault).staticcall{ gas: 10_000 }(
             abi.encodeWithSelector(vault.manageUserBalance.selector, 0)
         );
 
diff --git a/pkg/pool-utils/test/VaultReentrancyLib.test.ts b/pkg/pool-utils/test/VaultReentrancyLib.test.ts
index 5dfcba8148..f72215a5fb 100644
--- a/pkg/pool-utils/test/VaultReentrancyLib.test.ts
+++ b/pkg/pool-utils/test/VaultReentrancyLib.test.ts
@@ -188,6 +188,14 @@ describe('VaultReentrancyLib', function () {
 
         expectEvent.inReceipt(receipt, 'ProtectedFunctionCalled');
       });
+
+      it('can call a protected view function outside the Vault context', async () => {
+        await expect(pool.protectedViewFunction()).to.not.be.reverted;
+      });
+
+      it('do not waste gas', async () => {
+        await expect(pool.protectedFunction({ gasLimit: 40000 })).to.not.be.reverted;
+      });
     });
   });