fix: use solidity division instead of fixed point scaling (OZ L-04)

Signed-off-by: Tomás Migone <[email protected]>

Author: tmigone

Diff for: packages/horizon/contracts/staking/HorizonStaking.sol

@@ -32,9 +32,6 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
     using PPMMath for uint256;
     using LinkedList for LinkedList.List;
 
-    /// @dev Fixed point precision
-    uint256 private constant FIXED_POINT_PRECISION = 1e18;
-
     /// @dev Maximum number of simultaneous stake thaw requests (per provision) or undelegations (per delegation)
     uint256 private constant MAX_THAW_REQUESTS = 1_000;
 
@@ -441,12 +438,8 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
             // Burn remainder
             _graphToken().burnTokens(providerTokensSlashed - tokensVerifier);
 
-            // Provision accounting
-            uint256 provisionFractionSlashed = (providerTokensSlashed * FIXED_POINT_PRECISION + prov.tokens - 1) /
-                prov.tokens;
-            prov.tokensThawing =
-                (prov.tokensThawing * (FIXED_POINT_PRECISION - provisionFractionSlashed)) /
-                (FIXED_POINT_PRECISION);
+            // Provision accounting - round down, 1 wei max precision loss
+            prov.tokensThawing = (prov.tokensThawing * (prov.tokens - providerTokensSlashed)) / prov.tokens;
             prov.tokens = prov.tokens - providerTokensSlashed;
 
             // If the slashing leaves the thawing shares with no thawing tokens, cancel pending thawings by:
@@ -477,13 +470,9 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
                 // Burn tokens
                 _graphToken().burnTokens(tokensToSlash);
 
-                // Delegation pool accounting
-                uint256 delegationFractionSlashed = (tokensToSlash * FIXED_POINT_PRECISION + pool.tokens - 1) /
-                    pool.tokens;
+                // Delegation pool accounting - round down, 1 wei max precision loss
+                pool.tokensThawing = (pool.tokensThawing * (pool.tokens - tokensToSlash)) / pool.tokens;
                 pool.tokens = pool.tokens - tokensToSlash;
-                pool.tokensThawing =
-                    (pool.tokensThawing * (FIXED_POINT_PRECISION - delegationFractionSlashed)) /
-                    FIXED_POINT_PRECISION;
 
                 // If the slashing leaves the thawing shares with no thawing tokens, cancel pending thawings by:
                 // - deleting all thawing shares

Diff for: packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol

@@ -1525,9 +1525,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
             uint256 tokensSlashed = calcValues.providerTokensSlashed +
                 (isDelegationSlashingEnabled ? calcValues.delegationTokensSlashed : 0);
             uint256 provisionThawingTokens = (before.provision.tokensThawing *
-                (1e18 -
-                    ((calcValues.providerTokensSlashed * 1e18 + before.provision.tokens - 1) /
-                        before.provision.tokens))) / (1e18);
+                (before.provision.tokens - calcValues.providerTokensSlashed)) / before.provision.tokens;
 
             // assert
             assertEq(afterProvision.tokens + calcValues.providerTokensSlashed, before.provision.tokens);
@@ -1548,9 +1546,8 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
             );
             if (isDelegationSlashingEnabled) {
                 uint256 poolThawingTokens = (before.pool.tokensThawing *
-                    (1e18 -
-                        ((calcValues.delegationTokensSlashed * 1e18 + before.pool.tokens - 1) / before.pool.tokens))) /
-                    (1e18);
+                    (before.pool.tokens - calcValues.delegationTokensSlashed)) /
+                    before.pool.tokens;
                 assertEq(afterPool.tokens + calcValues.delegationTokensSlashed, before.pool.tokens);
                 assertEq(afterPool.shares, before.pool.shares);
                 assertEq(afterPool.tokensThawing, poolThawingTokens);

Diff for: packages/horizon/test/staking/provision/parameters.t.sol

