fix(StakeManager): use OpenZeppelin Math to avoid precision loss in int divisions

This commit is contained in:
Ricardo Guilherme Schmidt 2024-02-24 16:39:38 -03:00
parent 81a28da777
commit ecc6271c1f
3 changed files with 44 additions and 37 deletions

View File

@ -1,26 +1,29 @@
| contracts/StakeManager.sol:StakeManager contract | | | | | | | contracts/StakeManager.sol:StakeManager contract | | | | | |
|--------------------------------------------------|-----------------|--------|--------|--------|---------| |--------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost | Deployment Size | | | | | | Deployment Cost | Deployment Size | | | | |
| 2011229 | 10261 | | | | | | 2058079 | 10495 | | | | |
| Function Name | min | avg | median | max | # calls | | Function Name | min | avg | median | max | # calls |
| EPOCH_SIZE | 285 | 285 | 285 | 285 | 9 |
| MAX_LOCKUP_PERIOD | 405 | 405 | 405 | 405 | 2 | | MAX_LOCKUP_PERIOD | 405 | 405 | 405 | 405 | 2 |
| MIN_LOCKUP_PERIOD | 264 | 264 | 264 | 264 | 3 | | MIN_LOCKUP_PERIOD | 264 | 264 | 264 | 264 | 3 |
| accounts | 1406 | 1406 | 1406 | 1406 | 2 | | accounts | 1406 | 1406 | 1406 | 1406 | 22 |
| currentEpoch | 341 | 2118 | 2341 | 2341 | 9 | | currentEpoch | 341 | 1571 | 2341 | 2341 | 13 |
| executeAccount | 1311 | 1311 | 1311 | 1311 | 1 | | epochEnd | 627 | 627 | 627 | 627 | 56 |
| isVault | 517 | 2335 | 2517 | 2517 | 11 | | executeAccount | 1311 | 54054 | 58730 | 104630 | 63 |
| executeEpoch | 87833 | 95166 | 87833 | 109833 | 3 |
| isVault | 517 | 2117 | 2517 | 2517 | 15 |
| lock | 2614 | 2614 | 2614 | 2614 | 1 | | lock | 2614 | 2614 | 2614 | 2614 | 1 |
| migrateTo | 1041 | 1713 | 1041 | 2721 | 5 | | migrateTo | 1041 | 1713 | 1041 | 2721 | 5 |
| oldManager | 240 | 240 | 240 | 240 | 8 | | oldManager | 240 | 240 | 240 | 240 | 8 |
| owner | 2341 | 2341 | 2341 | 2341 | 8 | | owner | 2341 | 2341 | 2341 | 2341 | 8 |
| pendingReward | 2386 | 2386 | 2386 | 2386 | 8 | | pendingReward | 386 | 1243 | 386 | 2386 | 21 |
| setVault | 22606 | 22606 | 22606 | 22606 | 10 | | setVault | 22606 | 22606 | 22606 | 22606 | 12 |
| stake | 2638 | 128658 | 188409 | 189093 | 13 | | stake | 2638 | 136864 | 188409 | 189128 | 17 |
| stakedToken | 260 | 260 | 260 | 260 | 22 | | stakedToken | 260 | 260 | 260 | 260 | 26 |
| totalSupply | 561 | 561 | 561 | 561 | 8 | | totalSupply | 561 | 561 | 561 | 561 | 17 |
| totalSupplyBalance | 362 | 1592 | 2362 | 2362 | 13 | | totalSupplyBalance | 362 | 1592 | 2362 | 2362 | 13 |
| totalSupplyMP | 384 | 1984 | 2384 | 2384 | 10 | | totalSupplyMP | 384 | 1614 | 2384 | 2384 | 13 |
| unstake | 1730 | 18924 | 8964 | 107445 | 8 | | unstake | 1730 | 20008 | 8086 | 127550 | 9 |
| contracts/StakeVault.sol:StakeVault contract | | | | | | | contracts/StakeVault.sol:StakeVault contract | | | | | |
@ -30,10 +33,10 @@
| Function Name | min | avg | median | max | # calls | | Function Name | min | avg | median | max | # calls |
| acceptMigration | 1726 | 1726 | 1726 | 1726 | 2 | | acceptMigration | 1726 | 1726 | 1726 | 1726 | 2 |
| leave | 1712 | 1712 | 1712 | 1712 | 1 | | leave | 1712 | 1712 | 1712 | 1712 | 1 |
| owner | 362 | 362 | 362 | 362 | 10 | | owner | 362 | 362 | 362 | 362 | 14 |
| stake | 3433 | 156693 | 219184 | 219868 | 13 | | stake | 3433 | 165544 | 219184 | 219903 | 17 |
| stakedToken | 212 | 212 | 212 | 212 | 2 | | stakedToken | 212 | 212 | 212 | 212 | 2 |
| unstake | 2588 | 24583 | 14285 | 111766 | 7 | | unstake | 2588 | 28122 | 14407 | 131871 | 8 |
| contracts/VaultFactory.sol:VaultFactory contract | | | | | | | contracts/VaultFactory.sol:VaultFactory contract | | | | | |
@ -41,7 +44,7 @@
| Deployment Cost | Deployment Size | | | | | | Deployment Cost | Deployment Size | | | | |
| 1043406 | 5305 | | | | | | 1043406 | 5305 | | | | |
| Function Name | min | avg | median | max | # calls | | 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 | | setStakeManager | 2518 | 5317 | 4644 | 8790 | 3 |
| stakeManager | 368 | 1868 | 2368 | 2368 | 4 | | stakeManager | 368 | 1868 | 2368 | 2368 | 4 |
@ -51,18 +54,18 @@
| Deployment Cost | Deployment Size | | | | | | Deployment Cost | Deployment Size | | | | |
| 649818 | 3562 | | | | | | 649818 | 3562 | | | | |
| Function Name | min | avg | median | max | # calls | | Function Name | min | avg | median | max | # calls |
| approve | 24603 | 24603 | 24603 | 24603 | 11 | | approve | 24603 | 24603 | 24603 | 24603 | 15 |
| balanceOf | 561 | 1132 | 561 | 2561 | 42 | | balanceOf | 561 | 819 | 561 | 2561 | 139 |
| transfer | 3034 | 3034 | 3034 | 3034 | 5 | | transfer | 3034 | 8340 | 3034 | 22934 | 15 |
| transferFrom | 27530 | 27530 | 27530 | 27530 | 12 | | transferFrom | 27530 | 27530 | 27530 | 27530 | 16 |
| script/Deploy.s.sol:Deploy contract | | | | | | | script/Deploy.s.sol:Deploy contract | | | | | |
|-------------------------------------|-----------------|---------|---------|---------|---------| |-------------------------------------|-----------------|---------|---------|---------|---------|
| Deployment Cost | Deployment Size | | | | | | Deployment Cost | Deployment Size | | | | |
| 5095726 | 26758 | | | | | | 5142589 | 26992 | | | | |
| Function Name | min | avg | median | max | # calls | | 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 | | | | | | | script/DeploymentConfig.s.sol:DeploymentConfig contract | | | | | |
@ -70,7 +73,7 @@
| Deployment Cost | Deployment Size | | | | | | Deployment Cost | Deployment Size | | | | |
| 1634091 | 8548 | | | | | | 1634091 | 8548 | | | | |
| Function Name | min | avg | median | max | # calls | | 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 | | | | | | | test/mocks/BrokenERC20.s.sol:BrokenERC20 contract | | | | | |
@ -86,9 +89,9 @@
| test/script/DeployBroken.s.sol:DeployBroken contract | | | | | | | test/script/DeployBroken.s.sol:DeployBroken contract | | | | | |
|------------------------------------------------------|-----------------|---------|---------|---------|---------| |------------------------------------------------------|-----------------|---------|---------|---------|---------|
| Deployment Cost | Deployment Size | | | | | | Deployment Cost | Deployment Size | | | | |
| 3868460 | 20556 | | | | | | 3915327 | 20790 | | | | |
| Function Name | min | avg | median | max | # calls | | Function Name | min | avg | median | max | # calls |
| run | 3630620 | 3630620 | 3630620 | 3630620 | 1 | | run | 3677521 | 3677521 | 3677521 | 3677521 | 1 |

