Merge pull request #430 from rsksmart/feature/FLY-2215

Luisfc68 · web-flow · commit 9ecf645bea44 · 2026-02-18T20:34:24.000+01:00
Fix/FLY-2215
diff --git a/src/CollateralManagement.sol b/src/CollateralManagement.sol
@@ -113,6 +113,9 @@ contract CollateralManagementContract is
         uint256 rewardPercentage,
         IPauseRegistry pauseRegistry
     ) external initializer {
+        if (rewardPercentage > TOTAL_REWARD_PERCENTAGE) {
+            revert InvalidRewardPercentage(TOTAL_REWARD_PERCENTAGE, rewardPercentage);
+        }
         if (address(pauseRegistry).code.length == 0) revert Flyover.NoContract(address(pauseRegistry));
         __AccessControlDefaultAdminRules_init(initialDelay, defaultAdmin);
         __ReentrancyGuard_init();
@@ -142,6 +145,9 @@ contract CollateralManagementContract is
     /// @param rewardPercentage The new reward percentage
     // solhint-disable-next-line comprehensive-interface
     function setRewardPercentage(uint256 rewardPercentage) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        if (rewardPercentage > TOTAL_REWARD_PERCENTAGE) {
+            revert InvalidRewardPercentage(TOTAL_REWARD_PERCENTAGE, rewardPercentage);
+        }
         emit RewardPercentageSet(_rewardPercentage, rewardPercentage);
         _rewardPercentage = rewardPercentage;
     }
diff --git a/src/interfaces/ICollateralManagement.sol b/src/interfaces/ICollateralManagement.sol
@@ -78,6 +78,11 @@ interface ICollateralManagement is IPausable {
     /// @param from The address of the liquidity provider
     error NothingToWithdraw(address from);
 
+    /// @notice Emitted when reward percentage exceeds the allowed maximum
+    /// @param maxRewardPercentage The maximum allowed reward percentage (basis points)
+    /// @param passedRewardPercentage The provided reward percentage (basis points)
+    error InvalidRewardPercentage(uint256 maxRewardPercentage, uint256 passedRewardPercentage);
+
     /// @notice Adds peg in collateral to an account
     /// @param addr The address of the account
     /// @dev This function requires the COLLATERAL_ADDER role
diff --git a/src/test-contracts/PegOutPayer.sol b/src/test-contracts/PegOutPayer.sol
@@ -27,7 +27,7 @@ contract PegOutPayer {
     }
 
     function executePegOut(QuotesV2.PegOutQuote calldata quote, bytes calldata signature) external {
-        uint256 total = quote.value + quote.gasFee + quote.callFee + quote.productFeeAmount;
+        uint256 total = quote.value + quote.gasFee + quote.callFee;
         if (address(this).balance < total) {
             revert InsufficientBalance(address(this).balance, total);
         }
diff --git a/test/collateral/Configuration.t.sol b/test/collateral/Configuration.t.sol
@@ -3,6 +3,7 @@ pragma solidity 0.8.25;
 
 import {CollateralTestBase} from "./CollateralTestBase.sol";
 import {CollateralManagementContract} from "../../src/CollateralManagement.sol";
+import {ICollateralManagement} from "../../src/interfaces/ICollateralManagement.sol";
 import {IAccessControl} from "@openzeppelin/contracts/access/IAccessControl.sol";
 import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 import {Flyover} from "../../src/libraries/Flyover.sol";
@@ -105,7 +106,67 @@ contract ConfigurationTest is CollateralTestBase {
             TEST_DEFAULT_ADMIN_DELAY,
             TEST_MIN_COLLATERAL,
             TEST_RESIGN_DELAY_BLOCKS,
-            TEST_REWARD_PERCENTAGE
+            TEST_REWARD_PERCENTAGE,
+            pauseRegistry
+        );
+    }
+
+    function test_Initialize_RevertsWhenRewardPercentageExceedsMaximum()
+        public
+    {
+        uint256 invalidRewardPercentage = collateralManagement
+            .TOTAL_REWARD_PERCENTAGE() + 1;
+        CollateralManagementContract implementation = new CollateralManagementContract();
+        bytes memory initData = abi.encodeCall(
+            CollateralManagementContract.initialize,
+            (
+                owner,
+                TEST_DEFAULT_ADMIN_DELAY,
+                TEST_MIN_COLLATERAL,
+                TEST_RESIGN_DELAY_BLOCKS,
+                invalidRewardPercentage,
+                pauseRegistry
+            )
+        );
+
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                ICollateralManagement.InvalidRewardPercentage.selector,
+                collateralManagement.TOTAL_REWARD_PERCENTAGE(),
+                invalidRewardPercentage
+            )
+        );
+        new ERC1967Proxy(address(implementation), initData);
+    }
+
+    function test_Initialize_AllowsMaximumBoundary() public {
+        uint256 maxRewardPercentage = collateralManagement
+            .TOTAL_REWARD_PERCENTAGE();
+        CollateralManagementContract implementation = new CollateralManagementContract();
+        bytes memory initData = abi.encodeCall(
+            CollateralManagementContract.initialize,
+            (
+                owner,
+                TEST_DEFAULT_ADMIN_DELAY,
+                TEST_MIN_COLLATERAL,
+                TEST_RESIGN_DELAY_BLOCKS,
+                maxRewardPercentage,
+                pauseRegistry
+            )
+        );
+
+        ERC1967Proxy proxy = new ERC1967Proxy(
+            address(implementation),
+            initData
+        );
+        CollateralManagementContract collateralManagementWithMaxReward = CollateralManagementContract(
+                payable(address(proxy))
+            );
+
+        assertEq(
+            collateralManagementWithMaxReward.getRewardPercentage(),
+            maxRewardPercentage,
+            "Initialize should accept maximum basis points value"
         );
     }
 
