refactor(RewardsStreamerMP): use Math.mulDiv to reduce likelyhood of

precision loss Closes #85
vacp2p · Dec 6, 2024 · ebfb426 · ebfb426
1 parent bca74ce
commit ebfb426
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 75 deletions.
diff --git a/.gas-report b/.gas-report
@@ -15,13 +15,12 @@
 | src/RewardsStreamerMP.sol:RewardsStreamerMP contract |                 |        |        |        |         |
 |------------------------------------------------------|-----------------|--------|--------|--------|---------|
 | Deployment Cost                                      | Deployment Size |        |        |        |         |
-| 2494954                                              | 11497           |        |        |        |         |
+| 2513064                                              | 11581           |        |        |        |         |
 | Function Name                                        | min             | avg    | median | max    | # calls |
 | MAX_LOCKUP_PERIOD                                    | 294             | 294    | 294    | 294    | 23      |
 | MAX_MULTIPLIER                                       | 251             | 251    | 251    | 251    | 30      |
 | MIN_LOCKUP_PERIOD                                    | 297             | 297    | 297    | 297    | 11      |
 | MP_RATE_PER_YEAR                                     | 253             | 253    | 253    | 253    | 3       |
-| SCALE_FACTOR                                         | 273             | 273    | 273    | 273    | 41      |
 | STAKING_TOKEN                                        | 2428            | 2428   | 2428   | 2428   | 292     |
 | emergencyModeEnabled                                 | 2398            | 2398   | 2398   | 2398   | 7       |
 | enableEmergencyMode                                  | 2463            | 19370  | 24655  | 24655  | 8       |
@@ -34,24 +33,24 @@
 | initialize                                           | 115611          | 115611 | 115611 | 115611 | 59      |
 | isTrustedCodehash                                    | 519             | 519    | 519    | 519    | 231     |
 | lastRewardTime                                       | 373             | 1373   | 1373   | 2373   | 2       |
-| leave                                                | 56613           | 56613  | 56613  | 56613  | 1       |
-| lock                                                 | 12041           | 34170  | 16370  | 74099  | 3       |
+| leave                                                | 56289           | 56289  | 56289  | 56289  | 1       |
+| lock                                                 | 12041           | 34121  | 16370  | 73953  | 3       |
 | proxiableUUID                                        | 331             | 331    | 331    | 331    | 3       |
 | registerVault                                        | 55866           | 72745  | 72966  | 72966  | 233     |
 | rewardEndTime                                        | 373             | 1373   | 1373   | 2373   | 2       |
 | rewardStartTime                                      | 352             | 1352   | 1352   | 2352   | 2       |
-| rewardsBalanceOf                                     | 1294            | 1294   | 1294   | 1294   | 4       |
-| setReward                                            | 2561            | 50875  | 60256  | 102573 | 7       |
+| rewardsBalanceOf                                     | 1317            | 1317   | 1317   | 1317   | 4       |
+| setReward                                            | 2561            | 50870  | 60256  | 102573 | 7       |
 | setTrustedCodehash                                   | 26243           | 26243  | 26243  | 26243  | 59      |
-| stake                                                | 131082          | 170235 | 177899 | 198378 | 66      |
+| stake                                                | 131082          | 170202 | 177899 | 198232 | 66      |
 | totalMP                                              | 373             | 373    | 373    | 373    | 81      |
 | totalMaxMP                                           | 350             | 350    | 350    | 350    | 81      |
 | totalRewardsAccrued                                  | 351             | 351    | 351    | 351    | 3       |
 | totalRewardsSupply                                   | 1003            | 1964   | 1767   | 6743   | 30      |
 | totalStaked                                          | 396             | 396    | 396    | 396    | 82      |
-| unstake                                              | 60706           | 61254  | 60706  | 64269  | 13      |
-| updateAccountMP                                      | 15464           | 18542  | 17966  | 35066  | 21      |
-| updateGlobalState                                    | 11066           | 28094  | 25315  | 110295 | 21      |
+| unstake                                              | 60382           | 60919  | 60382  | 63877  | 13      |
+| updateAccountMP                                      | 15396           | 18474  | 17898  | 34998  | 21      |
+| updateGlobalState                                    | 11066           | 28023  | 25227  | 110262 | 21      |
 | upgradeToAndCall                                     | 3225            | 9387   | 10926  | 10936  | 5       |
 
 