View File

@ -1,7 +1,8 @@
CreateVaultTest:testDeployment() (gas: 9774) CreateVaultTest:testDeployment() (gas: 9774)
CreateVaultTest:test_createVault() (gas: 692923) CreateVaultTest:test_createVault() (gas: 692923)
ExecuteAccountTest:testDeployment() (gas: 26335) 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:testDeployment() (gas: 26335)
LeaveTest:test_RevertWhen_NoPendingMigration() (gas: 1052239) LeaveTest:test_RevertWhen_NoPendingMigration() (gas: 1052239)
LeaveTest:test_RevertWhen_SenderIsNotVault() (gas: 10794) LeaveTest:test_RevertWhen_SenderIsNotVault() (gas: 10794)
@ -19,13 +20,14 @@ StakeTest:testDeployment() (gas: 26335)
StakeTest:test_RevertWhen_InvalidLockupPeriod() (gas: 883366) StakeTest:test_RevertWhen_InvalidLockupPeriod() (gas: 883366)
StakeTest:test_RevertWhen_SenderIsNotVault() (gas: 10650) StakeTest:test_RevertWhen_SenderIsNotVault() (gas: 10650)
StakeTest:test_RevertWhen_StakeTokenTransferFails() (gas: 175040) StakeTest:test_RevertWhen_StakeTokenTransferFails() (gas: 175040)
StakeTest:test_StakeWithoutLockUpTimeMintsMultiplierPoints() (gas: 948606) StakeTest:test_StakeWithoutLockUpTimeMintsMultiplierPoints() (gas: 948728)
StakedTokenTest:testStakeToken() (gas: 7616) StakedTokenTest:testStakeToken() (gas: 7616)
UnstakeTest:testDeployment() (gas: 26357) UnstakeTest:testDeployment() (gas: 26357)
UnstakeTest:test_RevertWhen_FundsLocked() (gas: 1051778) UnstakeTest:test_RevertWhen_FundsLocked() (gas: 1051813)
UnstakeTest:test_RevertWhen_SenderIsNotVault() (gas: 10631) UnstakeTest:test_RevertWhen_SenderIsNotVault() (gas: 10653)
UnstakeTest:test_UnstakeShouldReturnFunds() (gas: 946735) UnstakeTest:test_UnstakeShouldBurnMultiplierPoints() (gas: 3573382)
UnstakeTest:test_UnstakeShouldReturnFunds() (gas: 946825)
UserFlowsTest:testDeployment() (gas: 26335) UserFlowsTest:testDeployment() (gas: 26335)
UserFlowsTest:test_StakeWithLockUpTimeLocksStake() (gas: 1026340) UserFlowsTest:test_StakeWithLockUpTimeLocksStake() (gas: 1046480)
UserFlowsTest:test_StakedSupplyShouldIncreaseAndDecreaseAgain() (gas: 1825381) UserFlowsTest:test_StakedSupplyShouldIncreaseAndDecreaseAgain() (gas: 1825625)
VaultFactoryTest:testDeployment() (gas: 9774) VaultFactoryTest:testDeployment() (gas: 9774)

