From ffad259548be6d8aae54136e8f66dc1dc4eb67a0 Mon Sep 17 00:00:00 2001 From: Benjamin Date: Tue, 16 Jul 2024 10:01:28 +0200 Subject: [PATCH 1/6] Improve stake & lock UX --- .../script/DeployPufferL2Depositor.s.sol | 4 +- mainnet-contracts/src/PufLocker.sol | 6 +- mainnet-contracts/src/PufferL2Depositor.sol | 54 ++++++++--- .../src/interface/IPufLocker.sol | 3 +- .../src/interface/IPufferL2Depositor.sol | 10 +- mainnet-contracts/test/unit/PufLocker.t.sol | 25 +++-- .../test/unit/PufferL2Staking.t.sol | 91 ++++++++++++++++--- 7 files changed, 148 insertions(+), 45 deletions(-) diff --git a/mainnet-contracts/script/DeployPufferL2Depositor.s.sol b/mainnet-contracts/script/DeployPufferL2Depositor.s.sol index 586a922..d0397f5 100644 --- a/mainnet-contracts/script/DeployPufferL2Depositor.s.sol +++ b/mainnet-contracts/script/DeployPufferL2Depositor.s.sol @@ -39,13 +39,13 @@ contract DeployPufferL2Depositor is Script { vm.startBroadcast(); - depositor = new PufferL2Depositor(address(accessManager), weth); - address pufLockerImpl = address(new PufLocker()); pufLocker = PufLocker( address(new ERC1967Proxy(pufLockerImpl, abi.encodeCall(PufLocker.initialize, (address(accessManager))))) ); + depositor = new PufferL2Depositor(address(accessManager), weth, pufLocker); + bytes[] memory calldatas = new bytes[](4); bytes4[] memory publicSelectors = new bytes4[](3); diff --git a/mainnet-contracts/src/PufLocker.sol b/mainnet-contracts/src/PufLocker.sol index f12f47c..43c9e14 100644 --- a/mainnet-contracts/src/PufLocker.sol +++ b/mainnet-contracts/src/PufLocker.sol @@ -42,7 +42,7 @@ contract PufLocker is IPufLocker, AccessManagedUpgradeable, UUPSUpgradeable, Puf * @inheritdoc IPufLocker * @dev Restricted in this context is like `whenNotPaused` modifier from Pausable.sol */ - function deposit(address token, uint128 lockPeriod, Permit calldata permitData) + function deposit(address token, address recipient, uint128 lockPeriod, Permit calldata permitData) external isAllowedToken(token) restricted @@ -71,9 +71,9 @@ contract PufLocker is IPufLocker, AccessManagedUpgradeable, UUPSUpgradeable, Puf uint128 releaseTime = uint128(block.timestamp) + lockPeriod; - $.deposits[msg.sender][token].push(Deposit(uint128(permitData.amount), releaseTime)); + $.deposits[recipient][token].push(Deposit(uint128(permitData.amount), releaseTime)); - emit Deposited(msg.sender, token, uint128(permitData.amount), releaseTime); + emit Deposited(recipient, token, uint128(permitData.amount), releaseTime); } /** diff --git a/mainnet-contracts/src/PufferL2Depositor.sol b/mainnet-contracts/src/PufferL2Depositor.sol index 980c03a..3f822bd 100644 --- a/mainnet-contracts/src/PufferL2Depositor.sol +++ b/mainnet-contracts/src/PufferL2Depositor.sol @@ -10,6 +10,7 @@ import { ERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/ERC2 import { Permit } from "./structs/Permit.sol"; import { IWETH } from "./interface/IWETH.sol"; import { IPufferL2Depositor } from "./interface/IPufferL2Depositor.sol"; +import { IPufLocker } from "./interface/IPufLocker.sol"; /** * @title Puffer L2 Depositor contract @@ -25,11 +26,14 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { address public immutable WETH; + IPufLocker public immutable PUFFER_LOCKER; + mapping(address token => address pufToken) public tokens; mapping(address migrator => bool isAllowed) public isAllowedMigrator; - constructor(address accessManager, address weth) AccessManaged(accessManager) { + constructor(address accessManager, address weth, IPufLocker locker) AccessManaged(accessManager) { WETH = weth; + PUFFER_LOCKER = locker; _addNewToken(weth); } @@ -44,11 +48,13 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { * @inheritdoc IPufferL2Depositor * @dev Restricted in this context is like `whenNotPaused` modifier from Pausable.sol */ - function deposit(address token, address account, Permit calldata permitData, uint256 referralCode) - external - onlySupportedTokens(token) - restricted - { + function deposit( + address token, + address account, + Permit calldata permitData, + uint256 referralCode, + uint128 lockPeriod + ) external onlySupportedTokens(token) restricted { // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations try ERC20Permit(token).permit({ owner: msg.sender, @@ -63,6 +69,7 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { IERC20(token).safeTransferFrom(msg.sender, address(this), permitData.amount); _deposit({ + lockPeriod: lockPeriod, token: token, depositor: msg.sender, account: account, @@ -75,10 +82,17 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { * @inheritdoc IPufferL2Depositor * @dev Restricted in this context is like `whenNotPaused` modifier from Pausable.sol */ - function depositETH(address account, uint256 referralCode) external payable restricted { + function depositETH(address account, uint256 referralCode, uint128 lockPeriod) external payable restricted { IWETH(WETH).deposit{ value: msg.value }(); - _deposit({ token: WETH, depositor: msg.sender, account: account, amount: msg.value, referralCode: referralCode }); + _deposit({ + token: WETH, + depositor: msg.sender, + account: account, + amount: msg.value, + referralCode: referralCode, + lockPeriod: lockPeriod + }); } /** @@ -117,14 +131,30 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { */ function revertIfPaused() external restricted { } - function _deposit(address token, address depositor, address account, uint256 amount, uint256 referralCode) - internal - { + function _deposit( + address token, + address depositor, + address account, + uint256 amount, + uint256 referralCode, + uint128 lockPeriod + ) internal { PufToken pufToken = PufToken(tokens[token]); IERC20(token).safeIncreaseAllowance(address(pufToken), amount); - pufToken.deposit(depositor, account, amount); + // If the lockPeriod is greater than 0 we wrap and then deposit the wrapped tokens to the locker contract + if (lockPeriod > 0) { + pufToken.deposit(depositor, address(this), amount); + IERC20(address(pufToken)).safeIncreaseAllowance(address(PUFFER_LOCKER), amount); + Permit memory permitData; + permitData.amount = amount; + // Tokens are being deposited to the locker contract for the account + PUFFER_LOCKER.deposit(address(pufToken), account, lockPeriod, permitData); + } else { + // The account will receive the ERC20 tokens + pufToken.deposit(depositor, account, amount); + } emit DepositedToken(token, msg.sender, account, amount, referralCode); } diff --git a/mainnet-contracts/src/interface/IPufLocker.sol b/mainnet-contracts/src/interface/IPufLocker.sol index 3095de3..5cd2bcc 100644 --- a/mainnet-contracts/src/interface/IPufLocker.sol +++ b/mainnet-contracts/src/interface/IPufLocker.sol @@ -87,10 +87,11 @@ interface IPufLocker { /** * @notice Deposit tokens into the locker * @param token The address of the token to deposit + * @param token The address of the recipient * @param lockPeriod The lock period for the deposit * @param permitData The permit data for the deposit */ - function deposit(address token, uint128 lockPeriod, Permit calldata permitData) external; + function deposit(address token, address recipient, uint128 lockPeriod, Permit calldata permitData) external; /** * @notice Withdraws specified deposits for a given token and transfers the funds to the recipient diff --git a/mainnet-contracts/src/interface/IPufferL2Depositor.sol b/mainnet-contracts/src/interface/IPufferL2Depositor.sol index 7c1cd3f..ee622e0 100644 --- a/mainnet-contracts/src/interface/IPufferL2Depositor.sol +++ b/mainnet-contracts/src/interface/IPufferL2Depositor.sol @@ -54,14 +54,20 @@ interface IPufferL2Depositor { * * @dev Restricted in this context is like `whenNotPaused` modifier from Pausable.sol */ - function deposit(address token, address account, Permit calldata permitData, uint256 referralCode) external; + function deposit( + address token, + address account, + Permit calldata permitData, + uint256 referralCode, + uint128 lockPeriod + ) external; /** * @notice Deposits naative ETH by wrapping it into WETH and then depositing to corresponding token contract * * @dev Restricted in this context is like `whenNotPaused` modifier from Pausable.sol */ - function depositETH(address account, uint256 referralCode) external payable; + function depositETH(address account, uint256 referralCode, uint128 lockPeriod) external payable; /** * @notice Called by the Token contracts to check if the system is paused diff --git a/mainnet-contracts/test/unit/PufLocker.t.sol b/mainnet-contracts/test/unit/PufLocker.t.sol index fadfdab..b4b82b6 100644 --- a/mainnet-contracts/test/unit/PufLocker.t.sol +++ b/mainnet-contracts/test/unit/PufLocker.t.sol @@ -7,9 +7,6 @@ import { IPufLocker } from "../../src/interface/IPufLocker.sol"; import { AccessManager } from "@openzeppelin/contracts/access/manager/AccessManager.sol"; import { Multicall } from "@openzeppelin/contracts/utils/Multicall.sol"; import { ROLE_ID_OPERATIONS_MULTISIG, PUBLIC_ROLE } from "../../script/Roles.sol"; -import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; -import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; -import { ERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol"; import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import { IAccessManaged } from "@openzeppelin/contracts/access/manager/IAccessManaged.sol"; import { ERC20Mock } from "../mocks/ERC20Mock.sol"; @@ -75,7 +72,7 @@ contract PufLockerTest is UnitTestHelper { Permit memory permit = _signPermit(_testTemps("bob", address(pufLocker), amount, block.timestamp), mockToken.DOMAIN_SEPARATOR()); - pufLocker.deposit(address(mockToken), 61, permit); // Lock for 1 minute + pufLocker.deposit(address(mockToken), address(this), 61, permit); // Lock for 1 minute } function testRevert_SetAllowedToken_Unauthorized() public { @@ -105,7 +102,7 @@ contract PufLockerTest is UnitTestHelper { Permit memory permit = _signPermit(_testTemps("bob", address(pufLocker), amount, block.timestamp), mockToken.DOMAIN_SEPARATOR()); - pufLocker.deposit(address(mockToken), 3600, permit); // Lock for 1 hour + pufLocker.deposit(address(mockToken), bob, 3600, permit); // Lock for 1 hour (PufLocker.Deposit[] memory deposits) = pufLocker.getDeposits(bob, address(mockToken), 0, 1); assertEq(deposits.length, 1, "Should have 1 deposit"); assertEq(deposits[0].amount, 10e18, "Deposit amount should be 100 tokens"); @@ -118,7 +115,7 @@ contract PufLockerTest is UnitTestHelper { Permit memory permit = _signPermit(_testTemps("bob", address(pufLocker), amount, block.timestamp), mockToken.DOMAIN_SEPARATOR()); vm.expectRevert(abi.encodeWithSelector(InvalidAmount.selector)); - pufLocker.deposit(address(mockToken), 3600, permit); // Lock for 1 hour with 0 amount + pufLocker.deposit(address(mockToken), bob, 3600, permit); // Lock for 1 hour with 0 amount vm.stopPrank(); } @@ -128,7 +125,7 @@ contract PufLockerTest is UnitTestHelper { Permit memory permit = _signPermit(_testTemps("bob", address(pufLocker), amount, block.timestamp), mockToken.DOMAIN_SEPARATOR()); vm.expectRevert(abi.encodeWithSelector(IPufLocker.InvalidLockPeriod.selector)); - pufLocker.deposit(address(mockToken), 30, permit); // Lock for 30 seconds, which is less than minLockPeriod + pufLocker.deposit(address(mockToken), bob, 30, permit); // Lock for 30 seconds, which is less than minLockPeriod vm.stopPrank(); } @@ -137,7 +134,7 @@ contract PufLockerTest is UnitTestHelper { uint256 amount = 10e18; Permit memory permit = _signPermit(_testTemps("bob", address(pufLocker), amount, block.timestamp), mockToken.DOMAIN_SEPARATOR()); - pufLocker.deposit(address(mockToken), 60, permit); // Lock for 1 minute + pufLocker.deposit(address(mockToken), bob, 60, permit); // Lock for 1 minute vm.warp(block.timestamp + 61); // Fast forward time by 61 seconds uint256[] memory indexes = new uint256[](1); @@ -155,7 +152,7 @@ contract PufLockerTest is UnitTestHelper { uint256 amount = 10e18; Permit memory permit = _signPermit(_testTemps("bob", address(pufLocker), amount, block.timestamp), mockToken.DOMAIN_SEPARATOR()); - pufLocker.deposit(address(mockToken), 3600, permit); // Lock for 1 hour + pufLocker.deposit(address(mockToken), bob, 3600, permit); // Lock for 1 hour uint256[] memory indexes = new uint256[](1); indexes[0] = 0; @@ -169,7 +166,7 @@ contract PufLockerTest is UnitTestHelper { uint256 amount = 10e18; Permit memory permit = _signPermit(_testTemps("bob", address(pufLocker), amount, block.timestamp), mockToken.DOMAIN_SEPARATOR()); - pufLocker.deposit(address(mockToken), 60, permit); // Lock for 1 minute + pufLocker.deposit(address(mockToken), bob, 60, permit); // Lock for 1 minute vm.warp(block.timestamp + 61); // Fast forward time by 61 seconds uint256[] memory indexes = new uint256[](2); @@ -186,7 +183,7 @@ contract PufLockerTest is UnitTestHelper { uint256 amount = 10e18; Permit memory permit = _signPermit(_testTemps("bob", address(pufLocker), amount, block.timestamp), mockToken.DOMAIN_SEPARATOR()); - pufLocker.deposit(address(mockToken), 60, permit); // Lock for 1 minute + pufLocker.deposit(address(mockToken), bob, 60, permit); // Lock for 1 minute vm.warp(block.timestamp + 61); // Fast forward time by 61 seconds uint256[] memory indexes = new uint256[](1); @@ -206,20 +203,20 @@ contract PufLockerTest is UnitTestHelper { uint256 amount = 2e18; Permit memory permit = _signPermit(_testTemps("bob", address(pufLocker), amount, block.timestamp), mockToken.DOMAIN_SEPARATOR()); - pufLocker.deposit(address(mockToken), 3601, permit); + pufLocker.deposit(address(mockToken), bob, 3601, permit); uint256 amount2 = 4e18; Permit memory permit2 = _signPermit(_testTemps("bob", address(pufLocker), amount2, block.timestamp), mockToken.DOMAIN_SEPARATOR()); mockToken.approve(address(pufLocker), amount2); - pufLocker.deposit(address(mockToken), 3620, permit2); + pufLocker.deposit(address(mockToken), bob, 3620, permit2); uint256 amount3 = 5e18; Permit memory permit3 = _signPermit( _testTemps("bob", address(pufLocker), amount3, block.timestamp + 2), mockToken.DOMAIN_SEPARATOR() ); mockToken.approve(address(pufLocker), amount3); - pufLocker.deposit(address(mockToken), 3630, permit3); + pufLocker.deposit(address(mockToken), bob, 3630, permit3); // Get the first 2 deposits PufLocker.Deposit[] memory depositsPage1 = pufLocker.getDeposits(bob, address(mockToken), 0, 2); diff --git a/mainnet-contracts/test/unit/PufferL2Staking.t.sol b/mainnet-contracts/test/unit/PufferL2Staking.t.sol index 77163ac..bb0bd69 100644 --- a/mainnet-contracts/test/unit/PufferL2Staking.t.sol +++ b/mainnet-contracts/test/unit/PufferL2Staking.t.sol @@ -14,8 +14,12 @@ import { Permit } from "../../src/structs/Permit.sol"; import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import { ERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol"; +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import { IAccessManaged } from "@openzeppelin/contracts/access/manager/IAccessManaged.sol"; import { InvalidAmount } from "../../src/Errors.sol"; +import { IPufLocker } from "../../src/interface/IPufLocker.sol"; +import { PufLocker } from "../../src/PufLocker.sol"; +import { IPufLocker } from "../../src/interface/IPufLocker.sol"; contract MockToken is ERC20, ERC20Permit { uint8 _dec; // decimals @@ -58,6 +62,7 @@ contract PufferL2Staking is UnitTestHelper { MockToken sixDecimal; MockToken twentyTwoDecimal; MockToken notSupportedToken; + PufLocker pufLocker; address mockMigrator; @@ -74,7 +79,12 @@ contract PufferL2Staking is UnitTestHelper { twentyTwoDecimal = new MockToken("TwentyTwoDecimal", "TKN22", 22); notSupportedToken = new MockToken("NotSupported", "NOT", 18); - depositor = new PufferL2Depositor(address(accessManager), address(weth)); + address pufLockerImpl = address(new PufLocker()); + pufLocker = PufLocker( + address(new ERC1967Proxy(pufLockerImpl, abi.encodeCall(PufLocker.initialize, (address(accessManager))))) + ); + + depositor = new PufferL2Depositor(address(accessManager), address(weth), pufLocker); // Access setup @@ -105,11 +115,44 @@ contract PufferL2Staking is UnitTestHelper { vm.prank(address(timelock)); accessManager.multicall(calldatas); + // Access setup Locker + + calldatas = new bytes[](2); + + publicSelectors = new bytes4[](2); + publicSelectors[0] = PufLocker.deposit.selector; + publicSelectors[1] = PufLocker.withdraw.selector; + + calldatas[0] = abi.encodeWithSelector( + AccessManager.setTargetFunctionRole.selector, address(pufLocker), publicSelectors, PUBLIC_ROLE + ); + + multisigSelectors = new bytes4[](2); + multisigSelectors[0] = PufLocker.setIsAllowedToken.selector; + multisigSelectors[1] = PufLocker.setLockPeriods.selector; + + calldatas[1] = abi.encodeWithSelector( + AccessManager.setTargetFunctionRole.selector, + address(pufLocker), + multisigSelectors, + ROLE_ID_OPERATIONS_MULTISIG + ); + + // bytes memory multicallData = abi.encodeCall(Multicall.multicall, (calldatas)); + vm.prank(address(timelock)); + accessManager.multicall(calldatas); + vm.startPrank(OPERATIONS_MULTISIG); depositor.addNewToken(address(dai)); depositor.addNewToken(address(sixDecimal)); depositor.addNewToken(address(twentyTwoDecimal)); + + pufLocker.setLockPeriods(0, 365 days); + + pufLocker.setIsAllowedToken(depositor.tokens(address(dai)), true); + pufLocker.setIsAllowedToken(depositor.tokens(address(sixDecimal)), true); + pufLocker.setIsAllowedToken(depositor.tokens(address(twentyTwoDecimal)), true); } function test_setup() public view { @@ -147,7 +190,7 @@ contract PufferL2Staking is UnitTestHelper { vm.expectEmit(true, true, true, true); emit IPufferL2Depositor.DepositedToken(address(dai), bob, bob, amount, refCode); - depositor.deposit(address(dai), bob, permit, refCode); + depositor.deposit(address(dai), bob, permit, refCode, 0); PufToken pufToken = PufToken(depositor.tokens(address(dai))); assertEq(pufToken.balanceOf(bob), amount, "bob got pufToken"); @@ -168,7 +211,7 @@ contract PufferL2Staking is UnitTestHelper { vm.expectEmit(true, true, true, true); emit IPufferL2Depositor.DepositedToken(address(sixDecimal), bob, bob, amount, referralCode); - depositor.deposit(address(sixDecimal), bob, permit, referralCode); + depositor.deposit(address(sixDecimal), bob, permit, referralCode, 0); PufToken pufToken = PufToken(depositor.tokens(address(sixDecimal))); @@ -198,7 +241,7 @@ contract PufferL2Staking is UnitTestHelper { vm.expectEmit(true, true, true, true); emit IPufferL2Depositor.DepositedToken(address(twentyTwoDecimal), bob, bob, amount, referralCode); - depositor.deposit(address(twentyTwoDecimal), bob, permit, referralCode); + depositor.deposit(address(twentyTwoDecimal), bob, permit, referralCode, 0); PufToken pufToken = PufToken(depositor.tokens(address(twentyTwoDecimal))); @@ -228,7 +271,7 @@ contract PufferL2Staking is UnitTestHelper { vm.expectEmit(true, true, true, true); emit IPufferL2Depositor.DepositedToken(address(dai), bob, bob, amount, refCode); - depositor.deposit(address(dai), bob, permit, refCode); + depositor.deposit(address(dai), bob, permit, refCode, 0); PufToken pufToken = PufToken(depositor.tokens(address(dai))); assertEq(pufToken.balanceOf(bob), amount, "bob got pufToken"); @@ -252,7 +295,7 @@ contract PufferL2Staking is UnitTestHelper { // weth.permit triggers weth.fallback() and it doesn't revert vm.expectEmit(true, true, true, true); emit IPufferL2Depositor.DepositedToken(address(weth), bob, bob, amount, refCode); - depositor.deposit(address(weth), bob, permit, refCode); + depositor.deposit(address(weth), bob, permit, refCode, 0); PufToken pufToken = PufToken(depositor.tokens(address(weth))); assertEq(pufToken.balanceOf(bob), amount, "bob got pufToken"); @@ -268,7 +311,7 @@ contract PufferL2Staking is UnitTestHelper { vm.expectEmit(true, true, true, true); emit IPufferL2Depositor.DepositedToken(address(weth), bob, bob, amount, refCode); - depositor.depositETH{ value: amount }(bob, refCode); + depositor.depositETH{ value: amount }(bob, refCode, 0); PufToken pufToken = PufToken(depositor.tokens(address(weth))); assertEq(pufToken.balanceOf(bob), amount, "bob got pufToken"); @@ -389,7 +432,7 @@ contract PufferL2Staking is UnitTestHelper { vm.startPrank(bob); vm.expectRevert(); - depositor.deposit(address(notSupportedToken), bob, permit, referralCode); + depositor.deposit(address(notSupportedToken), bob, permit, referralCode, 0); } // zero address token reverts @@ -421,7 +464,7 @@ contract PufferL2Staking is UnitTestHelper { vm.startPrank(bob); vm.expectRevert(InvalidAmount.selector); - depositor.depositETH{ value: 0 }(bob, referralCode); + depositor.depositETH{ value: 0 }(bob, referralCode, 0); } // Mock address 123 is not allowed to be migrator @@ -437,14 +480,14 @@ contract PufferL2Staking is UnitTestHelper { vm.deal(bob, 1 ether); vm.startPrank(bob); vm.expectRevert(); - depositor.depositETH{ value: 1 ether }(address(0), referralCode); + depositor.depositETH{ value: 1 ether }(address(0), referralCode, 0); } // 0 deposit eth reverts function testRevert_zero_deposit_ETH() public { vm.startPrank(bob); vm.expectRevert(); - depositor.depositETH{ value: 0 }(bob, referralCode); + depositor.depositETH{ value: 0 }(bob, referralCode, 0); } // No deposit reverts @@ -501,6 +544,32 @@ contract PufferL2Staking is UnitTestHelper { pufToken.deposit(bob, bob, 7 ether); } + // Deposit and lock tokens in one tx + function test_deposit_and_lock(uint32 amount, uint256 refCode) public { + vm.assume(amount > 0); + + // This is a bad permit signature + Permit memory permit = + _signPermit(_testTemps("bob", address(depositor), amount, block.timestamp), "dummy domain separator"); + + dai.mint(bob, amount); + + vm.startPrank(bob); + + dai.approve(address(depositor), amount); + + vm.expectEmit(true, true, true, true); + emit IPufferL2Depositor.DepositedToken(address(dai), bob, bob, amount, refCode); + depositor.deposit(address(dai), bob, permit, refCode, 15); // lock for 15 seconds + + PufToken pufToken = PufToken(depositor.tokens(address(dai))); + assertEq(pufToken.balanceOf(bob), 0, "bob did not get pufToken"); + + (PufLocker.Deposit[] memory deposits) = pufLocker.getDeposits(bob, address(pufToken), 0, 1); + assertEq(deposits.length, 1, "Should have 1 deposit"); + assertEq(deposits[0].amount, amount, "Deposit amount locked for Bob"); + } + function testRevert_SetDepositCap_Unauthorized() public { // Try setting the supply cap from an unauthorized address vm.startPrank(bob); From c5d5a7d3e962446276693e55897e2f8d5a63f760 Mon Sep 17 00:00:00 2001 From: Benjamin Date: Tue, 23 Jul 2024 10:19:20 +0200 Subject: [PATCH 2/6] Skip calling .permit() if the data might be bad. --- mainnet-contracts/src/PufLocker.sol | 31 +++++++++++++------- mainnet-contracts/src/PufferL2Depositor.sol | 32 ++++++++++++++------- 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/mainnet-contracts/src/PufLocker.sol b/mainnet-contracts/src/PufLocker.sol index 43c9e14..cd6c019 100644 --- a/mainnet-contracts/src/PufLocker.sol +++ b/mainnet-contracts/src/PufLocker.sol @@ -56,16 +56,27 @@ contract PufLocker is IPufLocker, AccessManagedUpgradeable, UUPSUpgradeable, Puf revert InvalidLockPeriod(); } - // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations - try ERC20Permit(token).permit({ - owner: msg.sender, - spender: address(this), - value: permitData.amount, - deadline: permitData.deadline, - v: permitData.v, - s: permitData.s, - r: permitData.r - }) { } catch { } + // The users that use a smart wallet and do not use the Permit and they do the .approve and then .deposit. + // They might get confused when they open Etherscan, and see: + // "Although one or more Error Occurred [execution reverted] Contract Execution Completed" + + // To avoid that, we don't want to call the permit function if it is not necessary. + // OpenZeppelin's ERC20Permit tokens use the ECDSA.sol library to check if the signature is valid. + // We can borrow the line of code that they wrote and skip calling the permit function if `permitData.s` is not valid. + + // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9e73c4b58120c17135bf269945d0f148810bf7f1/contracts/utils/cryptography/ECDSA.sol#L137 + if (uint256(permitData.s) < 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { + // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations + try ERC20Permit(token).permit({ + owner: msg.sender, + spender: address(this), + value: permitData.amount, + deadline: permitData.deadline, + v: permitData.v, + s: permitData.s, + r: permitData.r + }) { } catch { } + } IERC20(token).safeTransferFrom(msg.sender, address(this), permitData.amount); diff --git a/mainnet-contracts/src/PufferL2Depositor.sol b/mainnet-contracts/src/PufferL2Depositor.sol index 3f822bd..36116bc 100644 --- a/mainnet-contracts/src/PufferL2Depositor.sol +++ b/mainnet-contracts/src/PufferL2Depositor.sol @@ -55,16 +55,28 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { uint256 referralCode, uint128 lockPeriod ) external onlySupportedTokens(token) restricted { - // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations - try ERC20Permit(token).permit({ - owner: msg.sender, - spender: address(this), - value: permitData.amount, - deadline: permitData.deadline, - v: permitData.v, - s: permitData.s, - r: permitData.r - }) { } catch { } + + // The users that use a smart wallet and do not use the Permit and they do the .approve and then .deposit. + // They might get confused when they open Etherscan, and see: + // "Although one or more Error Occurred [execution reverted] Contract Execution Completed" + + // To avoid that, we don't want to call the permit function if it is not necessary. + // OpenZeppelin's ERC20Permit tokens use the ECDSA.sol library to check if the signature is valid. + // We can borrow the line of code that they wrote and skip calling the permit function if `permitData.s` is not valid. + + // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9e73c4b58120c17135bf269945d0f148810bf7f1/contracts/utils/cryptography/ECDSA.sol#L137 + if (uint256(permitData.s) < 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { + // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations + try ERC20Permit(token).permit({ + owner: msg.sender, + spender: address(this), + value: permitData.amount, + deadline: permitData.deadline, + v: permitData.v, + s: permitData.s, + r: permitData.r + }) { } catch { } + } IERC20(token).safeTransferFrom(msg.sender, address(this), permitData.amount); From ff2ead8c2898f65118d0551bd267910964f74199 Mon Sep 17 00:00:00 2001 From: bxmmm1 Date: Tue, 23 Jul 2024 08:20:37 +0000 Subject: [PATCH 3/6] forge fmt --- mainnet-contracts/src/PufLocker.sol | 2 +- mainnet-contracts/src/PufferL2Depositor.sol | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/mainnet-contracts/src/PufLocker.sol b/mainnet-contracts/src/PufLocker.sol index cd6c019..da19676 100644 --- a/mainnet-contracts/src/PufLocker.sol +++ b/mainnet-contracts/src/PufLocker.sol @@ -57,7 +57,7 @@ contract PufLocker is IPufLocker, AccessManagedUpgradeable, UUPSUpgradeable, Puf } // The users that use a smart wallet and do not use the Permit and they do the .approve and then .deposit. - // They might get confused when they open Etherscan, and see: + // They might get confused when they open Etherscan, and see: // "Although one or more Error Occurred [execution reverted] Contract Execution Completed" // To avoid that, we don't want to call the permit function if it is not necessary. diff --git a/mainnet-contracts/src/PufferL2Depositor.sol b/mainnet-contracts/src/PufferL2Depositor.sol index 36116bc..446003e 100644 --- a/mainnet-contracts/src/PufferL2Depositor.sol +++ b/mainnet-contracts/src/PufferL2Depositor.sol @@ -55,9 +55,8 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { uint256 referralCode, uint128 lockPeriod ) external onlySupportedTokens(token) restricted { - // The users that use a smart wallet and do not use the Permit and they do the .approve and then .deposit. - // They might get confused when they open Etherscan, and see: + // They might get confused when they open Etherscan, and see: // "Although one or more Error Occurred [execution reverted] Contract Execution Completed" // To avoid that, we don't want to call the permit function if it is not necessary. From 6e0aa66ae77d206bd71f960c07feaec1d32d017c Mon Sep 17 00:00:00 2001 From: Benjamin Date: Tue, 23 Jul 2024 10:35:24 +0200 Subject: [PATCH 4/6] update the if statement wrapping permit call --- mainnet-contracts/src/PufLocker.sol | 6 +----- mainnet-contracts/src/PufferL2Depositor.sol | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/mainnet-contracts/src/PufLocker.sol b/mainnet-contracts/src/PufLocker.sol index da19676..f1e2f62 100644 --- a/mainnet-contracts/src/PufLocker.sol +++ b/mainnet-contracts/src/PufLocker.sol @@ -61,11 +61,7 @@ contract PufLocker is IPufLocker, AccessManagedUpgradeable, UUPSUpgradeable, Puf // "Although one or more Error Occurred [execution reverted] Contract Execution Completed" // To avoid that, we don't want to call the permit function if it is not necessary. - // OpenZeppelin's ERC20Permit tokens use the ECDSA.sol library to check if the signature is valid. - // We can borrow the line of code that they wrote and skip calling the permit function if `permitData.s` is not valid. - - // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9e73c4b58120c17135bf269945d0f148810bf7f1/contracts/utils/cryptography/ECDSA.sol#L137 - if (uint256(permitData.s) < 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { + if (permitData.deadline < block.timestamp) { // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations try ERC20Permit(token).permit({ owner: msg.sender, diff --git a/mainnet-contracts/src/PufferL2Depositor.sol b/mainnet-contracts/src/PufferL2Depositor.sol index 446003e..4986463 100644 --- a/mainnet-contracts/src/PufferL2Depositor.sol +++ b/mainnet-contracts/src/PufferL2Depositor.sol @@ -60,11 +60,7 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { // "Although one or more Error Occurred [execution reverted] Contract Execution Completed" // To avoid that, we don't want to call the permit function if it is not necessary. - // OpenZeppelin's ERC20Permit tokens use the ECDSA.sol library to check if the signature is valid. - // We can borrow the line of code that they wrote and skip calling the permit function if `permitData.s` is not valid. - - // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9e73c4b58120c17135bf269945d0f148810bf7f1/contracts/utils/cryptography/ECDSA.sol#L137 - if (uint256(permitData.s) < 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { + if (permitData.deadline < block.timestamp) { // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations try ERC20Permit(token).permit({ owner: msg.sender, From 9a5b8ef08f347684ecc45716e24ae8e08caa8c8e Mon Sep 17 00:00:00 2001 From: Benjamin Date: Tue, 23 Jul 2024 10:36:46 +0200 Subject: [PATCH 5/6] flip the if statement --- mainnet-contracts/src/PufLocker.sol | 2 +- mainnet-contracts/src/PufferL2Depositor.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mainnet-contracts/src/PufLocker.sol b/mainnet-contracts/src/PufLocker.sol index f1e2f62..89a08f7 100644 --- a/mainnet-contracts/src/PufLocker.sol +++ b/mainnet-contracts/src/PufLocker.sol @@ -61,7 +61,7 @@ contract PufLocker is IPufLocker, AccessManagedUpgradeable, UUPSUpgradeable, Puf // "Although one or more Error Occurred [execution reverted] Contract Execution Completed" // To avoid that, we don't want to call the permit function if it is not necessary. - if (permitData.deadline < block.timestamp) { + if (permitData.deadline > block.timestamp) { // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations try ERC20Permit(token).permit({ owner: msg.sender, diff --git a/mainnet-contracts/src/PufferL2Depositor.sol b/mainnet-contracts/src/PufferL2Depositor.sol index 4986463..8ebbcd8 100644 --- a/mainnet-contracts/src/PufferL2Depositor.sol +++ b/mainnet-contracts/src/PufferL2Depositor.sol @@ -60,7 +60,7 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { // "Although one or more Error Occurred [execution reverted] Contract Execution Completed" // To avoid that, we don't want to call the permit function if it is not necessary. - if (permitData.deadline < block.timestamp) { + if (permitData.deadline > block.timestamp) { // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations try ERC20Permit(token).permit({ owner: msg.sender, From ba6890c7627b90696a8ded7173c510800fa5cb5a Mon Sep 17 00:00:00 2001 From: Benjamin Date: Tue, 23 Jul 2024 10:38:52 +0200 Subject: [PATCH 6/6] >= for permit.deadline check --- mainnet-contracts/src/PufLocker.sol | 2 +- mainnet-contracts/src/PufferL2Depositor.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mainnet-contracts/src/PufLocker.sol b/mainnet-contracts/src/PufLocker.sol index 89a08f7..f49d4cd 100644 --- a/mainnet-contracts/src/PufLocker.sol +++ b/mainnet-contracts/src/PufLocker.sol @@ -61,7 +61,7 @@ contract PufLocker is IPufLocker, AccessManagedUpgradeable, UUPSUpgradeable, Puf // "Although one or more Error Occurred [execution reverted] Contract Execution Completed" // To avoid that, we don't want to call the permit function if it is not necessary. - if (permitData.deadline > block.timestamp) { + if (permitData.deadline >= block.timestamp) { // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations try ERC20Permit(token).permit({ owner: msg.sender, diff --git a/mainnet-contracts/src/PufferL2Depositor.sol b/mainnet-contracts/src/PufferL2Depositor.sol index 8ebbcd8..73b51f0 100644 --- a/mainnet-contracts/src/PufferL2Depositor.sol +++ b/mainnet-contracts/src/PufferL2Depositor.sol @@ -60,7 +60,7 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { // "Although one or more Error Occurred [execution reverted] Contract Execution Completed" // To avoid that, we don't want to call the permit function if it is not necessary. - if (permitData.deadline > block.timestamp) { + if (permitData.deadline >= block.timestamp) { // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations try ERC20Permit(token).permit({ owner: msg.sender,