From 04649f58ea8c6faacfd90d2b89391d3c3df07240 Mon Sep 17 00:00:00 2001 From: Benjamin Date: Fri, 14 Jun 2024 14:13:55 +0200 Subject: [PATCH] add natspec, change token logic --- mainnet-contracts/src/PufLocker.sol | 45 ++++++++++++---- mainnet-contracts/src/PufLockerStorage.sol | 15 ++++-- mainnet-contracts/src/PufToken.sol | 38 +++++++------ mainnet-contracts/src/PufferL2Depositor.sol | 25 +++++---- .../src/interface/IPufLocker.sol | 16 +++--- .../src/interface/IPufStakingPool.sol | 8 ++- mainnet-contracts/test/unit/PufLocker.t.sol | 21 ++++---- .../test/unit/PufferL2Staking.t.sol | 53 ++++++++++++++++--- 8 files changed, 150 insertions(+), 71 deletions(-) diff --git a/mainnet-contracts/src/PufLocker.sol b/mainnet-contracts/src/PufLocker.sol index 05cd978..59646c1 100644 --- a/mainnet-contracts/src/PufLocker.sol +++ b/mainnet-contracts/src/PufLocker.sol @@ -8,10 +8,23 @@ import { Address } from "@openzeppelin/contracts/utils/Address.sol"; import { PufLockerStorage } from "./PufLockerStorage.sol"; import { IPufLocker } from "./interface/IPufLocker.sol"; import { ERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol"; +import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import { Permit } from "./structs/Permit.sol"; +/** + * @title PufLocker + * @author Puffer Finance + * @custom:security-contact security@puffer.fi + */ contract PufLocker is AccessManagedUpgradeable, IPufLocker, PufLockerStorage { + using SafeERC20 for IERC20; + + constructor() { + _disableInitializers(); + } + function initialize(address accessManager) external initializer { + require(accessManager != address(0)); __AccessManaged_init(accessManager); } @@ -23,32 +36,40 @@ contract PufLocker is AccessManagedUpgradeable, IPufLocker, PufLockerStorage { _; } - function setAllowedToken(address token, bool allowed) external restricted { + /** + * @notice Creates a new staking token contract + * @dev Restricted to Puffer DAO + */ + function setIsAllowedToken(address token, bool allowed) external restricted { PufLockerData storage $ = _getPufLockerStorage(); $.allowedTokens[token] = allowed; - emit TokenAllowanceChanged(token, allowed); + emit SetTokenIsAllowed(token, allowed); } - function setLockPeriods(uint40 minLock, uint40 maxLock) external restricted { + /** + * @notice Creates a new staking token contract + * @dev Restricted to Puffer DAO + */ + function setLockPeriods(uint128 minLock, uint128 maxLock) external restricted { if (minLock > maxLock) { revert InvalidLockPeriod(); } PufLockerData storage $ = _getPufLockerStorage(); + emit LockPeriodsChanged($.minLockPeriod, minLock, $.maxLockPeriod, maxLock); $.minLockPeriod = minLock; $.maxLockPeriod = maxLock; } - function deposit(address token, uint40 lockPeriod, Permit calldata permitData) external isAllowedToken(token) { + function deposit(address token, uint128 lockPeriod, Permit calldata permitData) external isAllowedToken(token) { if (permitData.amount == 0) { revert InvalidAmount(); } PufLockerData storage $ = _getPufLockerStorage(); + if (lockPeriod < $.minLockPeriod || lockPeriod > $.maxLockPeriod) { revert InvalidLockPeriod(); } - uint40 releaseTime = uint40(block.timestamp) + lockPeriod; - // https://docs.openzeppelin.com/contracts/5.x/api/token/erc20#security_considerations try ERC20Permit(token).permit({ owner: msg.sender, @@ -60,7 +81,10 @@ contract PufLocker is AccessManagedUpgradeable, IPufLocker, PufLockerStorage { r: permitData.r }) { } catch { } - IERC20(token).transferFrom(msg.sender, address(this), permitData.amount); + IERC20(token).safeTransferFrom(msg.sender, address(this), permitData.amount); + + uint128 releaseTime = uint128(block.timestamp) + lockPeriod; + $.deposits[msg.sender][token].push(Deposit(uint128(permitData.amount), releaseTime)); emit Deposited(msg.sender, token, uint128(permitData.amount), releaseTime); @@ -82,7 +106,7 @@ contract PufLocker is AccessManagedUpgradeable, IPufLocker, PufLockerStorage { } Deposit storage userDeposit = userDeposits[index]; - if (userDeposit.releaseTime > uint40(block.timestamp)) { + if (userDeposit.releaseTime > uint128(block.timestamp)) { revert DepositStillLocked(); } @@ -124,7 +148,10 @@ contract PufLocker is AccessManagedUpgradeable, IPufLocker, PufLockerStorage { return depositPage; } - function getLockPeriods() external view returns (uint40, uint40) { + /** + * @inheritdoc IPufLocker + */ + function getLockPeriods() external view returns (uint128, uint128) { PufLockerData storage $ = _getPufLockerStorage(); return ($.minLockPeriod, $.maxLockPeriod); } diff --git a/mainnet-contracts/src/PufLockerStorage.sol b/mainnet-contracts/src/PufLockerStorage.sol index d34783b..8c5048b 100644 --- a/mainnet-contracts/src/PufLockerStorage.sol +++ b/mainnet-contracts/src/PufLockerStorage.sol @@ -5,18 +5,23 @@ import { IPufLocker } from "./interface/IPufLocker.sol"; /** * @title PufLockerStorage + * @author Puffer Finance * @dev Storage contract for PufLocker to support upgradability + * @custom:security-contact security@puffer.fi */ abstract contract PufLockerStorage { // Storage slot location for PufLocker data + + // keccak256(abi.encode(uint256(keccak256("PufLocker.storage")) - 1)) & ~bytes32(uint256(0xff)) bytes32 private constant _PUF_LOCKER_STORAGE_SLOT = - 0xed4b58c94786491f32821dd56ebc03d5f67df2b901c79c3e972343a4fbb3dfed; // keccak256("PufLocker.storage"); + 0xaf4bf4b31f04ca259733013a412d3e67552036ab2d2af267876ad7f9110e5d00; + /// @custom:storage-location erc7201:PufLocker.storage struct PufLockerData { - mapping(address => bool) allowedTokens; - mapping(address => mapping(address => IPufLocker.Deposit[])) deposits; - uint40 minLockPeriod; - uint40 maxLockPeriod; + mapping(address token => bool isAllowed) allowedTokens; + mapping(address token => mapping(address depositor => IPufLocker.Deposit[])) deposits; + uint128 minLockPeriod; + uint128 maxLockPeriod; } function _getPufLockerStorage() internal pure returns (PufLockerData storage $) { diff --git a/mainnet-contracts/src/PufToken.sol b/mainnet-contracts/src/PufToken.sol index 6a56ba9..3d0d697 100644 --- a/mainnet-contracts/src/PufToken.sol +++ b/mainnet-contracts/src/PufToken.sol @@ -8,7 +8,6 @@ import { SignatureChecker } from "@openzeppelin/contracts/utils/cryptography/Sig import { PufferL2Depositor } from "./PufferL2Depositor.sol"; import { IMigrator } from "./interface/IMigrator.sol"; import { IPufStakingPool } from "./interface/IPufStakingPool.sol"; -import { Permit } from "./structs/Permit.sol"; /** * @title Puf token @@ -46,6 +45,10 @@ contract PufToken is IPufStakingPool, ERC20, ERC20Permit { */ ERC20 public immutable TOKEN; + /** + * @notice The maximum deposit amount. + * @dev Deposit cap is in wei + */ uint256 public totalDepositCap; constructor(address token, string memory tokenName, string memory tokenSymbol, uint256 depositCap) @@ -97,24 +100,15 @@ contract PufToken is IPufStakingPool, ERC20, ERC20Permit { _; } - function depositFrom(address sender, address account, uint256 amount) - external - whenNotPaused - validateAddressAndAmount(account, amount) - onlyPufferFactory - { - _deposit(sender, account, amount); - } - /** * @notice Deposits the underlying token to receive pufToken to the `account` */ - function deposit(address account, uint256 amount) + function deposit(address from, address account, uint256 amount) external whenNotPaused validateAddressAndAmount(account, amount) { - _deposit(msg.sender, account, amount); + _deposit(from, account, amount); } /** @@ -189,21 +183,25 @@ contract PufToken is IPufStakingPool, ERC20, ERC20Permit { totalDepositCap = newDepositCap; } - function _deposit(address sender, address account, uint256 amount) internal { - TOKEN.safeTransferFrom(sender, address(this), amount); - uint256 deNormalizedTotalSupply = _denormalizeAmount(totalSupply()); + function _deposit(address depositor, address account, uint256 amount) internal { + TOKEN.safeTransferFrom(msg.sender, address(this), amount); + + uint256 normalizedAmount = _normalizeAmount(amount); - if (deNormalizedTotalSupply + amount > totalDepositCap) { + if (totalSupply() + normalizedAmount > totalDepositCap) { revert TotalDepositCapReached(); } - uint256 normalizedAmount = _normalizeAmount(amount); - // Mint puffToken to the account _mint(account, normalizedAmount); - // Using the original deposit amount in the event - emit Deposited(sender, account, amount); + // If the user is deposiing using the factory, we emit the `depositor` from the parameters + if (msg.sender == address(PUFFER_FACTORY)) { + emit Deposited(depositor, account, amount); + } else { + // If it is a direct deposit not comming from the depositor, we use msg.sender + emit Deposited(msg.sender, account, amount); + } } /** diff --git a/mainnet-contracts/src/PufferL2Depositor.sol b/mainnet-contracts/src/PufferL2Depositor.sol index 0a25118..0f98da5 100644 --- a/mainnet-contracts/src/PufferL2Depositor.sol +++ b/mainnet-contracts/src/PufferL2Depositor.sol @@ -60,17 +60,19 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { r: permitData.r }) { } catch { } - _deposit({ token: token, sender: msg.sender, account: account, amount: permitData.amount }); + IERC20(token).safeTransferFrom(msg.sender, address(this), permitData.amount); + + _deposit({ token: token, depositor: msg.sender, account: account, amount: permitData.amount }); } /** * @inheritdoc IPufferL2Depositor * @dev Restricted in this context is like `whenNotPaused` modifier from Pausable.sol */ - function depositETH(address account) external payable onlySupportedTokens(WETH) restricted { + function depositETH(address account) external payable restricted { IWETH(WETH).deposit{ value: msg.value }(); - _deposit({ token: WETH, sender: address(this), account: account, amount: msg.value }); + _deposit({ token: WETH, depositor: msg.sender, account: account, amount: msg.value }); } /** @@ -93,12 +95,14 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { emit SetIsMigratorAllowed(migrator, allowed); } + /** + * @notice Changes the status of `migrator` to `allowed` + * @dev Restricted to Puffer DAO + */ function setDepositCap(address token, uint256 newDepositCap) external onlySupportedTokens(token) restricted { PufToken pufToken = PufToken(tokens[token]); - uint256 oldDepositCap = pufToken.totalDepositCap(); + emit DepositCapUpdated(token, pufToken.totalDepositCap(), newDepositCap); pufToken.setDepositCap(newDepositCap); - - emit DepositCapUpdated(token, oldDepositCap, newDepositCap); } /** @@ -107,13 +111,12 @@ contract PufferL2Depositor is IPufferL2Depositor, AccessManaged { */ function revertIfPaused() external restricted { } - function _deposit(address token, address sender, address account, uint256 amount) internal { + function _deposit(address token, address depositor, address account, uint256 amount) internal { PufToken pufToken = PufToken(tokens[token]); - if (sender == address(this)) { - IERC20(token).safeIncreaseAllowance(address(pufToken), amount); - } - pufToken.depositFrom(sender, account, amount); + IERC20(token).safeIncreaseAllowance(address(pufToken), amount); + + pufToken.deposit(depositor, account, amount); emit DepositedToken(token, msg.sender, account, amount); } diff --git a/mainnet-contracts/src/interface/IPufLocker.sol b/mainnet-contracts/src/interface/IPufLocker.sol index c25b212..823b389 100644 --- a/mainnet-contracts/src/interface/IPufLocker.sol +++ b/mainnet-contracts/src/interface/IPufLocker.sol @@ -5,7 +5,6 @@ import { Permit } from "../structs/Permit.sol"; interface IPufLocker { // Custom error messages - error NotAdmin(); error TokenNotAllowed(); error InvalidAmount(); error InvalidLockPeriod(); @@ -15,16 +14,17 @@ interface IPufLocker { error InvalidRecipientAddress(); // Events - event TokenAllowanceChanged(address indexed token, bool allowed); - event Deposited(address indexed user, address indexed token, uint128 amount, uint40 releaseTime); + event SetTokenIsAllowed(address indexed token, bool allowed); + event Deposited(address indexed user, address indexed token, uint128 amount, uint128 releaseTime); event Withdrawn(address indexed user, address indexed token, uint128 amount, address recipient); + event LockPeriodsChanged(uint128 previousMinLock, uint128 newMinLock, uint128 previousMaxLock, uint128 newMaxLock); // Functions - function setAllowedToken(address token, bool allowed) external; + function setIsAllowedToken(address token, bool allowed) external; - function setLockPeriods(uint40 minLockPeriod, uint40 maxLockPeriod) external; + function setLockPeriods(uint128 minLockPeriod, uint128 maxLockPeriod) external; - function deposit(address token, uint40 lockPeriod, Permit calldata permitData) external; + function deposit(address token, uint128 lockPeriod, Permit calldata permitData) external; function withdraw(address token, uint256[] calldata depositIndexes, address recipient) external; @@ -32,10 +32,10 @@ interface IPufLocker { external view returns (Deposit[] memory); - function getLockPeriods() external view returns (uint40, uint40); + function getLockPeriods() external view returns (uint128, uint128); struct Deposit { uint128 amount; - uint40 releaseTime; + uint128 releaseTime; } } diff --git a/mainnet-contracts/src/interface/IPufStakingPool.sol b/mainnet-contracts/src/interface/IPufStakingPool.sol index 7add8eb..07d83b3 100644 --- a/mainnet-contracts/src/interface/IPufStakingPool.sol +++ b/mainnet-contracts/src/interface/IPufStakingPool.sol @@ -21,7 +21,13 @@ interface IPufStakingPool { address indexed depositor, address indexed destination, address indexed migratorContract, uint256 amount ); - function deposit(address account, uint256 amount) external; + /** + * @notice Function used to deposit the undelrying token + * @param depositor is the msg.sender or a parameter passed from the PufferL2Depositor + * @param account is the recipient of the deposit + * @param amount is the deposit amount + */ + function deposit(address depositor, address account, uint256 amount) external; function withdraw(address recipient, uint256 amount) external; diff --git a/mainnet-contracts/test/unit/PufLocker.t.sol b/mainnet-contracts/test/unit/PufLocker.t.sol index 8f2d702..80a0d5f 100644 --- a/mainnet-contracts/test/unit/PufLocker.t.sol +++ b/mainnet-contracts/test/unit/PufLocker.t.sol @@ -10,11 +10,12 @@ 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"; import { Permit } from "../../src/structs/Permit.sol"; -contract PufferL2Staking is UnitTestHelper { +contract PufLockerTest is UnitTestHelper { PufLocker public pufLocker; ERC20Mock public mockToken; address bob = makeAddr("bob"); @@ -24,8 +25,10 @@ contract PufferL2Staking is UnitTestHelper { mockToken = new ERC20Mock("DAI", "DAI"); - pufLocker = new PufLocker(); - pufLocker.initialize(address(accessManager)); + address pufLockerImpl = address(new PufLocker()); + pufLocker = PufLocker( + address(new ERC1967Proxy(pufLockerImpl, abi.encodeCall(PufLocker.initialize, (address(accessManager))))) + ); // Access setup @@ -40,7 +43,7 @@ contract PufferL2Staking is UnitTestHelper { ); bytes4[] memory multisigSelectors = new bytes4[](2); - multisigSelectors[0] = PufLocker.setAllowedToken.selector; + multisigSelectors[0] = PufLocker.setIsAllowedToken.selector; multisigSelectors[1] = PufLocker.setLockPeriods.selector; calldatas[1] = abi.encodeWithSelector( @@ -56,15 +59,15 @@ contract PufferL2Staking is UnitTestHelper { // Set the lock periods vm.startPrank(OPERATIONS_MULTISIG); - pufLocker.setLockPeriods(60, 86400); // Min 1 minute, Max 1 day - pufLocker.setAllowedToken(address(mockToken), true); + pufLocker.setLockPeriods(1 minutes, 1 days); + pufLocker.setIsAllowedToken(address(mockToken), true); mockToken.mint(bob, 1000e18); } function test_SetAllowedToken_Success() public { vm.startPrank(OPERATIONS_MULTISIG); - pufLocker.setAllowedToken(address(mockToken), true); + pufLocker.setIsAllowedToken(address(mockToken), true); vm.startPrank(bob); uint256 amount = 10e18; @@ -77,13 +80,13 @@ contract PufferL2Staking is UnitTestHelper { function testRevert_SetAllowedToken_Unauthorized() public { vm.startPrank(bob); vm.expectRevert(abi.encodeWithSelector(IAccessManaged.AccessManagedUnauthorized.selector, bob)); - pufLocker.setAllowedToken(address(mockToken), true); + pufLocker.setIsAllowedToken(address(mockToken), true); } function test_SetLockPeriods_Success() public { vm.startPrank(OPERATIONS_MULTISIG); pufLocker.setLockPeriods(120, 172800); // Min 2 minutes, Max 2 days - (uint40 minLock, uint40 maxLock) = pufLocker.getLockPeriods(); + (uint128 minLock, uint128 maxLock) = pufLocker.getLockPeriods(); assertEq(minLock, 120, "Min lock period should be 120"); assertEq(maxLock, 172800, "Max lock period should be 172800"); } diff --git a/mainnet-contracts/test/unit/PufferL2Staking.t.sol b/mainnet-contracts/test/unit/PufferL2Staking.t.sol index bab4640..21d22fa 100644 --- a/mainnet-contracts/test/unit/PufferL2Staking.t.sol +++ b/mainnet-contracts/test/unit/PufferL2Staking.t.sol @@ -133,7 +133,7 @@ contract PufferL2Staking is UnitTestHelper { vm.startPrank(bob); - dai.approve(depositor.tokens(address(dai)), amount); + dai.approve(address(depositor), amount); vm.expectEmit(true, true, true, true); emit IPufferL2Depositor.DepositedToken(address(dai), bob, bob, amount); @@ -154,7 +154,7 @@ contract PufferL2Staking is UnitTestHelper { sixDecimal.mint(bob, amount); vm.startPrank(bob); - sixDecimal.approve(depositor.tokens(address(sixDecimal)), amount); + sixDecimal.approve(address(depositor), amount); vm.expectEmit(true, true, true, true); emit IPufferL2Depositor.DepositedToken(address(sixDecimal), bob, bob, amount); @@ -181,7 +181,7 @@ contract PufferL2Staking is UnitTestHelper { twentyTwoDecimal.mint(bob, amount); vm.startPrank(bob); - twentyTwoDecimal.approve(depositor.tokens(address(twentyTwoDecimal)), amount); + twentyTwoDecimal.approve(address(depositor), amount); vm.expectEmit(true, true, true, true); emit IPufferL2Depositor.DepositedToken(address(twentyTwoDecimal), bob, bob, amount); @@ -231,7 +231,7 @@ contract PufferL2Staking is UnitTestHelper { vm.startPrank(bob); weth.deposit{ value: amount }(); - weth.approve(depositor.tokens(address(weth)), amount); + weth.approve(address(depositor), amount); // weth.permit triggers weth.fallback() and it doesn't revert vm.expectEmit(true, true, true, true); @@ -249,7 +249,6 @@ contract PufferL2Staking is UnitTestHelper { vm.deal(bob, amount); vm.startPrank(bob); - dai.approve(address(depositor), amount); vm.expectEmit(true, true, true, true); emit IPufferL2Depositor.DepositedToken(address(weth), bob, bob, amount); @@ -262,8 +261,8 @@ contract PufferL2Staking is UnitTestHelper { emit IPufStakingPool.Withdrawn(bob, bob, amount); pufToken.withdraw(bob, amount); } - // direct deposit to the token contract, without using the depositor + // direct deposit to the token contract, without using the depositor function test_direct_deposit_dai(uint256 amount) public { vm.assume(amount > 0); dai.mint(bob, amount); @@ -276,7 +275,7 @@ contract PufferL2Staking is UnitTestHelper { vm.expectEmit(true, true, true, true); emit IPufStakingPool.Deposited(bob, bob, amount); - pufToken.deposit(bob, amount); + pufToken.deposit(bob, bob, amount); } // Allow migrator @@ -357,6 +356,14 @@ contract PufferL2Staking is UnitTestHelper { pufToken.migrate(500, address(0), bob); } + // 0 ETH deposit + function testRevert_zero_eth_deposit() public { + vm.startPrank(bob); + + vm.expectRevert(IPufStakingPool.InvalidAmount.selector); + depositor.depositETH{ value: 0 }(bob); + } + // Mock address 123 is not allowed to be migrator function testRevert_migrate_with_contract_that_is_not_allowed() public { PufToken pufToken = PufToken(depositor.tokens(address(weth))); @@ -404,6 +411,36 @@ contract PufferL2Staking is UnitTestHelper { assertEq(pufToken.totalDepositCap(), newDepositCap, "Supply cap should be updated"); } + function test_depositCap_changes_with_withdrawal() public { + // sets cap to 500000 ether + test_SetDepositCap(); + + dai.mint(bob, 5000000 ether); + + PufToken pufToken = PufToken(depositor.tokens(address(dai))); + + vm.startPrank(bob); + + dai.approve(address(pufToken), type(uint256).max); + + // deposit max amount + vm.expectEmit(true, true, true, true); + emit IPufStakingPool.Deposited(bob, bob, pufToken.totalDepositCap()); + pufToken.deposit(bob, bob, pufToken.totalDepositCap()); + + // deposit reverts + vm.expectRevert(IPufStakingPool.TotalDepositCapReached.selector); + pufToken.deposit(bob, bob, 1 ether); + + // withdraw some tokens + pufToken.withdraw(bob, 10 ether); + + // now the deposit is available again + vm.expectEmit(true, true, true, true); + emit IPufStakingPool.Deposited(bob, bob, 7 ether); + pufToken.deposit(bob, bob, 7 ether); + } + function testRevert_SetDepositCap_Unauthorized() public { // Try setting the supply cap from an unauthorized address vm.startPrank(bob); @@ -426,7 +463,7 @@ contract PufferL2Staking is UnitTestHelper { dai.mint(bob, amount); PufToken pufToken = PufToken(depositor.tokens(address(dai))); dai.approve(address(pufToken), amount); - pufToken.deposit(bob, amount); + pufToken.deposit(bob, bob, amount); // Try setting the supply cap below the current total supply vm.startPrank(OPERATIONS_MULTISIG);