View File

@ -4,6 +4,8 @@ pragma solidity ^0.8.18;
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
import { Math } from "@openzeppelin/contracts/utils/math/Math.sol";
import { StakeVault } from "./StakeVault.sol"; import { StakeVault } from "./StakeVault.sol";
contract StakeManager is Ownable { contract StakeManager is Ownable {
@ -162,8 +164,8 @@ contract StakeManager is Ownable {
} }
_processAccount(account, currentEpoch); _processAccount(account, currentEpoch);
uint256 reducedMP = ((_amount * account.currentMP) / account.balance); //TODO: fix precision loss uint256 reducedMP = Math.mulDiv(_amount, account.currentMP, account.balance);
uint256 reducedInitialMP = ((_amount * account.initialMP) / account.balance); //TODO: fix precision loss uint256 reducedInitialMP = Math.mulDiv(_amount, account.initialMP, account.balance);
//update storage //update storage
account.balance -= _amount; account.balance -= _amount;
@ -324,8 +326,8 @@ contract StakeManager is Ownable {
//mint multiplier points to that epoch //mint multiplier points to that epoch
_mintMP(account, iEpoch.startTime + EPOCH_SIZE, iEpoch); _mintMP(account, iEpoch.startTime + EPOCH_SIZE, iEpoch);
uint256 userSupply = account.balance + account.currentMP; uint256 userSupply = account.balance + account.currentMP;
uint256 userShare = userSupply / iEpoch.totalSupply; //TODO: fix precision loss; uint256 userEpochReward = Math.mulDiv(userSupply, iEpoch.epochReward, iEpoch.totalSupply);
uint256 userEpochReward = userShare * iEpoch.epochReward;
userReward += userEpochReward; userReward += userEpochReward;
iEpoch.epochReward -= userEpochReward; iEpoch.epochReward -= userEpochReward;
iEpoch.totalSupply -= userSupply; iEpoch.totalSupply -= userSupply;
@ -432,7 +434,7 @@ contract StakeManager is Ownable {
* @return _increasedMP increased multiplier points * @return _increasedMP increased multiplier points
*/ */
function _getIncreasedMP(uint256 _balance, uint256 _deltaTime) private pure returns (uint256 _increasedMP) { 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;
} }
/** /**