@@ -145,6 +206,51 @@ contract ConfigurationTest is CollateralTestBase {
         );
     }
 
+    function test_SetRewardPercentage_RevertsWhenExceedingMaximum() public {
+        uint256 initialRewardPercentage = collateralManagement
+            .getRewardPercentage();
+        uint256 invalidRewardPercentage = collateralManagement
+            .TOTAL_REWARD_PERCENTAGE() + 1;
+
+        vm.startPrank(owner);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                ICollateralManagement.InvalidRewardPercentage.selector,
+                collateralManagement.TOTAL_REWARD_PERCENTAGE(),
+                invalidRewardPercentage
+            )
+        );
+        collateralManagement.setRewardPercentage(invalidRewardPercentage);
+        vm.stopPrank();
+
+        assertEq(
+            collateralManagement.getRewardPercentage(),
+            initialRewardPercentage,
+            "RewardPercentage should remain unchanged after revert"
+        );
+    }
+
+    function test_SetRewardPercentage_AllowsMaximumBoundary() public {
+        uint256 oldRewardPercentage = collateralManagement
+            .getRewardPercentage();
+        uint256 maxRewardPercentage = collateralManagement
+            .TOTAL_REWARD_PERCENTAGE();
+
+        vm.prank(owner);
+        vm.expectEmit(true, true, false, false);
+        emit CollateralManagementContract.RewardPercentageSet(
+            oldRewardPercentage,
+            maxRewardPercentage
+        );
+        collateralManagement.setRewardPercentage(maxRewardPercentage);
+
+        assertEq(
+            collateralManagement.getRewardPercentage(),
+            maxRewardPercentage,
+            "RewardPercentage should accept maximum basis points value"
+        );
+    }
+
     // ============ setResignDelayInBlocks function tests ============
 
     function test_SetResignDelayInBlocks_OnlyAllowsOwnerToModify() public {
diff --git a/test/discovery/Registration.t.sol b/test/discovery/Registration.t.sol
@@ -18,7 +18,12 @@ contract RegistrationTest is DiscoveryTestBase {
         FlyoverDiscovery implementation = new FlyoverDiscovery();
 
         vm.expectRevert(abi.encodeWithSignature("InvalidInitialization()"));
-        implementation.initialize(owner, 0, address(collateralManagement));
+        implementation.initialize(
+            owner,
+            0,
+            address(collateralManagement),
+            pauseRegistry
+        );
     }
 
     function test_Register_RegistersProvidersAndIncrementsLastProviderId()
diff --git a/test/pegin/Configuration.t.sol b/test/pegin/Configuration.t.sol
@@ -91,7 +91,8 @@ contract ConfigurationTest is PegInTestBase {
             TEST_DUST_THRESHOLD,
             TEST_MIN_PEGIN,
             address(collateralManagement),
-            false
+            false,
+            pauseRegistry
         );
     }
 
