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

precision loss Closes #85
vacp2p · Dec 4, 2024 · 9d7df96 · 9d7df96
1 parent 32a076d
commit 9d7df96
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 61 deletions.
diff --git a/.gas-report b/.gas-report
@@ -15,7 +15,7 @@
 | src/RewardsStreamerMP.sol:RewardsStreamerMP contract |                 |        |        |        |         |
 |------------------------------------------------------|-----------------|--------|--------|--------|---------|
 | Deployment Cost                                      | Deployment Size |        |        |        |         |
-| 2491772                                              | 11482           |        |        |        |         |
+| 2529128                                              | 11656           |        |        |        |         |
 | Function Name                                        | min             | avg    | median | max    | # calls |
 | MAX_LOCKUP_PERIOD                                    | 294             | 294    | 294    | 294    | 23      |
 | MAX_MULTIPLIER                                       | 251             | 251    | 251    | 251    | 30      |
@@ -34,24 +34,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           | 34212  | 16370  | 74225  | 3       |
+| leave                                                | 56653           | 56653  | 56653  | 56653  | 1       |
+| lock                                                 | 12041           | 34250  | 16370  | 74340  | 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       |
+| setReward                                            | 2561            | 50878  | 60256  | 102573 | 7       |
 | setTrustedCodehash                                   | 26243           | 26243  | 26243  | 26243  | 59      |
-| stake                                                | 131211          | 170364 | 178028 | 198507 | 66      |
+| stake                                                | 131211          | 170390 | 178028 | 198622 | 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                                              | 60746           | 61297  | 60746  | 64329  | 13      |
+| updateAccountMP                                      | 15484           | 18562  | 17986  | 35086  | 21      |
+| updateGlobalState                                    | 11066           | 28096  | 25315  | 110318 | 21      |
 | upgradeToAndCall                                     | 3225            | 9387   | 10926  | 10936  | 5       |
 
 
@@ -80,15 +80,15 @@
 | rewardEndTime                                        | 800             | 1800  | 1800   | 2800   | 2       |
 | rewardStartTime                                      | 779             | 4029  | 4029   | 7279   | 2       |
 | rewardsBalanceOf                                     | 1724            | 1724  | 1724   | 1724   | 4       |
-| setReward                                            | 28841           | 77189 | 86614  | 128859 | 7       |
+| setReward                                            | 28841           | 77192 | 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                                      | 41843           | 44921 | 44345  | 61445  | 21      |
+| updateGlobalState                                    | 37054           | 54084 | 51303  | 136306 | 21      |
 | upgradeToAndCall                                     | 29868           | 36025 | 37562  | 37572  | 5       |
 
 
@@ -99,14 +99,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           | 60706  | 50779  | 108022 | 4       |
+| leave                                  | 33507           | 131521 | 60799  | 370978 | 4       |
+| lock                                   | 33245           | 60735  | 50779  | 108137 | 4       |
 | owner                                  | 2339            | 2339   | 2339   | 2339   | 233     |
 | register                               | 87015           | 103894 | 104115 | 104115 | 233     |
-| stake                                  | 33411           | 241651 | 252532 | 273059 | 67      |
+| stake                                  | 33411           | 241677 | 252532 | 273174 | 67      |
 | stakeManager                           | 368             | 368    | 368    | 368    | 233     |
 | trustStakeManager                      | 28953           | 28953  | 28953  | 28953  | 1       |
-| unstake                                | 33282           | 96931  | 102420 | 110233 | 14      |
+| unstake                                | 33282           | 96970  | 102470 | 110273 | 14      |
 | withdraw                               | 42289           | 42289  | 42289  | 42289  | 1       |
 
 

diff --git a/.gas-snapshot b/.gas-snapshot
@@ -3,16 +3,16 @@ EmergencyExitTest:test_CannotLeaveBeforeEmergencyMode() (gas: 297824)
 EmergencyExitTest:test_EmergencyExitBasic() (gas: 384518)
 EmergencyExitTest:test_EmergencyExitMultipleUsers() (gas: 659358)
 EmergencyExitTest:test_EmergencyExitToAlternateAddress() (gas: 392437)
-EmergencyExitTest:test_EmergencyExitWithLock() (gas: 392015)
+EmergencyExitTest:test_EmergencyExitWithLock() (gas: 392130)
 EmergencyExitTest:test_EmergencyExitWithRewards() (gas: 377401)
 EmergencyExitTest:test_OnlyOwnerCanEnableEmergencyMode() (gas: 39408)
