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.

Sign up for GitHub

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

OZ-L06: try-catch permit2Forwarder to avoid exposing DOS on multicall #309

Merged
merged 5 commits into from
Sep 3, 2024
Merged

OZ-L06: try-catch permit2Forwarder to avoid exposing DOS on multicall #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d77a130
try-catch permit2Forwarder to avoid exposing DOS on multicall
saucepoint Aug 28, 2024
b03a53b
fix typo
saucepoint Aug 28, 2024
7efca6b
Merge branch 'main' into oz-L06-permit2forwarder-dos
saucepoint Aug 29, 2024
0d0ada1
pr feedback nits
saucepoint Aug 29, 2024
4efa23f
return error and assert the permit error
saucepoint Aug 29, 2024
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
16 changes: 14 additions & 2 deletions src/base/Permit2Forwarder.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -17,16 +19,26 @@ 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
/// @dev this function is payable to allow multicall with NATIVE based actions
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);
}
}
}
146 changes: 145 additions & 1 deletion test/position-managers/PositionManager.multicall.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -54,13 +56,17 @@ 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;

function setUp() public {
(alice, alicePK) = makeAddrAndKey("ALICE");
(bob, bobPK) = makeAddrAndKey("BOB");
(charlie, charliePK) = makeAddrAndKey("CHARLIE");

deployFreshManagerAndRouters();
deployMintAndApprove2Currencies();
Expand All @@ -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 {
Expand Down Expand Up @@ -135,7 +148,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);
Expand Down Expand Up @@ -321,4 +333,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();

saucepoint marked this conversation as resolved.
Show resolved Hide resolved
// 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);
hensha256 marked this conversation as resolved.
Show resolved Hide resolved
}

/// @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);
saucepoint marked this conversation as resolved.
Show resolved Hide resolved

// 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);
}
}
Loading