Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid wasting gas on vault reentrancy check #2467

Merged
merged 4 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions pkg/pool-utils/contracts/lib/VaultReentrancyLib.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
);

Expand Down
8 changes: 8 additions & 0 deletions pkg/pool-utils/test/VaultReentrancyLib.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
});
});

Expand Down