@@ -78,11 +78,17 @@ contract HorizonStakingProvisionParametersTest is HorizonStakingTest {
         vm.assume(amount > 0);
         vm.assume(amount <= MAX_STAKING_TOKENS);
         vm.assume(maxVerifierCut <= MAX_PPM);
-        
+
         // create provision with initial parameters
         uint32 initialMaxVerifierCut = 1000;
-        uint64 initialThawingPeriod = 14 days;  // Max thawing period is 28 days
-        _createProvision(users.indexer, subgraphDataServiceAddress, amount, initialMaxVerifierCut, initialThawingPeriod);
+        uint64 initialThawingPeriod = 14 days; // Max thawing period is 28 days
+        _createProvision(
+            users.indexer,
+            subgraphDataServiceAddress,
+            amount,
+            initialMaxVerifierCut,
+            initialThawingPeriod
+        );
 
         // change the max thawing period allowed so that the initial thawing period is not valid anymore
         uint64 newMaxThawingPeriod = 7 days;

Diff for: packages/horizon/test/staking/slash/slash.t.sol

@@ -141,45 +141,57 @@ contract HorizonStakingSlashTest is HorizonStakingTest {
         _slash(users.indexer, subgraphDataServiceAddress, tokens + delegationTokens, 0);
     }
 
-    function testSlash_RoundDown_TokensThawing_Provision() public useIndexer {
-        uint256 tokens = 1 ether + 1;
+    function testSlash_RoundDown_TokensThawing_Provision(
+        uint256 tokens,
+        uint256 slashTokens,
+        uint256 tokensToThaw
+    ) public useIndexer {
+        vm.assume(slashTokens <= tokens);
+        vm.assume(tokensToThaw <= tokens);
+        vm.assume(tokensToThaw > 0);
+
         _useProvision(subgraphDataServiceAddress, tokens, MAX_PPM, MAX_THAWING_PERIOD);
+        _thaw(users.indexer, subgraphDataServiceAddress, tokensToThaw);
 
-        _thaw(users.indexer, subgraphDataServiceAddress, tokens);
+        Provision memory beforeProvision = staking.getProvision(users.indexer, subgraphDataServiceAddress);
 
         resetPrank(subgraphDataServiceAddress);
-        _slash(users.indexer, subgraphDataServiceAddress, 1, 0);
+        _slash(users.indexer, subgraphDataServiceAddress, slashTokens, 0);
 
-        resetPrank(users.indexer);
-        Provision memory provision = staking.getProvision(users.indexer, subgraphDataServiceAddress);
-        assertEq(provision.tokens, tokens - 1);
-        // Tokens thawing should be rounded down
-        assertEq(provision.tokensThawing, tokens - 2);
+        Provision memory afterProvision = staking.getProvision(users.indexer, subgraphDataServiceAddress);
+        assertEq(afterProvision.tokens, beforeProvision.tokens - slashTokens);
+        assertEq(
+            afterProvision.tokensThawing,
+            (beforeProvision.tokensThawing * (beforeProvision.tokens - slashTokens)) / beforeProvision.tokens
+        );
     }
 
     function testSlash_RoundDown_TokensThawing_Delegation(
-        uint256 tokens
+        uint256 tokens,
+        uint256 delegationTokensToSlash,
+        uint256 delegationTokensToUndelegate
     ) public useIndexer useProvision(tokens, MAX_PPM, 0) useDelegationSlashing {
+        uint256 delegationTokens = 10 ether;
+
+        vm.assume(delegationTokensToSlash <= delegationTokens);
+        vm.assume(delegationTokensToUndelegate <= delegationTokens);
+        vm.assume(delegationTokensToUndelegate > 0);
+
         resetPrank(users.delegator);
-        uint256 delegationTokens = 1 ether + 1;
         _delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);
+        _undelegate(users.indexer, subgraphDataServiceAddress, delegationTokensToUndelegate);
 
-        DelegationInternal memory delegation = _getStorage_Delegation(
-            users.indexer,
-            subgraphDataServiceAddress,
-            users.delegator,
-            false
-        );
-        _undelegate(users.indexer, subgraphDataServiceAddress, delegation.shares);
+        DelegationPool memory beforePool = staking.getDelegationPool(users.indexer, subgraphDataServiceAddress);
 
+        // Slash
         resetPrank(subgraphDataServiceAddress);
-        // Slash 1 token from delegation
-        _slash(users.indexer, subgraphDataServiceAddress, tokens + 1, 0);
+        _slash(users.indexer, subgraphDataServiceAddress, tokens + delegationTokensToSlash, 0);
 
-        resetPrank(users.delegator);
-        DelegationPool memory pool = staking.getDelegationPool(users.indexer, subgraphDataServiceAddress);
-        assertEq(pool.tokens, delegationTokens - 1);
-        // Tokens thawing should be rounded down
-        assertEq(pool.tokensThawing, delegationTokens - 2);
+        DelegationPool memory afterPool = staking.getDelegationPool(users.indexer, subgraphDataServiceAddress);
+        assertEq(afterPool.tokens, beforePool.tokens - delegationTokensToSlash);
+        assertEq(
+            afterPool.tokensThawing,
+            (beforePool.tokensThawing * (beforePool.tokens - delegationTokensToSlash)) / beforePool.tokens
+        );
     }
 }