From 6c358dab9e45d131d4a3e8019c418c7f8536c1dd Mon Sep 17 00:00:00 2001 From: Ricardo Guilherme Schmidt <3esmit@gmail.com> Date: Sat, 24 Feb 2024 16:39:38 -0300 Subject: [PATCH] fix(StakeManager): use OpenZeppelin Math to avoid precision loss in int divisions --- .gas-report | 53 ++++++++++++++++++++------------------ .gas-snapshot | 16 +++++++----- contracts/StakeManager.sol | 12 +++++---- 3 files changed, 44 insertions(+), 37 deletions(-) diff --git a/.gas-report b/.gas-report index 3834445..dcf8f68 100644 --- a/.gas-report +++ b/.gas-report @@ -1,26 +1,29 @@ | contracts/StakeManager.sol:StakeManager contract | | | | | | |--------------------------------------------------|-----------------|--------|--------|--------|---------| | Deployment Cost | Deployment Size | | | | | -| 2011229 | 10261 | | | | | +| 2058079 | 10495 | | | | | | Function Name | min | avg | median | max | # calls | +| EPOCH_SIZE | 285 | 285 | 285 | 285 | 9 | | MAX_LOCKUP_PERIOD | 405 | 405 | 405 | 405 | 2 | | MIN_LOCKUP_PERIOD | 264 | 264 | 264 | 264 | 3 | -| accounts | 1406 | 1406 | 1406 | 1406 | 2 | -| currentEpoch | 341 | 2118 | 2341 | 2341 | 9 | -| executeAccount | 1311 | 1311 | 1311 | 1311 | 1 | -| isVault | 517 | 2335 | 2517 | 2517 | 11 | +| accounts | 1406 | 1406 | 1406 | 1406 | 22 | +| currentEpoch | 341 | 1571 | 2341 | 2341 | 13 | +| epochEnd | 627 | 627 | 627 | 627 | 56 | +| executeAccount | 1311 | 54054 | 58730 | 104630 | 63 | +| executeEpoch | 87833 | 95166 | 87833 | 109833 | 3 | +| isVault | 517 | 2117 | 2517 | 2517 | 15 | | lock | 2614 | 2614 | 2614 | 2614 | 1 | | migrateTo | 1041 | 1713 | 1041 | 2721 | 5 | | oldManager | 240 | 240 | 240 | 240 | 8 | | owner | 2341 | 2341 | 2341 | 2341 | 8 | -| pendingReward | 2386 | 2386 | 2386 | 2386 | 8 | -| setVault | 22606 | 22606 | 22606 | 22606 | 10 | -| stake | 2638 | 128658 | 188409 | 189093 | 13 | -| stakedToken | 260 | 260 | 260 | 260 | 22 | -| totalSupply | 561 | 561 | 561 | 561 | 8 | +| pendingReward | 386 | 1243 | 386 | 2386 | 21 | +| setVault | 22606 | 22606 | 22606 | 22606 | 12 | +| stake | 2638 | 136864 | 188409 | 189128 | 17 | +| stakedToken | 260 | 260 | 260 | 260 | 26 | +| totalSupply | 561 | 561 | 561 | 561 | 17 | | totalSupplyBalance | 362 | 1592 | 2362 | 2362 | 13 | -| totalSupplyMP | 384 | 1984 | 2384 | 2384 | 10 | -| unstake | 1730 | 18924 | 8964 | 107445 | 8 | +| totalSupplyMP | 384 | 1614 | 2384 | 2384 | 13 | +| unstake | 1730 | 20008 | 8086 | 127550 | 9 | | contracts/StakeVault.sol:StakeVault contract | | | | | | @@ -30,10 +33,10 @@ | Function Name | min | avg | median | max | # calls | | acceptMigration | 1726 | 1726 | 1726 | 1726 | 2 | | leave | 1712 | 1712 | 1712 | 1712 | 1 | -| owner | 362 | 362 | 362 | 362 | 10 | -| stake | 3433 | 156693 | 219184 | 219868 | 13 | +| owner | 362 | 362 | 362 | 362 | 14 | +| stake | 3433 | 165544 | 219184 | 219903 | 17 | | stakedToken | 212 | 212 | 212 | 212 | 2 | -| unstake | 2588 | 24583 | 14285 | 111766 | 7 | +| unstake | 2588 | 28122 | 14407 | 131871 | 8 | | contracts/VaultFactory.sol:VaultFactory contract | | | | | | @@ -41,7 +44,7 @@ | Deployment Cost | Deployment Size | | | | | | 1043406 | 5305 | | | | | | Function Name | min | avg | median | max | # calls | -| createVault | 670954 | 674489 | 675454 | 675454 | 14 | +| createVault | 670954 | 674204 | 675454 | 675454 | 18 | | setStakeManager | 2518 | 5317 | 4644 | 8790 | 3 | | stakeManager | 368 | 1868 | 2368 | 2368 | 4 | @@ -51,18 +54,18 @@ | Deployment Cost | Deployment Size | | | | | | 649818 | 3562 | | | | | | Function Name | min | avg | median | max | # calls | -| approve | 24603 | 24603 | 24603 | 24603 | 11 | -| balanceOf | 561 | 1132 | 561 | 2561 | 42 | -| transfer | 3034 | 3034 | 3034 | 3034 | 5 | -| transferFrom | 27530 | 27530 | 27530 | 27530 | 12 | +| approve | 24603 | 24603 | 24603 | 24603 | 15 | +| balanceOf | 561 | 819 | 561 | 2561 | 139 | +| transfer | 3034 | 8340 | 3034 | 22934 | 15 | +| transferFrom | 27530 | 27530 | 27530 | 27530 | 16 | | script/Deploy.s.sol:Deploy contract | | | | | | |-------------------------------------|-----------------|---------|---------|---------|---------| | Deployment Cost | Deployment Size | | | | | -| 5095726 | 26758 | | | | | +| 5142589 | 26992 | | | | | | Function Name | min | avg | median | max | # calls | -| run | 4790797 | 4790797 | 4790797 | 4790797 | 30 | +| run | 4837698 | 4837698 | 4837698 | 4837698 | 32 | | script/DeploymentConfig.s.sol:DeploymentConfig contract | | | | | | @@ -70,7 +73,7 @@ | Deployment Cost | Deployment Size | | | | | | 1634091 | 8548 | | | | | | Function Name | min | avg | median | max | # calls | -| activeNetworkConfig | 455 | 455 | 455 | 455 | 60 | +| activeNetworkConfig | 455 | 455 | 455 | 455 | 64 | | test/mocks/BrokenERC20.s.sol:BrokenERC20 contract | | | | | | @@ -86,9 +89,9 @@ | test/script/DeployBroken.s.sol:DeployBroken contract | | | | | | |------------------------------------------------------|-----------------|---------|---------|---------|---------| | Deployment Cost | Deployment Size | | | | | -| 3868460 | 20556 | | | | | +| 3915327 | 20790 | | | | | | Function Name | min | avg | median | max | # calls | -| run | 3630620 | 3630620 | 3630620 | 3630620 | 1 | +| run | 3677521 | 3677521 | 3677521 | 3677521 | 1 | diff --git a/.gas-snapshot b/.gas-snapshot index 4d2a4db..73d50f9 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,7 +1,8 @@ CreateVaultTest:testDeployment() (gas: 9774) CreateVaultTest:test_createVault() (gas: 692923) ExecuteAccountTest:testDeployment() (gas: 26335) -ExecuteAccountTest:test_RevertWhen_InvalidLimitEpoch() (gas: 1051620) +ExecuteAccountTest:test_ExecuteAccountMintMP() (gas: 3633587) +ExecuteAccountTest:test_RevertWhen_InvalidLimitEpoch() (gas: 1051631) LeaveTest:testDeployment() (gas: 26335) LeaveTest:test_RevertWhen_NoPendingMigration() (gas: 1052239) LeaveTest:test_RevertWhen_SenderIsNotVault() (gas: 10794) @@ -19,13 +20,14 @@ StakeTest:testDeployment() (gas: 26335) StakeTest:test_RevertWhen_InvalidLockupPeriod() (gas: 883366) StakeTest:test_RevertWhen_SenderIsNotVault() (gas: 10650) StakeTest:test_RevertWhen_StakeTokenTransferFails() (gas: 175040) -StakeTest:test_StakeWithoutLockUpTimeMintsMultiplierPoints() (gas: 948606) +StakeTest:test_StakeWithoutLockUpTimeMintsMultiplierPoints() (gas: 948728) StakedTokenTest:testStakeToken() (gas: 7616) UnstakeTest:testDeployment() (gas: 26357) -UnstakeTest:test_RevertWhen_FundsLocked() (gas: 1051778) -UnstakeTest:test_RevertWhen_SenderIsNotVault() (gas: 10631) -UnstakeTest:test_UnstakeShouldReturnFunds() (gas: 946735) +UnstakeTest:test_RevertWhen_FundsLocked() (gas: 1051813) +UnstakeTest:test_RevertWhen_SenderIsNotVault() (gas: 10653) +UnstakeTest:test_UnstakeShouldBurnMultiplierPoints() (gas: 3573382) +UnstakeTest:test_UnstakeShouldReturnFunds() (gas: 946825) UserFlowsTest:testDeployment() (gas: 26335) -UserFlowsTest:test_StakeWithLockUpTimeLocksStake() (gas: 1026340) -UserFlowsTest:test_StakedSupplyShouldIncreaseAndDecreaseAgain() (gas: 1825381) +UserFlowsTest:test_StakeWithLockUpTimeLocksStake() (gas: 1046480) +UserFlowsTest:test_StakedSupplyShouldIncreaseAndDecreaseAgain() (gas: 1825625) VaultFactoryTest:testDeployment() (gas: 9774) \ No newline at end of file diff --git a/contracts/StakeManager.sol b/contracts/StakeManager.sol index 3b61f98..77c3d09 100644 --- a/contracts/StakeManager.sol +++ b/contracts/StakeManager.sol @@ -4,6 +4,8 @@ pragma solidity ^0.8.18; import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; +import { Math } from "@openzeppelin/contracts/utils/math/Math.sol"; + import { StakeVault } from "./StakeVault.sol"; contract StakeManager is Ownable { @@ -162,8 +164,8 @@ contract StakeManager is Ownable { } _processAccount(account, currentEpoch); - uint256 reducedMP = ((_amount * account.currentMP) / account.balance); //TODO: fix precision loss - uint256 reducedInitialMP = ((_amount * account.initialMP) / account.balance); //TODO: fix precision loss + uint256 reducedMP = Math.mulDiv(_amount, account.currentMP, account.balance); + uint256 reducedInitialMP = Math.mulDiv(_amount, account.initialMP, account.balance); //update storage account.balance -= _amount; @@ -324,8 +326,8 @@ contract StakeManager is Ownable { //mint multiplier points to that epoch _mintMP(account, iEpoch.startTime + EPOCH_SIZE, iEpoch); uint256 userSupply = account.balance + account.currentMP; - uint256 userShare = userSupply / iEpoch.totalSupply; //TODO: fix precision loss; - uint256 userEpochReward = userShare * iEpoch.epochReward; + uint256 userEpochReward = Math.mulDiv(userSupply, iEpoch.epochReward, iEpoch.totalSupply); + userReward += userEpochReward; iEpoch.epochReward -= userEpochReward; iEpoch.totalSupply -= userSupply; @@ -432,7 +434,7 @@ contract StakeManager is Ownable { * @return _increasedMP increased multiplier points */ function _getIncreasedMP(uint256 _balance, uint256 _deltaTime) private pure returns (uint256 _increasedMP) { - return _balance * ((_deltaTime / YEAR) * MP_APY); //TODO: fix precision loss + return Math.mulDiv(_balance, _deltaTime, YEAR) * MP_APY; } /**