From 540448c5c77eeb82c2f6cfe90c63a8c78ce8592c Mon Sep 17 00:00:00 2001 From: Hayden Shively <17186559+haydenshively@users.noreply.github.com> Date: Fri, 27 Oct 2023 22:05:46 -0500 Subject: [PATCH] Add BoostManager slippage protection and create Permit2Manager --- periphery/src/managers/BoostManager.sol | 8 ++- periphery/src/managers/Permit2Manager.sol | 73 ++++++++++++++++++++++ periphery/test/managers/BoostManager.t.sol | 11 +++- 3 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 periphery/src/managers/Permit2Manager.sol diff --git a/periphery/src/managers/BoostManager.sol b/periphery/src/managers/BoostManager.sol index 6632e59b..bf75d527 100644 --- a/periphery/src/managers/BoostManager.sol +++ b/periphery/src/managers/BoostManager.sol @@ -76,7 +76,12 @@ contract BoostManager is IManager, IUniswapV3SwapCallback { uint128 liquidity; // Leverage factor uint24 boost; - (tokenId, lower, upper, liquidity, boost) = abi.decode(args, (uint256, int24, int24, uint128, uint24)); + // Packed maxBorrow0 and maxBorrow1; slippage protection + uint224 maxBorrows; + (tokenId, lower, upper, liquidity, boost, maxBorrows) = abi.decode( + args, + (uint256, int24, int24, uint128, uint24, uint224) + ); require(owner == UNISWAP_NFT.ownerOf(tokenId), "Aloe: owners must match to import"); @@ -96,6 +101,7 @@ contract BoostManager is IManager, IUniswapV3SwapCallback { amount1 = (needs1 + 1) > amount1 ? (needs1 + 1 - amount1) : 0; } + require(amount0 < uint112(maxBorrows) && amount1 < (maxBorrows >> 112), "slippage"); borrower.borrow(amount0, amount1, msg.sender); borrower.uniswapDeposit(lower, upper, liquidity); } diff --git a/periphery/src/managers/Permit2Manager.sol b/periphery/src/managers/Permit2Manager.sol new file mode 100644 index 00000000..3235938f --- /dev/null +++ b/periphery/src/managers/Permit2Manager.sol @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.17; + +import {IManager} from "aloe-ii-core/Borrower.sol"; +import {Factory} from "aloe-ii-core/Factory.sol"; + +import {IPermit2} from "../interfaces/IPermit2.sol"; + +/// @dev Permit2 signatures attest to {token, amount, spender, nonce, deadline}. Noticeable lacking is a `to` +/// field. So given a signature, this contract would have permission to send the user's tokens *anywhere*. In +/// this case the `to` address is part of the incoming calldata, so we must verify that the user is the creator +/// of that calldata (or has authorized it somehow). +/// Within `callback`, if `FACTORY.isBorrower(msg.sender) && owner == BORROWER_NFT`, then we can be sure +/// that either the user themselves OR someone who's been approved to manage their NFT(s) is the source +/// of the calldata. +contract Permit2Manager is IManager { + error Permit2CallFailed(); + + error BorrowerCallFailed(); + + IPermit2 public immutable PERMIT2; + + Factory public immutable FACTORY; + + address public immutable BORROWER_NFT; + + constructor(IPermit2 permit2, Factory factory, address borrowerNft) { + PERMIT2 = permit2; + FACTORY = factory; + BORROWER_NFT = borrowerNft; + } + + function callback(bytes calldata data, address owner, uint208) external override returns (uint208) { + // Need to check that `msg.sender` is really a borrower and that its owner is `BORROWER_NFT` + // in order to be sure that incoming `data` is in the expected format + require(FACTORY.isBorrower(msg.sender) && owner == BORROWER_NFT, "Aloe: bad caller"); + + // `data` layout... + // ------------------------------------------- + // | value | start | end | length | + // | owner | 0 | 20 | 20 | + // | permit2 selector | 20 | 24 | 4 | + // | permit2 args | 24 | 248 | 224 | + // | permit2 sig | 248 | 313 | 65 | + // | borrower call | 313 | ? | ? | + // ------------------------------------------- + + // Get references to the calldata for the 2 calls we're going to make + bytes calldata dataPermit2 = data[20:313]; + bytes calldata dataBorrower = data[313:]; + + // Before calling `PERMIT2`, verify + // (a) correct function selector + // (b) `to` field is the Borrower (`msg.sender`) + // (c) correct signer address, i.e. [claimed Permit2 signer] == [user who owns the Borrower] + // Note that data[:20] is the true owner prepended by the `BORROWER_NFT` + require( + bytes4(dataPermit2[:4]) == IPermit2.permitTransferFrom.selector && + bytes20(dataPermit2[144:164]) == bytes20(msg.sender) && + bytes20(dataPermit2[208:228]) == bytes20(data[:20]) + ); + + // Make calls + bool success; + (success, ) = address(PERMIT2).call(dataPermit2); // solhint-disable-line avoid-low-level-calls + if (!success) revert Permit2CallFailed(); + + (success, ) = msg.sender.call(dataBorrower); // solhint-disable-line avoid-low-level-calls + if (!success) revert BorrowerCallFailed(); + + return 0; + } +} diff --git a/periphery/test/managers/BoostManager.t.sol b/periphery/test/managers/BoostManager.t.sol index c56cf5d0..5bcd4642 100644 --- a/periphery/test/managers/BoostManager.t.sol +++ b/periphery/test/managers/BoostManager.t.sol @@ -45,7 +45,7 @@ contract BoostManagerTest is Test { lender1.deposit(1e18, address(0)); } - function test_mint() public { + function test_mint() public { address owner = 0xde8E7d3fFada10dE2A57E7bAc090dB06596F51Cd; bytes memory mintCall; @@ -67,7 +67,14 @@ contract BoostManagerTest is Test { managers[0] = boostManager; datas[0] = abi.encode( uint8(0), - abi.encode(uint256(425835), int24(70020), int24(71700), uint128(344339104909795631), 10_000) + abi.encode( + uint256(425835), + int24(70020), + int24(71700), + uint128(344339104909795631), + 10_000, + uint224(type(uint224).max) + ) ); antes[0] = 0.01 ether / 1e13; modifyCall = abi.encodeCall(borrowerNft.modify, (owner, indices, managers, datas, antes));