From d18df07b287e14031289f9d3c25bc56843563e88 Mon Sep 17 00:00:00 2001 From: r4bbit <445106+0x-r4bbit@users.noreply.github.com> Date: Wed, 19 Jun 2024 11:26:03 +0200 Subject: [PATCH] refactor(StakeManager): make function names more descriptive Some of the functions on our contracts were confusing. This commit changes them so they describe what they actually do. --- certora/specs/StakeManager.spec | 8 +- certora/specs/StakeManagerProcessAccount.spec | 2 +- certora/specs/StakeManagerStartMigration.spec | 2 +- certora/specs/StakeVault.spec | 4 +- contracts/StakeManager.sol | 131 ++++++++++-------- test/StakeManager.t.sol | 26 +--- 6 files changed, 86 insertions(+), 87 deletions(-) diff --git a/certora/specs/StakeManager.spec b/certora/specs/StakeManager.spec index 79f41c0..7f69ed7 100644 --- a/certora/specs/StakeManager.spec +++ b/certora/specs/StakeManager.spec @@ -3,9 +3,9 @@ methods { function staked.balanceOf(address) external returns (uint256) envfree; function totalSupplyBalance() external returns (uint256) envfree; function totalSupplyMP() external returns (uint256) envfree; - function oldManager() external returns (address) envfree; + function previousManager() external returns (address) envfree; function _.migrateFrom(address, bool, StakeManager.Account) external => NONDET; - function _.increaseMPFromMigration(uint256) external => NONDET; + function _.increaseTotalMP(uint256) external => NONDET; function _.migrationInitialize(uint256,uint256,uint256,uint256) external => NONDET; function accounts(address) external returns(address, uint256, uint256, uint256, uint256, uint256, uint256) envfree; function Math.mulDiv(uint256 a, uint256 b, uint256 c) internal returns uint256 => mulDivSummary(a,b,c); @@ -54,14 +54,14 @@ function isMigrationfunction(method f) returns bool { cases where it is zero. specifically no externall call to the migration contract */ function simplification(env e) { require currentContract.migration == 0; - require currentContract.oldManager() == 0; + require currentContract.previousManager() == 0; require e.msg.sender != 0; } definition requiresPreviousManager(method f) returns bool = ( f.selector == sig:migrationInitialize(uint256,uint256,uint256,uint256).selector || f.selector == sig:migrateFrom(address,bool,StakeManager.Account).selector || - f.selector == sig:increaseMPFromMigration(uint256).selector + f.selector == sig:increaseTotalMP(uint256).selector ); definition requiresNextManager(method f) returns bool = ( diff --git a/certora/specs/StakeManagerProcessAccount.spec b/certora/specs/StakeManagerProcessAccount.spec index d2da58a..a9c7ae7 100644 --- a/certora/specs/StakeManagerProcessAccount.spec +++ b/certora/specs/StakeManagerProcessAccount.spec @@ -31,7 +31,7 @@ hook Sstore accounts[KEY address addr].balance uint256 newValue (uint256 oldValu definition requiresPreviousManager(method f) returns bool = ( f.selector == sig:migrationInitialize(uint256,uint256,uint256,uint256).selector || f.selector == sig:migrateFrom(address,bool,StakeManager.Account).selector || - f.selector == sig:increaseMPFromMigration(uint256).selector + f.selector == sig:increaseTotalMP(uint256).selector ); definition requiresNextManager(method f) returns bool = ( diff --git a/certora/specs/StakeManagerStartMigration.spec b/certora/specs/StakeManagerStartMigration.spec index 2e1a3b9..0355cd1 100644 --- a/certora/specs/StakeManagerStartMigration.spec +++ b/certora/specs/StakeManagerStartMigration.spec @@ -5,7 +5,7 @@ methods { function staked.balanceOf(address) external returns (uint256) envfree; function totalSupplyBalance() external returns (uint256) envfree; function totalSupplyMP() external returns (uint256) envfree; - function oldManager() external returns (address) envfree; + function previousManager() external returns (address) envfree; function accounts(address) external returns(address, uint256, uint256, uint256, uint256, uint256, uint256) envfree; function _.migrationInitialize(uint256,uint256,uint256,uint256) external => DISPATCHER(true); diff --git a/certora/specs/StakeVault.spec b/certora/specs/StakeVault.spec index 4ba8650..6efc393 100644 --- a/certora/specs/StakeVault.spec +++ b/certora/specs/StakeVault.spec @@ -7,7 +7,7 @@ methods { function ERC20A.totalSupply() external returns(uint256) envfree; function StakeManager.accounts(address) external returns(address, uint256, uint256, uint256, uint256, uint256, uint256) envfree; function _.migrateFrom(address, bool, StakeManager.Account) external => DISPATCHER(true); - function _.increaseMPFromMigration(uint256) external => DISPATCHER(true); + function _.increaseTotalMP(uint256) external => DISPATCHER(true); function _.owner() external => DISPATCHER(true); function Math.mulDiv(uint256 a, uint256 b, uint256 c) internal returns uint256 => mulDivSummary(a,b,c); } @@ -27,7 +27,7 @@ function getAccountBalance(address addr) returns uint256 { definition isMigrationFunction(method f) returns bool = ( f.selector == sig:stakeManager.migrationInitialize(uint256,uint256,uint256,uint256).selector || f.selector == sig:stakeManager.migrateFrom(address,bool,StakeManager.Account).selector || - f.selector == sig:stakeManager.increaseMPFromMigration(uint256).selector || + f.selector == sig:stakeManager.increaseTotalMP(uint256).selector || f.selector == sig:stakeManager.startMigration(address).selector ); diff --git a/contracts/StakeManager.sol b/contracts/StakeManager.sol index 2110bb1..24fa741 100644 --- a/contracts/StakeManager.sol +++ b/contracts/StakeManager.sol @@ -53,7 +53,7 @@ contract StakeManager is Ownable { uint256 public totalSupplyMP; uint256 public totalSupplyBalance; StakeManager public migration; - StakeManager public immutable oldManager; + StakeManager public immutable previousManager; ERC20 public immutable stakedToken; /** @@ -66,7 +66,7 @@ contract StakeManager is Ownable { _; } - modifier onlyInitialized(address account) { + modifier onlyAccountInitialized(address account) { if (accounts[account].lockUntil == 0) { revert StakeManager__AccountNotInitialized(); } @@ -76,7 +76,7 @@ contract StakeManager is Ownable { /** * @notice Only callable when migration is not initialized. */ - modifier noMigration() { + modifier noPendingMigration() { if (address(migration) != address(0)) { revert StakeManager__PendingMigration(); } @@ -86,7 +86,7 @@ contract StakeManager is Ownable { /** * @notice Only callable when migration is initialized. */ - modifier onlyMigration() { + modifier onlyPendingMigration() { if (address(migration) == address(0)) { revert StakeManager__NoPendingMigration(); } @@ -96,8 +96,8 @@ contract StakeManager is Ownable { /** * @notice Only callable from old manager. */ - modifier onlyOldManager() { - if (msg.sender != address(oldManager)) { + modifier onlyPreviousManager() { + if (msg.sender != address(previousManager)) { revert StakeManager__SenderIsNotPreviousStakeManager(); } _; @@ -106,7 +106,7 @@ contract StakeManager is Ownable { /** * @notice Process epoch if it has ended */ - modifier processEpoch() { + modifier finalizeEpoch() { if (block.timestamp >= epochEnd() && address(migration) == address(0)) { //finalize current epoch epochs[currentEpoch].epochReward = epochReward(); @@ -119,9 +119,9 @@ contract StakeManager is Ownable { _; } - constructor(address _stakedToken, address _oldManager) { + constructor(address _stakedToken, address _previousManager) { epochs[0].startTime = block.timestamp; - oldManager = StakeManager(_oldManager); + previousManager = StakeManager(_previousManager); stakedToken = ERC20(_stakedToken); } @@ -132,8 +132,9 @@ contract StakeManager is Ownable { * * @dev Reverts when resulting locked time is not in range of [MIN_LOCKUP_PERIOD, MAX_LOCKUP_PERIOD] */ - function stake(uint256 _amount, uint256 _timeToIncrease) external onlyVault noMigration processEpoch { + function stake(uint256 _amount, uint256 _timeToIncrease) external onlyVault noPendingMigration finalizeEpoch { Account storage account = accounts[msg.sender]; + if (account.lockUntil == 0) { // account not initialized account.lockUntil = block.timestamp; @@ -142,18 +143,22 @@ contract StakeManager is Ownable { } else { _processAccount(account, currentEpoch); } + uint256 deltaTime = 0; + if (_timeToIncrease > 0) { uint256 lockUntil = account.lockUntil + _timeToIncrease; if (lockUntil < block.timestamp) { revert StakeManager__InvalidLockTime(); } + deltaTime = lockUntil - block.timestamp; if (deltaTime < MIN_LOCKUP_PERIOD || deltaTime > MAX_LOCKUP_PERIOD) { revert StakeManager__InvalidLockTime(); } } - _mintIntialMP(account, deltaTime, _amount); + _mintInitialMP(account, deltaTime, _amount); + //update storage totalSupplyBalance += _amount; account.balance += _amount; @@ -163,7 +168,13 @@ contract StakeManager is Ownable { /** * leaves the staking pool and withdraws all funds; */ - function unstake(uint256 _amount) external onlyVault onlyInitialized(msg.sender) noMigration processEpoch { + function unstake(uint256 _amount) + external + onlyVault + onlyAccountInitialized(msg.sender) + noPendingMigration + finalizeEpoch + { Account storage account = accounts[msg.sender]; if (_amount > account.balance) { revert StakeManager__InsufficientFunds(); @@ -190,7 +201,13 @@ contract StakeManager is Ownable { * * @dev Reverts when resulting locked time is not in range of [MIN_LOCKUP_PERIOD, MAX_LOCKUP_PERIOD] */ - function lock(uint256 _timeToIncrease) external onlyVault onlyInitialized(msg.sender) noMigration processEpoch { + function lock(uint256 _timeToIncrease) + external + onlyVault + onlyAccountInitialized(msg.sender) + noPendingMigration + finalizeEpoch + { Account storage account = accounts[msg.sender]; _processAccount(account, currentEpoch); uint256 lockUntil = account.lockUntil; @@ -205,17 +222,17 @@ contract StakeManager is Ownable { if (deltaTime < MIN_LOCKUP_PERIOD || deltaTime > MAX_LOCKUP_PERIOD) { revert StakeManager__InvalidLockTime(); } - _mintIntialMP(account, _timeToIncrease, 0); + _mintInitialMP(account, _timeToIncrease, 0); //update account storage account.lockUntil = lockUntil; } /** * @notice Release rewards for current epoch and increase epoch. - * @dev only executes the prerequisite modifier processEpoch + * @dev only executes the prerequisite modifier finalizeEpoch */ - function executeEpoch() external noMigration processEpoch { - return; //see modifier processEpoch + function executeEpoch() external noPendingMigration finalizeEpoch { + return; //see modifier finalizeEpoch } /** @@ -223,7 +240,14 @@ contract StakeManager is Ownable { * @param _vault Referred account * @param _limitEpoch Until what epoch it should be executed */ - function executeAccount(address _vault, uint256 _limitEpoch) external onlyInitialized(_vault) processEpoch { + function executeAccount( + address _vault, + uint256 _limitEpoch + ) + external + onlyAccountInitialized(_vault) + finalizeEpoch + { _processAccount(accounts[_vault], _limitEpoch); } @@ -239,7 +263,7 @@ contract StakeManager is Ownable { * @notice starts migration to new StakeManager * @param _migration new StakeManager */ - function startMigration(StakeManager _migration) external onlyOwner noMigration processEpoch { + function startMigration(StakeManager _migration) external onlyOwner noPendingMigration finalizeEpoch { if (_migration == this || address(_migration) == address(0)) { revert StakeManager__InvalidMigration(); } @@ -263,7 +287,7 @@ contract StakeManager is Ownable { uint256 _epochStartTime ) external - onlyOldManager + onlyPreviousManager { if (address(migration) != address(0)) { revert StakeManager__PendingMigration(); @@ -280,7 +304,7 @@ contract StakeManager is Ownable { /** * @notice Transfer current epoch funds for migrated manager */ - function transferNonPending() external onlyMigration { + function transferNonPending() external onlyPendingMigration { stakedToken.transfer(address(migration), epochReward()); } @@ -291,9 +315,9 @@ contract StakeManager is Ownable { function migrateTo(bool _acceptMigration) external onlyVault - onlyInitialized(msg.sender) - onlyMigration - processEpoch + onlyAccountInitialized(msg.sender) + onlyPendingMigration + finalizeEpoch returns (StakeManager newManager) { _processAccount(accounts[msg.sender], currentEpoch); @@ -312,7 +336,7 @@ contract StakeManager is Ownable { * @param _account Account data * @param _acceptMigration If account should be stored or its MP/balance supply reduced */ - function migrateFrom(address _vault, bool _acceptMigration, Account memory _account) external onlyOldManager { + function migrateFrom(address _vault, bool _acceptMigration, Account memory _account) external onlyPreviousManager { if (_acceptMigration) { accounts[_vault] = _account; } else { @@ -324,10 +348,10 @@ contract StakeManager is Ownable { /** * @dev Only callable from old manager. * @notice Increase total MP from old manager - * @param _increasedMP amount MP increased on account after migration initialized + * @param _amount amount MP increased on account after migration initialized */ - function increaseMPFromMigration(uint256 _increasedMP) external onlyOldManager { - totalSupplyMP += _increasedMP; + function increaseTotalMP(uint256 _amount) external onlyPreviousManager { + totalSupplyMP += _amount; } /** @@ -359,41 +383,38 @@ contract StakeManager is Ownable { } mpDifference = account.currentMP - mpDifference; if (address(migration) != address(0)) { - migration.increaseMPFromMigration(mpDifference); + migration.increaseTotalMP(mpDifference); } else if (userEpoch == currentEpoch) { _mintMP(account, block.timestamp, epochs[currentEpoch]); } } /** - * @notice Mint initial multiplier points for given balance and time - * @dev if increased balance, increases difference of increased balance for current remaining lock time + * @notice Mint initial multiplier points for given staking amount and time + * @dev if amount is greater 0, it increases difference of amount for current remaining lock time * @dev if increased lock time, increases difference of total new balance for increased lock time * @param account Account to mint multiplier points * @param increasedLockTime increased lock time - * @param increasedBalance increased balance + * @param amount amount to stake */ - function _mintIntialMP(Account storage account, uint256 increasedLockTime, uint256 increasedBalance) private { - uint256 increasedMP; - if (increasedBalance > 0) { - increasedMP += increasedBalance; //initial multiplier points + function _mintInitialMP(Account storage account, uint256 increasedLockTime, uint256 amount) private { + uint256 mpToMint; + if (amount > 0) { + mpToMint += amount; //initial multiplier points if (block.timestamp < account.lockUntil) { //increasing balance on locked account? //bonus for remaining previously locked time of new balance. - increasedMP += _getIncreasedMP(increasedBalance, account.lockUntil - block.timestamp); + mpToMint += _getMPToMint(amount, account.lockUntil - block.timestamp); } } if (increasedLockTime > 0) { //bonus for increased lock time - increasedMP += _getIncreasedMP(account.balance + increasedBalance, increasedLockTime); + mpToMint += _getMPToMint(account.balance + amount, increasedLockTime); } - - //does not check for MAX_BOOST - //update storage - totalSupplyMP += increasedMP; - account.initialMP += increasedMP; - account.currentMP += increasedMP; + totalSupplyMP += mpToMint; + account.initialMP += mpToMint; + account.currentMP += mpToMint; account.lastMint = block.timestamp; } @@ -404,8 +425,8 @@ contract StakeManager is Ownable { * @param epoch Epoch to increment total supply */ function _mintMP(Account storage account, uint256 processTime, Epoch storage epoch) private { - uint256 increasedMP = _capMaxMPIncrease( //check for MAX_BOOST - _getIncreasedMP(account.balance, processTime - account.lastMint), + uint256 increasedMP = _getMaxMPToMint( //check for MAX_BOOST + _getMPToMint(account.balance, processTime - account.lastMint), account.balance, account.initialMP, account.currentMP @@ -420,14 +441,14 @@ contract StakeManager is Ownable { /** * @notice Calculates maximum multiplier point increase for given balance - * @param _increasedMP tested value + * @param _mpToMint tested value * @param _balance balance of account * @param _currentMP current multiplier point of the account * @param _initialMP initial multiplier point of the account * @return _maxToIncrease maximum multiplier point increase */ - function _capMaxMPIncrease( - uint256 _increasedMP, + function _getMaxMPToMint( + uint256 _mpToMint, uint256 _balance, uint256 _initialMP, uint256 _currentMP @@ -437,23 +458,23 @@ contract StakeManager is Ownable { returns (uint256 _maxToIncrease) { // Maximum multiplier point for given balance - _maxToIncrease = _getIncreasedMP(_balance, MAX_BOOST * YEAR) + _initialMP; - if (_increasedMP + _currentMP > _maxToIncrease) { + _maxToIncrease = _getMPToMint(_balance, MAX_BOOST * YEAR) + _initialMP; + if (_mpToMint + _currentMP > _maxToIncrease) { //reached cap when increasing MP return _maxToIncrease - _currentMP; //how much left to reach cap } else { //not reached capw hen increasing MP - return _increasedMP; //just return tested value + return _mpToMint; //just return tested value } } /** - * @notice Calculates increased multiplier points for given balance and time + * @notice Calculates multiplier points to mint for given balance and time * @param _balance balance of account * @param _deltaTime time difference - * @return _increasedMP increased multiplier points + * @return multiplier points to mint */ - function _getIncreasedMP(uint256 _balance, uint256 _deltaTime) private pure returns (uint256 _increasedMP) { + function _getMPToMint(uint256 _balance, uint256 _deltaTime) private pure returns (uint256) { return Math.mulDiv(_balance, _deltaTime, YEAR) * MP_APY; } diff --git a/test/StakeManager.t.sol b/test/StakeManager.t.sol index 6e47c4f..01d5cc9 100644 --- a/test/StakeManager.t.sol +++ b/test/StakeManager.t.sol @@ -34,7 +34,7 @@ contract StakeManagerTest is Test { assertEq(stakeManager.totalSupplyMP(), 0); assertEq(stakeManager.totalSupplyBalance(), 0); assertEq(address(stakeManager.stakedToken()), stakeToken); - assertEq(address(stakeManager.oldManager()), address(0)); + assertEq(address(stakeManager.previousManager()), address(0)); assertEq(stakeManager.totalSupply(), 0); } @@ -183,7 +183,6 @@ contract StakeTest is StakeManagerTest { function test_restakeJustLock() public { uint256 lockToIncrease = stakeManager.MIN_LOCKUP_PERIOD(); uint256 stakeAmount = 100; - uint256 stakeAmount2 = 50; uint256 mintAmount = stakeAmount * 10; StakeVault userVault = _createStakingAccount(testUser, stakeAmount, 0, mintAmount); StakeVault userVault2 = _createStakingAccount(testUser2, stakeAmount, lockToIncrease, mintAmount); @@ -629,27 +628,6 @@ contract ExecuteAccountTest is StakeManagerTest { } } - function internal_logAccount(address vault) internal { - ( - address rewardAddress, - uint256 balance, - uint256 initialMP, - uint256 currentMP, - uint256 lastMint, - uint256 lockUntil, - uint256 epoch - ) = stakeManager.accounts(vault); - console.log("==============="); - console.log("rewardAddress :", rewardAddress); - console.log("##### balance :", balance); - console.log("### initialMP :", initialMP); - console.log("### currentMP :", currentMP); - console.log("#### lastMint :", lastMint); - console.log("### lockUntil :", lockUntil); - console.log("####### epoch :", epoch); - console.log("==============="); - } - function test_UpdateEpoch() public { } function test_PayRewards() public { } @@ -716,7 +694,7 @@ contract MigrationStakeManagerTest is StakeManagerTest { assertEq(newStakeManager.totalSupplyMP(), 0); assertEq(newStakeManager.totalSupplyBalance(), 0); assertEq(address(newStakeManager.stakedToken()), stakeToken); - assertEq(address(newStakeManager.oldManager()), address(stakeManager)); + assertEq(address(newStakeManager.previousManager()), address(stakeManager)); assertEq(newStakeManager.totalSupply(), 0); }