@@ -64,7 +63,6 @@
 | MAX_MULTIPLIER                                       | 678             | 1578  | 678    | 5178   | 30      |
 | MIN_LOCKUP_PERIOD                                    | 724             | 3996  | 5224   | 5224   | 11      |
 | MP_RATE_PER_YEAR                                     | 680             | 680   | 680    | 680    | 3       |
-| SCALE_FACTOR                                         | 700             | 700   | 700    | 700    | 41      |
 | STAKING_TOKEN                                        | 7355            | 7355  | 7355   | 7355   | 292     |
 | emergencyModeEnabled                                 | 7325            | 7325  | 7325   | 7325   | 7       |
 | enableEmergencyMode                                  | 28458           | 45359 | 50643  | 50643  | 8       |
@@ -79,16 +77,16 @@
 | lastRewardTime                                       | 800             | 1800  | 1800   | 2800   | 2       |
 | rewardEndTime                                        | 800             | 1800  | 1800   | 2800   | 2       |
 | rewardStartTime                                      | 779             | 4029  | 4029   | 7279   | 2       |
-| rewardsBalanceOf                                     | 1724            | 1724  | 1724   | 1724   | 4       |
-| setReward                                            | 28841           | 77189 | 86614  | 128859 | 7       |
+| rewardsBalanceOf                                     | 1747            | 1747  | 1747   | 1747   | 4       |
+| setReward                                            | 28841           | 77184 | 86614  | 128859 | 7       |
 | setTrustedCodehash                                   | 52889           | 52889 | 52889  | 52889  | 59      |
 | totalMP                                              | 800             | 800   | 800    | 800    | 81      |
 | totalMaxMP                                           | 777             | 777   | 777    | 777    | 81      |
 | totalRewardsAccrued                                  | 778             | 778   | 778    | 778    | 3       |
 | totalRewardsSupply                                   | 1430            | 2541  | 2194   | 11670  | 30      |
 | totalStaked                                          | 823             | 823   | 823    | 823    | 82      |
-| updateAccountMP                                      | 41823           | 44901 | 44325  | 61425  | 21      |
-| updateGlobalState                                    | 37054           | 54082 | 51303  | 136283 | 21      |
+| updateAccountMP                                      | 41755           | 44833 | 44257  | 61357  | 21      |
+| updateGlobalState                                    | 37054           | 54011 | 51215  | 136250 | 21      |
 | upgradeToAndCall                                     | 29868           | 36025 | 37562  | 37572  | 5       |
 
 
@@ -99,14 +97,14 @@
 | Function Name                          | min             | avg    | median | max    | # calls |
 | STAKING_TOKEN                          | 216             | 216    | 216    | 216    | 1       |
 | emergencyExit                          | 36353           | 48857  | 48091  | 65191  | 7       |
-| leave                                  | 33507           | 131513 | 60783  | 370978 | 4       |
-| lock                                   | 33245           | 60675  | 50779  | 107896 | 4       |
+| leave                                  | 33507           | 131448 | 60653  | 370978 | 4       |
+| lock                                   | 33245           | 60638  | 50779  | 107750 | 4       |
 | owner                                  | 2339            | 2339   | 2339   | 2339   | 233     |
 | register                               | 87015           | 103894 | 104115 | 104115 | 233     |
-| stake                                  | 33411           | 241524 | 252403 | 272930 | 67      |
+| stake                                  | 33411           | 241491 | 252403 | 272784 | 67      |
 | stakeManager                           | 368             | 368    | 368    | 368    | 233     |
 | trustStakeManager                      | 28953           | 28953  | 28953  | 28953  | 1       |
-| unstake                                | 33282           | 96931  | 102420 | 110233 | 14      |
+| unstake                                | 33282           | 96630  | 102062 | 109909 | 14      |
 | withdraw                               | 42289           | 42289  | 42289  | 42289  | 1       |
 
 

diff --git a/.gas-snapshot b/.gas-snapshot
@@ -3,16 +3,16 @@ EmergencyExitTest:test_CannotLeaveBeforeEmergencyMode() (gas: 297695)
 EmergencyExitTest:test_EmergencyExitBasic() (gas: 384389)
 EmergencyExitTest:test_EmergencyExitMultipleUsers() (gas: 659100)
 EmergencyExitTest:test_EmergencyExitToAlternateAddress() (gas: 392308)