diff --git a/test/pegout/Configuration.t.sol b/test/pegout/Configuration.t.sol
@@ -67,7 +67,8 @@ contract ConfigurationTest is PegOutTestBase {
             TEST_DUST_THRESHOLD,
             address(collateralManagement),
             false,
-            TEST_BTC_BLOCK_TIME
+            TEST_BTC_BLOCK_TIME,
+            pauseRegistry
         );
     }
 
diff --git a/test/pegout/Withdraw.t.sol b/test/pegout/Withdraw.t.sol
@@ -64,7 +64,7 @@ contract WithdrawTest is PegOutTestBase {
         );
 
         bytes32 quoteHash = pegOutContract.hashPegOutQuote(quote);
-        bytes memory signature = signQuote(fullLp, quoteHash);
+        bytes memory signature = signQuote(fullLp, quote);
         changeReceiver.setFail(true);
 
         vm.prank(user);
@@ -238,7 +238,7 @@ contract WithdrawTest is PegOutTestBase {
         );
 
         bytes32 quoteHash = pegOutContract.hashPegOutQuote(quote);
-        bytes memory signature = signQuote(fullLp, quoteHash);
+        bytes memory signature = signQuote(fullLp, quote);
         receiver.setFail(true);
 
         vm.prank(user);
@@ -268,6 +268,7 @@ contract WithdrawTest is PegOutTestBase {
                 callFee: 100000000000000,
                 penaltyFee: 10000000000000,
                 value: value,
+                chainId: block.chainid,
                 gasFee: 100,
                 lbcAddress: address(pegOutContract),
                 lpRskAddress: lp,
@@ -307,7 +308,7 @@ contract WithdrawTest is PegOutTestBase {
 
     function signQuote(
         address signer,
-        bytes32 quoteHash
+        Quotes.PegOutQuote memory quote
     ) internal view returns (bytes memory) {
         uint256 privateKey;
         if (signer == fullLp) {
@@ -320,13 +321,8 @@ contract WithdrawTest is PegOutTestBase {
             revert("Unknown signer");
         }
 
-        bytes32 ethSignedMessageHash = keccak256(
-            abi.encodePacked("\x19Ethereum Signed Message:\n32", quoteHash)
-        );
-        (uint8 v, bytes32 r, bytes32 s) = vm.sign(
-            privateKey,
-            ethSignedMessageHash
-        );
+        bytes32 eip712Hash = pegOutContract.hashPegOutQuoteEIP712(quote);
+        (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, eip712Hash);
         return abi.encodePacked(r, s, v);
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@ contract PegOutPayer {`
`27`	`27`	`}`
`28`	`28`
`29`	`29`	`function executePegOut(QuotesV2.PegOutQuote calldata quote, bytes calldata signature) external {`
`30`		`- uint256 total = quote.value + quote.gasFee + quote.callFee + quote.productFeeAmount;`
	`30`	`+ uint256 total = quote.value + quote.gasFee + quote.callFee;`
`31`	`31`	`if (address(this).balance < total) {`
`32`	`32`	`revert InsufficientBalance(address(this).balance, total);`
`33`	`33`	`}`
Original file line number	Diff line number	Diff line change
`@@ -91,7 +91,8 @@ contract ConfigurationTest is PegInTestBase {`
`91`	`91`	`TEST_DUST_THRESHOLD,`
`92`	`92`	`TEST_MIN_PEGIN,`
`93`	`93`	`address(collateralManagement),`
`94`		`- false`
	`94`	`+ false,`
	`95`	`+ pauseRegistry`
`95`	`96`	`);`
`96`	`97`	`}`
`97`	`98`
Original file line number	Diff line number	Diff line change
`@@ -67,7 +67,8 @@ contract ConfigurationTest is PegOutTestBase {`
`67`	`67`	`TEST_DUST_THRESHOLD,`
`68`	`68`	`address(collateralManagement),`
`69`	`69`	`false,`
`70`		`- TEST_BTC_BLOCK_TIME`
	`70`	`+ TEST_BTC_BLOCK_TIME,`
	`71`	`+ pauseRegistry`
`71`	`72`	`);`
`72`	`73`	`}`
`73`	`74`