From e198c5541a7448a4c45fff4305e374e4516f4a45 Mon Sep 17 00:00:00 2001
From: saucepoint <98790946+saucepoint@users.noreply.github.com>
Date: Tue, 3 Sep 2024 16:57:01 -0400
Subject: [PATCH] OZ-L06: try-catch permit2Forwarder to avoid exposing DOS on
multicall (#309)
* try-catch permit2Forwarder to avoid exposing DOS on multicall
* fix typo
* pr feedback nits
* return error and assert the permit error
---
src/base/Permit2Forwarder.sol | 16 +-
.../PositionManager.multicall.t.sol | 146 +++++++++++++++++-
2 files changed, 159 insertions(+), 3 deletions(-)
diff --git a/src/base/Permit2Forwarder.sol b/src/base/Permit2Forwarder.sol
index d6f28ccce..12e378aae 100644
--- a/src/base/Permit2Forwarder.sol
+++ b/src/base/Permit2Forwarder.sol
@@ -8,6 +8,8 @@ import {IAllowanceTransfer} from "permit2/src/interfaces/IAllowanceTransfer.sol"
contract Permit2Forwarder {
IAllowanceTransfer public immutable permit2;
+ error Wrap__Permit2Reverted(address _permit2, bytes reason);
+
constructor(IAllowanceTransfer _permit2) {
permit2 = _permit2;
}
@@ -17,8 +19,13 @@ contract Permit2Forwarder {
function permit(address owner, IAllowanceTransfer.PermitSingle calldata permitSingle, bytes calldata signature)
external
payable
+ returns (bytes memory err)
{
- permit2.permit(owner, permitSingle, signature);
+ // use try/catch in case an actor front-runs the permit, which would DOS multicalls
+ try permit2.permit(owner, permitSingle, signature) {}
+ catch (bytes memory reason) {
+ err = abi.encodeWithSelector(Wrap__Permit2Reverted.selector, address(permit2), reason);
+ }
}
/// @notice allows forwarding batch permits to permit2
@@ -26,7 +33,12 @@ contract Permit2Forwarder {
function permitBatch(address owner, IAllowanceTransfer.PermitBatch calldata _permitBatch, bytes calldata signature)
external
payable
+ returns (bytes memory err)
{
- permit2.permit(owner, _permitBatch, signature);
+ // use try/catch in case an actor front-runs the permit, which would DOS multicalls
+ try permit2.permit(owner, _permitBatch, signature) {}
+ catch (bytes memory reason) {
+ err = abi.encodeWithSelector(Wrap__Permit2Reverted.selector, address(permit2), reason);
+ }
}
}
diff --git a/test/position-managers/PositionManager.multicall.t.sol b/test/position-managers/PositionManager.multicall.t.sol
index 73c7e6587..9264c28e6 100644
--- a/test/position-managers/PositionManager.multicall.t.sol
+++ b/test/position-managers/PositionManager.multicall.t.sol
@@ -46,6 +46,8 @@ contract PositionManagerMulticallTest is Test, Permit2SignatureHelpers, PosmTest
address bob;
// bob used for permit2 signature tests
uint256 bobPK;
+ address charlie; // charlie will NOT approve posm in setup()
+ uint256 charliePK;
Permit2Forwarder permit2Forwarder;
@@ -54,6 +56,9 @@ contract PositionManagerMulticallTest is Test, Permit2SignatureHelpers, PosmTest
uint48 permitExpiration = uint48(block.timestamp + 10e18);
uint48 permitNonce = 0;
+ // redefine error from permit2/src/PermitErrors.sol since its hard-pinned to a solidity version
+ error InvalidNonce();
+
bytes32 PERMIT2_DOMAIN_SEPARATOR;
PositionConfig config;
@@ -61,6 +66,7 @@ contract PositionManagerMulticallTest is Test, Permit2SignatureHelpers, PosmTest
function setUp() public {
(alice, alicePK) = makeAddrAndKey("ALICE");
(bob, bobPK) = makeAddrAndKey("BOB");
+ (charlie, charliePK) = makeAddrAndKey("CHARLIE");
deployFreshManagerAndRouters();
deployMintAndApprove2Currencies();
@@ -78,6 +84,13 @@ contract PositionManagerMulticallTest is Test, Permit2SignatureHelpers, PosmTest
seedBalance(bob);
approvePosmFor(bob);
+
+ // do not approve posm for charlie, but approve permit2 for allowance transfer
+ seedBalance(charlie);
+ vm.startPrank(charlie);
+ IERC20(Currency.unwrap(currency0)).approve(address(permit2), type(uint256).max);
+ IERC20(Currency.unwrap(currency1)).approve(address(permit2), type(uint256).max);
+ vm.stopPrank();
}
function test_multicall_initializePool_mint() public {
@@ -174,7 +187,6 @@ contract PositionManagerMulticallTest is Test, Permit2SignatureHelpers, PosmTest
bytes[] memory calls = new bytes[](1);
calls[0] = abi.encodeWithSelector(IPositionManager.modifyLiquidities.selector, actions, _deadline);
- address charlie = makeAddr("CHARLIE");
vm.startPrank(charlie);
vm.expectRevert(abi.encodeWithSelector(IPositionManager.NotApproved.selector, charlie));
lpm.multicall(calls);
@@ -360,4 +372,136 @@ contract PositionManagerMulticallTest is Test, Permit2SignatureHelpers, PosmTest
assertEq(liquidity, 10e18);
assertEq(lpm.ownerOf(tokenId), bob);
}
+
+ /// @notice test that a front-ran permit does not fail a multicall with permit
+ function test_multicall_permit_frontrun_suceeds() public {
+ config = PositionConfig({
+ poolKey: key,
+ tickLower: TickMath.minUsableTick(key.tickSpacing),
+ tickUpper: TickMath.maxUsableTick(key.tickSpacing)
+ });
+
+ // Charlie signs permit for the two tokens
+ IAllowanceTransfer.PermitSingle memory permit0 =
+ defaultERC20PermitAllowance(Currency.unwrap(currency0), permitAmount, permitExpiration, permitNonce);
+ permit0.spender = address(lpm);
+ bytes memory sig0 = getPermitSignature(permit0, charliePK, PERMIT2_DOMAIN_SEPARATOR);
+
+ IAllowanceTransfer.PermitSingle memory permit1 =
+ defaultERC20PermitAllowance(Currency.unwrap(currency1), permitAmount, permitExpiration, permitNonce);
+ permit1.spender = address(lpm);
+ bytes memory sig1 = getPermitSignature(permit1, charliePK, PERMIT2_DOMAIN_SEPARATOR);
+
+ // bob front-runs the permits
+ vm.startPrank(bob);
+ lpm.permit(charlie, permit0, sig0);
+ lpm.permit(charlie, permit1, sig1);
+ vm.stopPrank();
+
+ // bob's front-run was successful
+ (uint160 _amount, uint48 _expiration, uint48 _nonce) =
+ permit2.allowance(charlie, Currency.unwrap(currency0), address(lpm));
+ assertEq(_amount, permitAmount);
+ assertEq(_expiration, permitExpiration);
+ assertEq(_nonce, permitNonce + 1);
+ (uint160 _amount1, uint48 _expiration1, uint48 _nonce1) =
+ permit2.allowance(charlie, Currency.unwrap(currency1), address(lpm));
+ assertEq(_amount1, permitAmount);
+ assertEq(_expiration1, permitExpiration);
+ assertEq(_nonce1, permitNonce + 1);
+
+ // charlie tries to mint an LP token with multicall(permit, permit, mint)
+ bytes[] memory calls = new bytes[](3);
+ calls[0] = abi.encodeWithSelector(Permit2Forwarder(lpm).permit.selector, charlie, permit0, sig0);
+ calls[1] = abi.encodeWithSelector(Permit2Forwarder(lpm).permit.selector, charlie, permit1, sig1);
+ bytes memory mintCall = getMintEncoded(config, 10e18, charlie, ZERO_BYTES);
+ calls[2] = abi.encodeWithSelector(IPositionManager.modifyLiquidities.selector, mintCall, _deadline);
+
+ uint256 tokenId = lpm.nextTokenId();
+ vm.expectRevert();
+ lpm.ownerOf(tokenId); // token does not exist
+
+ bytes[] memory results = lpm.multicall(calls);
+ assertEq(
+ results[0],
+ abi.encode(
+ abi.encodeWithSelector(
+ Permit2Forwarder.Wrap__Permit2Reverted.selector,
+ address(permit2),
+ abi.encodeWithSelector(InvalidNonce.selector)
+ )
+ )
+ );
+ assertEq(
+ results[1],
+ abi.encode(
+ abi.encodeWithSelector(
+ Permit2Forwarder.Wrap__Permit2Reverted.selector,
+ address(permit2),
+ abi.encodeWithSelector(InvalidNonce.selector)
+ )
+ )
+ );
+
+ assertEq(lpm.ownerOf(tokenId), charlie);
+ }
+
+ /// @notice test that a front-ran permitBatch does not fail a multicall with permitBatch
+ function test_multicall_permitBatch_frontrun_suceeds() public {
+ config = PositionConfig({
+ poolKey: key,
+ tickLower: TickMath.minUsableTick(key.tickSpacing),
+ tickUpper: TickMath.maxUsableTick(key.tickSpacing)
+ });
+
+ // Charlie signs permitBatch for the two tokens
+ address[] memory tokens = new address[](2);
+ tokens[0] = Currency.unwrap(currency0);
+ tokens[1] = Currency.unwrap(currency1);
+
+ IAllowanceTransfer.PermitBatch memory permit =
+ defaultERC20PermitBatchAllowance(tokens, permitAmount, permitExpiration, permitNonce);
+ permit.spender = address(lpm);
+ bytes memory sig = getPermitBatchSignature(permit, charliePK, PERMIT2_DOMAIN_SEPARATOR);
+
+ // bob front-runs the permits
+ vm.prank(bob);
+ lpm.permitBatch(charlie, permit, sig);
+
+ // bob's front-run was successful
+ (uint160 _amount, uint48 _expiration, uint48 _nonce) =
+ permit2.allowance(charlie, Currency.unwrap(currency0), address(lpm));
+ assertEq(_amount, permitAmount);
+ assertEq(_expiration, permitExpiration);
+ assertEq(_nonce, permitNonce + 1);
+ (uint160 _amount1, uint48 _expiration1, uint48 _nonce1) =
+ permit2.allowance(charlie, Currency.unwrap(currency1), address(lpm));
+ assertEq(_amount1, permitAmount);
+ assertEq(_expiration1, permitExpiration);
+ assertEq(_nonce1, permitNonce + 1);
+
+ // charlie tries to mint an LP token with multicall(permitBatch, mint)
+ bytes[] memory calls = new bytes[](2);
+ calls[0] = abi.encodeWithSelector(Permit2Forwarder(lpm).permitBatch.selector, charlie, permit, sig);
+ bytes memory mintCall = getMintEncoded(config, 10e18, charlie, ZERO_BYTES);
+ calls[1] = abi.encodeWithSelector(IPositionManager.modifyLiquidities.selector, mintCall, _deadline);
+
+ uint256 tokenId = lpm.nextTokenId();
+ vm.expectRevert();
+ lpm.ownerOf(tokenId); // token does not exist
+
+ bytes[] memory results = lpm.multicall(calls);
+ assertEq(
+ results[0],
+ abi.encode(
+ abi.encodeWithSelector(
+ Permit2Forwarder.Wrap__Permit2Reverted.selector,
+ address(permit2),
+ abi.encodeWithSelector(InvalidNonce.selector)
+ )
+ )
+ );
+
+ assertEq(lpm.ownerOf(tokenId), charlie);
+ }
}