-EmergencyExitTest:test_EmergencyExitWithLock() (gas: 391886)
+EmergencyExitTest:test_EmergencyExitWithLock() (gas: 391740)
 EmergencyExitTest:test_EmergencyExitWithRewards() (gas: 377272)
 EmergencyExitTest:test_OnlyOwnerCanEnableEmergencyMode() (gas: 39408)
-IntegrationTest:testStakeFoo() (gas: 1178931)
-LeaveTest:test_LeaveShouldProperlyUpdateAccounting() (gas: 2930114)
+IntegrationTest:testStakeFoo() (gas: 1178059)
+LeaveTest:test_LeaveShouldProperlyUpdateAccounting() (gas: 2948047)
 LeaveTest:test_RevertWhenStakeManagerIsTrusted() (gas: 294826)
-LeaveTest:test_TrustNewStakeManager() (gas: 3007555)
+LeaveTest:test_TrustNewStakeManager() (gas: 3025684)
 LockTest:test_LockFailsWithInvalidPeriod() (gas: 309889)
 LockTest:test_LockFailsWithNoStake() (gas: 63598)
-LockTest:test_LockWithoutPriorLock() (gas: 390882)
+LockTest:test_LockWithoutPriorLock() (gas: 388275)
 MaliciousUpgradeTest:test_UpgradeStackOverflowStakeManager() (gas: 1745333)
 MultipleVaultsStakeTest:test_StakeMultipleVaults() (gas: 716820)
 NFTMetadataGeneratorSVGTest:testGenerateMetadata() (gas: 85934)
@@ -21,47 +21,47 @@ NFTMetadataGeneratorSVGTest:testSetImageStringsRevert() (gas: 35804)
 NFTMetadataGeneratorURLTest:testGenerateMetadata() (gas: 102512)
 NFTMetadataGeneratorURLTest:testSetBaseURL() (gas: 49555)
 NFTMetadataGeneratorURLTest:testSetBaseURLRevert() (gas: 35979)
-RewardsStreamerMP_RewardsTest:testRewardsBalanceOf() (gas: 670855)
+RewardsStreamerMP_RewardsTest:testRewardsBalanceOf() (gas: 670745)
 RewardsStreamerMP_RewardsTest:testSetRewards() (gas: 160214)
 RewardsStreamerMP_RewardsTest:testSetRewards_RevertsBadAmount() (gas: 39323)
 RewardsStreamerMP_RewardsTest:testSetRewards_RevertsBadDuration() (gas: 39346)
 RewardsStreamerMP_RewardsTest:testSetRewards_RevertsNotAuthorized() (gas: 39359)
-RewardsStreamerMP_RewardsTest:testTotalRewardsSupply() (gas: 611786)
+RewardsStreamerMP_RewardsTest:testTotalRewardsSupply() (gas: 611753)
 RewardsStreamerTest:testStake() (gas: 869181)
 StakeTest:test_StakeMultipleAccounts() (gas: 494398)
 StakeTest:test_StakeMultipleAccountsAndRewards() (gas: 500336)
-StakeTest:test_StakeMultipleAccountsMPIncreasesMaxMPDoesNotChange() (gas: 830907)
-StakeTest:test_StakeMultipleAccountsWithMinLockUp() (gas: 517393)
-StakeTest:test_StakeMultipleAccountsWithRandomLockUp() (gas: 539321)
+StakeTest:test_StakeMultipleAccountsMPIncreasesMaxMPDoesNotChange() (gas: 830459)
+StakeTest:test_StakeMultipleAccountsWithMinLockUp() (gas: 512324)
+StakeTest:test_StakeMultipleAccountsWithRandomLockUp() (gas: 534106)
 StakeTest:test_StakeOneAccount() (gas: 276911)
 StakeTest:test_StakeOneAccountAndRewards() (gas: 282880)
-StakeTest:test_StakeOneAccountMPIncreasesMaxMPDoesNotChange() (gas: 499810)
-StakeTest:test_StakeOneAccountReachingMPLimit() (gas: 496147)
-StakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 301766)
-StakeTest:test_StakeOneAccountWithMinLockUp() (gas: 301755)
-StakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 301822)
+StakeTest:test_StakeOneAccountMPIncreasesMaxMPDoesNotChange() (gas: 499498)
+StakeTest:test_StakeOneAccountReachingMPLimit() (gas: 494611)
+StakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 299159)
+StakeTest:test_StakeOneAccountWithMinLockUp() (gas: 299148)
+StakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 299215)
 StakingTokenTest:testStakeToken() (gas: 10422)
 UnstakeTest:test_StakeMultipleAccounts() (gas: 494420)
 UnstakeTest:test_StakeMultipleAccountsAndRewards() (gas: 500336)