-IntegrationTest:testStakeFoo() (gas: 1179318)
-LeaveTest:test_LeaveShouldProperlyUpdateAccounting() (gas: 2927052)
+IntegrationTest:testStakeFoo() (gas: 1179438)
+LeaveTest:test_LeaveShouldProperlyUpdateAccounting() (gas: 2964489)
 LeaveTest:test_RevertWhenStakeManagerIsTrusted() (gas: 294955)
-LeaveTest:test_TrustNewStakeManager() (gas: 3004493)
+LeaveTest:test_TrustNewStakeManager() (gas: 3041905)
 LockTest:test_LockFailsWithInvalidPeriod() (gas: 310018)
 LockTest:test_LockFailsWithNoStake() (gas: 63598)
-LockTest:test_LockWithoutPriorLock() (gas: 391137)
+LockTest:test_LockWithoutPriorLock() (gas: 391380)
 MaliciousUpgradeTest:test_UpgradeStackOverflowStakeManager() (gas: 1745462)
 MultipleVaultsStakeTest:test_StakeMultipleVaults() (gas: 717207)
 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: 670984)
+RewardsStreamerMP_RewardsTest:testRewardsBalanceOf() (gas: 671070)
 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: 611915)
+RewardsStreamerMP_RewardsTest:testTotalRewardsSupply() (gas: 611938)
 RewardsStreamerTest:testStake() (gas: 869181)
 StakeTest:test_StakeMultipleAccounts() (gas: 494656)
 StakeTest:test_StakeMultipleAccountsAndRewards() (gas: 500594)
-StakeTest:test_StakeMultipleAccountsMPIncreasesMaxMPDoesNotChange() (gas: 831165)
-StakeTest:test_StakeMultipleAccountsWithMinLockUp() (gas: 517651)
-StakeTest:test_StakeMultipleAccountsWithRandomLockUp() (gas: 539579)
+StakeTest:test_StakeMultipleAccountsMPIncreasesMaxMPDoesNotChange() (gas: 831245)
+StakeTest:test_StakeMultipleAccountsWithMinLockUp() (gas: 518022)
+StakeTest:test_StakeMultipleAccountsWithRandomLockUp() (gas: 540065)
 StakeTest:test_StakeOneAccount() (gas: 277040)
 StakeTest:test_StakeOneAccountAndRewards() (gas: 283009)
-StakeTest:test_StakeOneAccountMPIncreasesMaxMPDoesNotChange() (gas: 499939)
-StakeTest:test_StakeOneAccountReachingMPLimit() (gas: 496276)
-StakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 301895)
-StakeTest:test_StakeOneAccountWithMinLockUp() (gas: 301884)
-StakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 301951)
+StakeTest:test_StakeOneAccountMPIncreasesMaxMPDoesNotChange() (gas: 499979)
+StakeTest:test_StakeOneAccountReachingMPLimit() (gas: 496446)
+StakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 302138)
+StakeTest:test_StakeOneAccountWithMinLockUp() (gas: 302127)
+StakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 302194)
 StakingTokenTest:testStakeToken() (gas: 10422)
 UnstakeTest:test_StakeMultipleAccounts() (gas: 494678)
 UnstakeTest:test_StakeMultipleAccountsAndRewards() (gas: 500594)
-UnstakeTest:test_StakeMultipleAccountsMPIncreasesMaxMPDoesNotChange() (gas: 831142)
-UnstakeTest:test_StakeMultipleAccountsWithMinLockUp() (gas: 517673)
-UnstakeTest:test_StakeMultipleAccountsWithRandomLockUp() (gas: 539601)
+UnstakeTest:test_StakeMultipleAccountsMPIncreasesMaxMPDoesNotChange() (gas: 831222)
+UnstakeTest:test_StakeMultipleAccountsWithMinLockUp() (gas: 518044)
+UnstakeTest:test_StakeMultipleAccountsWithRandomLockUp() (gas: 540087)
 UnstakeTest:test_StakeOneAccount() (gas: 277063)
 UnstakeTest:test_StakeOneAccountAndRewards() (gas: 283031)