-UnstakeTest:test_StakeMultipleAccountsMPIncreasesMaxMPDoesNotChange() (gas: 830884)
-UnstakeTest:test_StakeMultipleAccountsWithMinLockUp() (gas: 517415)
-UnstakeTest:test_StakeMultipleAccountsWithRandomLockUp() (gas: 539343)
+UnstakeTest:test_StakeMultipleAccountsMPIncreasesMaxMPDoesNotChange() (gas: 830436)
+UnstakeTest:test_StakeMultipleAccountsWithMinLockUp() (gas: 512346)
+UnstakeTest:test_StakeMultipleAccountsWithRandomLockUp() (gas: 534128)
 UnstakeTest:test_StakeOneAccount() (gas: 276934)
 UnstakeTest:test_StakeOneAccountAndRewards() (gas: 282902)
-UnstakeTest:test_StakeOneAccountMPIncreasesMaxMPDoesNotChange() (gas: 499832)
-UnstakeTest:test_StakeOneAccountReachingMPLimit() (gas: 496127)
-UnstakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 301766)
-UnstakeTest:test_StakeOneAccountWithMinLockUp() (gas: 301755)
-UnstakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 301866)
-UnstakeTest:test_UnstakeBonusMPAndAccuredMP() (gas: 542840)
-UnstakeTest:test_UnstakeMultipleAccounts() (gas: 693159)
-UnstakeTest:test_UnstakeMultipleAccountsAndRewards() (gas: 786966)
-UnstakeTest:test_UnstakeOneAccount() (gas: 473331)
-UnstakeTest:test_UnstakeOneAccountAndAccruedMP() (gas: 495001)
-UnstakeTest:test_UnstakeOneAccountAndRewards() (gas: 404402)
-UnstakeTest:test_UnstakeOneAccountWithLockUpAndAccruedMP() (gas: 531506)
-UpgradeTest:test_RevertWhenNotOwner() (gas: 2571219)
-UpgradeTest:test_UpgradeStakeManager() (gas: 2844658)
+UnstakeTest:test_StakeOneAccountMPIncreasesMaxMPDoesNotChange() (gas: 499520)
+UnstakeTest:test_StakeOneAccountReachingMPLimit() (gas: 494591)
+UnstakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 299159)
+UnstakeTest:test_StakeOneAccountWithMinLockUp() (gas: 299148)
+UnstakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 299259)
+UnstakeTest:test_UnstakeBonusMPAndAccuredMP() (gas: 538514)
+UnstakeTest:test_UnstakeMultipleAccounts() (gas: 692511)
+UnstakeTest:test_UnstakeMultipleAccountsAndRewards() (gas: 786124)
+UnstakeTest:test_UnstakeOneAccount() (gas: 472813)
+UnstakeTest:test_UnstakeOneAccountAndAccruedMP() (gas: 494521)
+UnstakeTest:test_UnstakeOneAccountAndRewards() (gas: 404078)
+UnstakeTest:test_UnstakeOneAccountWithLockUpAndAccruedMP() (gas: 523496)
+UpgradeTest:test_RevertWhenNotOwner() (gas: 2589348)
+UpgradeTest:test_UpgradeStakeManager() (gas: 2862787)
 VaultRegistrationTest:test_VaultRegistration() (gas: 62211)
 WithdrawTest:test_CannotWithdrawStakedFunds() (gas: 310550)
 XPNFTTokenTest:testApproveNotAllowed() (gas: 10500)

diff --git a/certora/specs/RewardsStreamerMP.spec b/certora/specs/RewardsStreamerMP.spec
@@ -13,6 +13,12 @@ methods {
     function updateAccountMP(address accountAddress) external;
     function emergencyModeEnabled() external returns (bool) envfree;
     function leave() external;
+    function Math.mulDiv(uint256 a, uint256 b, uint256 c) internal returns uint256 => mulDivSummary(a,b,c);
+}
+
+function mulDivSummary(uint256 a, uint256 b, uint256 c) returns uint256 {
+  require c != 0;
+  return require_uint256(a*b/c);
 }
 
 ghost mathint sumOfBalances {

diff --git a/src/RewardsStreamerMP.sol b/src/RewardsStreamerMP.sol
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.26;
 
+import { Math } from "@openzeppelin/contracts/utils/math/Math.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol";
 import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
@@ -33,7 +34,7 @@ contract RewardsStreamerMP is
     IERC20 public STAKING_TOKEN;
 
     uint256 public constant SCALE_FACTOR = 1e18;
-    uint256 public constant MP_RATE_PER_YEAR = 1e18;
+    uint256 public constant MP_RATE_PER_YEAR = 1;
 
     uint256 public constant MIN_LOCKUP_PERIOD = 90 days;
     uint256 public constant MAX_LOCKUP_PERIOD = 4 * 365 days;
@@ -289,8 +290,9 @@ contract RewardsStreamerMP is
 
         uint256 previousStakedBalance = account.stakedBalance;
 
-        uint256 mpToReduce = (account.accountMP * amount * SCALE_FACTOR) / (previousStakedBalance * SCALE_FACTOR);
-        uint256 maxMPToReduce = (account.maxMP * amount * SCALE_FACTOR) / (previousStakedBalance * SCALE_FACTOR);
+        // solhint-disable-next-line
+        uint256 mpToReduce = Math.mulDiv(account.accountMP, amount, previousStakedBalance);
+        uint256 maxMPToReduce = Math.mulDiv(account.maxMP, amount, previousStakedBalance);
 
         account.stakedBalance -= amount;
         account.accountMP -= mpToReduce;
@@ -340,7 +342,7 @@ contract RewardsStreamerMP is
             return;
         }
 
-        uint256 accruedMP = (timeDiff * totalStaked * MP_RATE_PER_YEAR) / (365 days * SCALE_FACTOR);
+        uint256 accruedMP = (timeDiff * totalStaked * MP_RATE_PER_YEAR) / 365 days;
         if (totalMP + accruedMP > totalMaxMP) {
             accruedMP = totalMaxMP - totalMP;
         }
@@ -423,13 +425,15 @@ contract RewardsStreamerMP is
         }
 
         totalRewardsAccrued += newRewards;
-        rewardIndex += (newRewards * SCALE_FACTOR) / totalWeight;
-        lastRewardTime = block.timestamp < rewardEndTime ? block.timestamp : rewardEndTime;
+        uint256 indexIncrease = Math.mulDiv(newRewards, SCALE_FACTOR, totalWeight);
+        if (indexIncrease > 0) {
+            rewardIndex += indexIncrease;
+            lastRewardTime = block.timestamp < rewardEndTime ? block.timestamp : rewardEndTime;
+        }
     }
 
     function _calculateBonusMP(uint256 amount, uint256 lockPeriod) internal pure returns (uint256) {
-        uint256 lockMultiplier = (lockPeriod * MAX_MULTIPLIER * SCALE_FACTOR) / MAX_LOCKUP_PERIOD;
-        return amount * lockMultiplier / SCALE_FACTOR;
+        return Math.mulDiv(amount * lockPeriod, MAX_MULTIPLIER, MAX_LOCKUP_PERIOD);
     }
 
     function _getAccountAccruedMP(Account storage account) internal view returns (uint256) {
@@ -442,7 +446,7 @@ contract RewardsStreamerMP is
             return 0;
         }
 
-        uint256 accruedMP = (timeDiff * account.stakedBalance * MP_RATE_PER_YEAR) / (365 days * SCALE_FACTOR);
+        uint256 accruedMP = Math.mulDiv(timeDiff * account.stakedBalance, MP_RATE_PER_YEAR, 365 days);
 
         if (account.accountMP + accruedMP > account.maxMP) {
             accruedMP = account.maxMP - account.accountMP;
@@ -468,7 +472,7 @@ contract RewardsStreamerMP is
         uint256 accountWeight = account.stakedBalance + account.accountMP;
         uint256 deltaRewardIndex = rewardIndex - account.accountRewardIndex;
 
-        return (accountWeight * deltaRewardIndex) / SCALE_FACTOR;
+        return Math.mulDiv(accountWeight, deltaRewardIndex, SCALE_FACTOR);
     }
 
     function enableEmergencyMode() external onlyOwner {