-UnstakeTest:test_StakeOneAccountMPIncreasesMaxMPDoesNotChange() (gas: 499961)
-UnstakeTest:test_StakeOneAccountReachingMPLimit() (gas: 496256)
-UnstakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 301895)
-UnstakeTest:test_StakeOneAccountWithMinLockUp() (gas: 301884)
-UnstakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 301995)
-UnstakeTest:test_UnstakeBonusMPAndAccuredMP() (gas: 542969)
-UnstakeTest:test_UnstakeMultipleAccounts() (gas: 693417)
-UnstakeTest:test_UnstakeMultipleAccountsAndRewards() (gas: 787224)
-UnstakeTest:test_UnstakeOneAccount() (gas: 473460)
-UnstakeTest:test_UnstakeOneAccountAndAccruedMP() (gas: 495130)
-UnstakeTest:test_UnstakeOneAccountAndRewards() (gas: 404531)
-UnstakeTest:test_UnstakeOneAccountWithLockUpAndAccruedMP() (gas: 531635)
-UpgradeTest:test_RevertWhenNotOwner() (gas: 2568028)
-UpgradeTest:test_UpgradeStakeManager() (gas: 2841596)
+UnstakeTest:test_StakeOneAccountMPIncreasesMaxMPDoesNotChange() (gas: 500001)
+UnstakeTest:test_StakeOneAccountReachingMPLimit() (gas: 496426)
+UnstakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 302138)
+UnstakeTest:test_StakeOneAccountWithMinLockUp() (gas: 302127)
+UnstakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 302238)
+UnstakeTest:test_UnstakeBonusMPAndAccuredMP() (gas: 543283)
+UnstakeTest:test_UnstakeMultipleAccounts() (gas: 693497)
+UnstakeTest:test_UnstakeMultipleAccountsAndRewards() (gas: 787328)
+UnstakeTest:test_UnstakeOneAccount() (gas: 473524)
+UnstakeTest:test_UnstakeOneAccountAndAccruedMP() (gas: 495190)
+UnstakeTest:test_UnstakeOneAccountAndRewards() (gas: 404571)
+UnstakeTest:test_UnstakeOneAccountWithLockUpAndAccruedMP() (gas: 532194)
+UpgradeTest:test_RevertWhenNotOwner() (gas: 2605440)
+UpgradeTest:test_UpgradeStakeManager() (gas: 2879009)
 VaultRegistrationTest:test_VaultRegistration() (gas: 62211)
 WithdrawTest:test_CannotWithdrawStakedFunds() (gas: 310679)
 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
@@ -432,9 +432,7 @@ contract RewardsStreamerMP is
 
     function _calculateBonusMP(uint256 amount, uint256 lockPeriod) internal pure returns (uint256) {
         return Math.mulDiv(
-            Math.mulDiv(amount * lockPeriod, MAX_MULTIPLIER * SCALE_FACTOR, MAX_LOCKUP_PERIOD),
-            1,
-            SCALE_FACTOR
+            Math.mulDiv(amount * lockPeriod, MAX_MULTIPLIER * SCALE_FACTOR, MAX_LOCKUP_PERIOD), 1, SCALE_FACTOR
         );
     }
 
@@ -448,11 +446,7 @@ contract RewardsStreamerMP is
             return 0;
         }
 
-        uint256 accruedMP = Math.mulDiv(
-            timeDiff * account.stakedBalance,
-            MP_RATE_PER_YEAR,
-            365 days * SCALE_FACTOR
-        );
+        uint256 accruedMP = Math.mulDiv(timeDiff * account.stakedBalance, MP_RATE_PER_YEAR, 365 days * SCALE_FACTOR);
 
         if (account.accountMP + accruedMP > account.maxMP) {
             accruedMP = account.maxMP - account.accountMP;

diff --git a/test/RewardsStreamerMP.t.sol b/test/RewardsStreamerMP.t.sol
@@ -144,18 +144,16 @@ contract RewardsStreamerMPTest is Test {
 
     function _calculateBonusMP(uint256 amount, uint256 lockupTime) public view returns (uint256) {
         return Math.mulDiv(
-            Math.mulDiv(amount * lockupTime, streamer.MAX_MULTIPLIER() * streamer.SCALE_FACTOR(), streamer.MAX_LOCKUP_PERIOD()),
+            Math.mulDiv(
+                amount * lockupTime, streamer.MAX_MULTIPLIER() * streamer.SCALE_FACTOR(), streamer.MAX_LOCKUP_PERIOD()
+            ),
             1,
             streamer.SCALE_FACTOR()
         );
     }
 
     function _calculeAccuredMP(uint256 totalStaked, uint256 timeDiff) public view returns (uint256) {
-        return Math.mulDiv(
-            timeDiff * totalStaked,
-            streamer.MP_RATE_PER_YEAR(),
-            365 days * streamer.SCALE_FACTOR()
-        );
+        return Math.mulDiv(timeDiff * totalStaked, streamer.MP_RATE_PER_YEAR(), 365 days * streamer.SCALE_FACTOR());
     }
 
     function _calculateTimeToMPLimit(uint256 amount) public view returns (uint256) {