chore(contracts/core): improve validation (#2143)

Adding additional contract validation and tests.

issue: none

contracts/core/.gas-snapshot

@@ -1,12 +1,12 @@
-Admin_Test:test_pause_unpause() (gas: 26108503)
-Admin_Test:test_pause_unpause_bridge() (gas: 21283971)
-Admin_Test:test_pause_unpause_xcall() (gas: 26062445)
-Admin_Test:test_pause_unpause_xsubmit() (gas: 26062196)
-Admin_Test:test_upgrade() (gas: 29998091)
-AllocPredeploys_Test:test_num_allocs() (gas: 1181045803)
-AllocPredeploys_Test:test_predeploys() (gas: 1180982105)
-AllocPredeploys_Test:test_preinstalls() (gas: 1181617374)
-AllocPredeploys_Test:test_proxies() (gas: 1408625344)
+Admin_Test:test_pause_unpause() (gas: 26225827)
+Admin_Test:test_pause_unpause_bridge() (gas: 21379532)
+Admin_Test:test_pause_unpause_xcall() (gas: 26179757)
+Admin_Test:test_pause_unpause_xsubmit() (gas: 26179508)
+Admin_Test:test_upgrade() (gas: 30136865)
+AllocPredeploys_Test:test_num_allocs() (gas: 1181048975)
+AllocPredeploys_Test:test_predeploys() (gas: 1180985277)
+AllocPredeploys_Test:test_preinstalls() (gas: 1181620546)
+AllocPredeploys_Test:test_proxies() (gas: 1408628516)
 FeeOracleV1_Test:test_bulkSetFeeParams() (gas: 172862)
 FeeOracleV1_Test:test_feeFor() (gas: 122551)
 FeeOracleV1_Test:test_setBaseGasLimit() (gas: 32208)
@@ -25,9 +25,10 @@ FeeOracleV2_Test:test_setToNativeRate() (gas: 43377)
 InitializableHelper_Test:test_disableInitalizers() (gas: 181686)
 InitializableHelper_Test:test_getInitialized() (gas: 178023)
 OmniBridgeL1_Test:test_bridge() (gas: 233678)
-OmniBridgeL1_Test:test_pauseAll() (gas: 49368)
-OmniBridgeL1_Test:test_pauseBridging() (gas: 59580)
-OmniBridgeL1_Test:test_pauseWithdraws() (gas: 48975)
+OmniBridgeL1_Test:test_initialize() (gas: 1820095)
+OmniBridgeL1_Test:test_pauseAll() (gas: 49376)
+OmniBridgeL1_Test:test_pauseBridging() (gas: 59591)
+OmniBridgeL1_Test:test_pauseWithdraws() (gas: 48992)
 OmniBridgeL1_Test:test_stub() (gas: 143)
 OmniBridgeL1_Test:test_withdraw() (gas: 1386987)
 OmniBridgeNative_Test:test_bridge() (gas: 156805)
@@ -37,23 +38,25 @@ OmniBridgeNative_Test:test_pauseBridging() (gas: 44449)
 OmniBridgeNative_Test:test_pauseWithdraws() (gas: 61348)
 OmniBridgeNative_Test:test_stub() (gas: 143)
 OmniBridgeNative_Test:test_withdraw() (gas: 279800)
-OmniGasPump_Test:testFuzz_quote(uint32) (runs: 256, μ: 63922, ~: 63969)
-OmniGasPump_Test:test_fillUp() (gas: 231427)
-OmniGasPump_Test:test_pause() (gas: 62757)
-OmniGasPump_Test:test_setMaxSwap() (gas: 34749)
+OmniGasPump_Test:testFuzz_quote(uint32) (runs: 256, μ: 63944, ~: 63991)
+OmniGasPump_Test:test_fillUp() (gas: 237094)
+OmniGasPump_Test:test_pause() (gas: 62825)
+OmniGasPump_Test:test_setMaxSwap() (gas: 34771)
 OmniGasPump_Test:test_setOmniGasStation() (gas: 36697)
-OmniGasPump_Test:test_setOracle() (gas: 34992)
-OmniGasPump_Test:test_setToll() (gas: 32168)
+OmniGasPump_Test:test_setOracle() (gas: 35014)
+OmniGasPump_Test:test_setToll() (gas: 32080)
+OmniGasPump_Test:test_withdraw() (gas: 71440)
 OmniGasStation_Test:test_pause() (gas: 65220)
 OmniGasStation_Test:test_setPump() (gas: 83966)
 OmniGasStation_Test:test_settleUp() (gas: 365260)
 OmniPortal_Test:test_example() (gas: 610147)
-OmniPortal_admin_Test:test_pauseAll() (gas: 66007313)
-OmniPortal_admin_Test:test_pauseXCall() (gas: 271775)
+OmniPortal_admin_Test:test_pauseAll() (gas: 66007401)
+OmniPortal_admin_Test:test_pauseXCall() (gas: 271805)
 OmniPortal_admin_Test:test_pauseXSubmit() (gas: 236326)
 OmniPortal_admin_Test:test_setFeeOracle() (gas: 31402)
-OmniPortal_admin_Test:test_setInXBlockOffset() (gas: 54078)
+OmniPortal_admin_Test:test_setInXBlockOffset() (gas: 54189)
 OmniPortal_admin_Test:test_setInXMsgOffset() (gas: 54095)
+OmniPortal_admin_Test:test_setXMsgGasLimits() (gas: 62136)
 OmniPortal_admin_Test:test_setXSubValsetCutoff() (gas: 35050)
 OmniPortal_adversarial:test_xcallToPortal__fails() (gas: 76867)
 OmniPortal_exec_Test:test_call_notEnoughGas_reverts() (gas: 4997001)
@@ -66,37 +69,37 @@ OmniPortal_exec_Test:test_exec_xmsg_succeeds() (gas: 153298)
 OmniPortal_exec_Test:test_syscall_forwardsRevert() (gas: 20668)
 OmniPortal_feeFor_Test:test_feeFor_succeeds() (gas: 46602)
 OmniPortal_sys_Test:test_setNetwork() (gas: 136788)
-OmniPortal_xcall_Test:test_xcall_gasLimitTooHigh_reverts() (gas: 64729)
-OmniPortal_xcall_Test:test_xcall_gasLimitTooLow_reverts() (gas: 64928)
-OmniPortal_xcall_Test:test_xcall_insufficientFee_reverts() (gas: 69780)
+OmniPortal_xcall_Test:test_xcall_gasLimitTooHigh_reverts() (gas: 64741)
+OmniPortal_xcall_Test:test_xcall_gasLimitTooLow_reverts() (gas: 64934)
+OmniPortal_xcall_Test:test_xcall_insufficientFee_reverts() (gas: 69786)
 OmniPortal_xcall_Test:test_xcall_sameChain_reverts() (gas: 61020)
-OmniPortal_xcall_Test:test_xcall_succeeds() (gas: 105596)
-OmniPortal_xcall_Test:test_xcall_unsupportedConf_reverts() (gas: 40701)
+OmniPortal_xcall_Test:test_xcall_succeeds() (gas: 105602)
+OmniPortal_xcall_Test:test_xcall_unsupportedConf_reverts() (gas: 40707)
 OmniPortal_xcall_Test:test_xcall_unsupportedDest_reverts() (gas: 36188)
-OmniPortal_xsubmit_Test:test_xsubmit_addValidatorSet_succeeds() (gas: 66705195)
-OmniPortal_xsubmit_Test:test_xsubmit_duplicateValidator_reverts() (gas: 65687783)
-OmniPortal_xsubmit_Test:test_xsubmit_invalidAttestationRoot_reverts() (gas: 65738412)
-OmniPortal_xsubmit_Test:test_xsubmit_invalidMsgs_reverts() (gas: 65725680)
-OmniPortal_xsubmit_Test:test_xsubmit_noQuorum_reverts() (gas: 65694301)
-OmniPortal_xsubmit_Test:test_xsubmit_noXmsgs_reverts() (gas: 65670786)
-OmniPortal_xsubmit_Test:test_xsubmit_notNewValSet_succeeds() (gas: 66676075)
-OmniPortal_xsubmit_Test:test_xsubmit_oldValSet_reverts() (gas: 65684238)
-OmniPortal_xsubmit_Test:test_xsubmit_reentrancy_reverts() (gas: 65768336)
-OmniPortal_xsubmit_Test:test_xsubmit_unknownValSetId_reverts() (gas: 65676190)
-OmniPortal_xsubmit_Test:test_xsubmit_wrongConsensusChainId_reverts() (gas: 65674104)
-OmniPortal_xsubmit_Test:test_xsubmit_wrongDestChainId_reverts() (gas: 65726503)
-OmniPortal_xsubmit_Test:test_xsubmit_wrongStreamOffset_reverts() (gas: 65728860)
-OmniPortal_xsubmit_Test:test_xsubmit_xblock1_chainB_succeeds() (gas: 65950597)
-OmniPortal_xsubmit_Test:test_xsubmit_xblock1_succeeds() (gas: 65951825)
-OmniPortal_xsubmit_Test:test_xsubmit_xblock2_chainB_succeeds() (gas: 66629008)
-OmniPortal_xsubmit_Test:test_xsubmit_xblock2_succeeds() (gas: 66631717)
-OmniPortal_xsubmit_gas_Test:test_singleExec() (gas: 66258656)
-OmniPortal_xsubmit_gas_Test:test_xsubmit_addValidator_succeeds() (gas: 65849755)
-OmniPortal_xsubmit_gas_Test:test_xsubmit_guzzle10_succeeds() (gas: 66911791)
-OmniPortal_xsubmit_gas_Test:test_xsubmit_guzzle1_succeeds() (gas: 65863128)
-OmniPortal_xsubmit_gas_Test:test_xsubmit_guzzle25_succeeds() (gas: 68660724)
-OmniPortal_xsubmit_gas_Test:test_xsubmit_guzzle50_succeeds() (gas: 71578682)
-OmniPortal_xsubmit_gas_Test:test_xsubmit_guzzle5_succeeds() (gas: 66329245)
+OmniPortal_xsubmit_Test:test_xsubmit_addValidatorSet_succeeds() (gas: 66705300)
+OmniPortal_xsubmit_Test:test_xsubmit_duplicateValidator_reverts() (gas: 65687887)
+OmniPortal_xsubmit_Test:test_xsubmit_invalidAttestationRoot_reverts() (gas: 65738516)
+OmniPortal_xsubmit_Test:test_xsubmit_invalidMsgs_reverts() (gas: 65725784)
+OmniPortal_xsubmit_Test:test_xsubmit_noQuorum_reverts() (gas: 65694405)
+OmniPortal_xsubmit_Test:test_xsubmit_noXmsgs_reverts() (gas: 65670890)
+OmniPortal_xsubmit_Test:test_xsubmit_notNewValSet_succeeds() (gas: 66676180)
+OmniPortal_xsubmit_Test:test_xsubmit_oldValSet_reverts() (gas: 65684342)
+OmniPortal_xsubmit_Test:test_xsubmit_reentrancy_reverts() (gas: 65768440)
+OmniPortal_xsubmit_Test:test_xsubmit_unknownValSetId_reverts() (gas: 65676295)
+OmniPortal_xsubmit_Test:test_xsubmit_wrongConsensusChainId_reverts() (gas: 65674208)
+OmniPortal_xsubmit_Test:test_xsubmit_wrongDestChainId_reverts() (gas: 65726607)
+OmniPortal_xsubmit_Test:test_xsubmit_wrongStreamOffset_reverts() (gas: 65728964)
+OmniPortal_xsubmit_Test:test_xsubmit_xblock1_chainB_succeeds() (gas: 65950702)
+OmniPortal_xsubmit_Test:test_xsubmit_xblock1_succeeds() (gas: 65951930)
+OmniPortal_xsubmit_Test:test_xsubmit_xblock2_chainB_succeeds() (gas: 66629113)
+OmniPortal_xsubmit_Test:test_xsubmit_xblock2_succeeds() (gas: 66631823)
+OmniPortal_xsubmit_gas_Test:test_singleExec() (gas: 66258760)
+OmniPortal_xsubmit_gas_Test:test_xsubmit_addValidator_succeeds() (gas: 65849860)
+OmniPortal_xsubmit_gas_Test:test_xsubmit_guzzle10_succeeds() (gas: 66911896)
+OmniPortal_xsubmit_gas_Test:test_xsubmit_guzzle1_succeeds() (gas: 65863233)
+OmniPortal_xsubmit_gas_Test:test_xsubmit_guzzle25_succeeds() (gas: 68660829)
+OmniPortal_xsubmit_gas_Test:test_xsubmit_guzzle50_succeeds() (gas: 71578787)
+OmniPortal_xsubmit_gas_Test:test_xsubmit_guzzle5_succeeds() (gas: 66329350)
 Omni_Test:test_constructor() (gas: 1008119)
 PortalRegistry_Test:test_register() (gas: 1092038)
 PortalRegistry_Test:test_stub() (gas: 143)
@@ -113,6 +116,7 @@ Quorum_Test:test_verify_succeeds() (gas: 294059)
 Slashing_Test:test_stub() (gas: 143)
 Slashing_Test:test_unjail() (gas: 54734)
 Staking_Test:test_createValidator() (gas: 107993)
+Staking_Test:test_delegate() (gas: 109941)
 Staking_Test:test_stub() (gas: 143)
 Upgrade_Test:test_stub() (gas: 143)
 XAppUpgradeable_Test:test_isXCall() (gas: 148924)

contracts/core/src/octane/Staking.sol

@@ -101,6 +101,7 @@ contract Staking is OwnableUpgradeable {
      * @dev Proxies x/staking.MsgDelegate
      */
     function delegate(address validator) external payable {
+        require(!isAllowlistEnabled || isAllowedValidator[validator], "Staking: not allowed val");
         require(msg.value >= MinDelegation, "Staking: insufficient deposit");
 
         // only support self delegation for now

contracts/core/src/token/OmniBridgeL1.sol

@@ -46,6 +46,7 @@ contract OmniBridgeL1 is OmniBridgeCommon {
     }
 
     function initialize(address owner_, address omni_) external initializer {
+        require(omni_ != address(0), "OmniBridge: no zero addr");
         __Ownable_init(owner_);
         omni = IOmniPortal(omni_);
     }

contracts/core/src/token/OmniGasPump.sol

@@ -84,6 +84,8 @@ contract OmniGasPump is IOmniGasPump, XAppUpgradeable, OwnableUpgradeable, Pausa
      * @param recipient Address on Omni to send OMNI to
      */
     function fillUp(address recipient) public payable whenNotPaused returns (uint256) {
+        require(recipient != address(0), "OmniGasPump: no zero addr");
+
         // take xcall fee
         uint256 f = xfee();
         require(msg.value >= f, "OmniGasPump: insufficient fee");
@@ -188,6 +190,7 @@ contract OmniGasPump is IOmniGasPump, XAppUpgradeable, OwnableUpgradeable, Pausa
 
     /// @notice Withdraw collected ETH to `to`
     function withdraw(address to) external onlyOwner {
+        require(to != address(0), "OmniGasPump: no zero addr");
         (bool success,) = to.call{ value: address(this).balance }("");
         require(success, "OmniGasPump: withdraw failed");
     }

contracts/core/src/xchain/OmniPortal.sol

@@ -90,8 +90,8 @@ contract OmniPortal is
 
         _setFeeOracle(p.feeOracle);
         _setXMsgMaxGasLimit(p.xmsgMaxGasLimit);
-        _setXMsgMinGasLimit(p.xmsgMinGasLimit);
         _setXMsgMaxDataSize(p.xmsgMaxDataSize);
+        _setXMsgMinGasLimit(p.xmsgMinGasLimit);
         _setXReceiptMaxErrorSize(p.xreceiptMaxErrorSize);
         _setXSubValsetCutoff(p.xsubValsetCutoff);
         _addValidatorSet(p.valSetId, p.validators);
@@ -650,6 +650,7 @@ contract OmniPortal is
      */
     function _setXMsgMinGasLimit(uint64 gasLimit) internal {
         require(gasLimit > 0, "OmniPortal: no zero min gas");
+        require(gasLimit < xmsgMaxGasLimit, "OmniPortal: not below max");
         xmsgMinGasLimit = gasLimit;
         emit XMsgMinGasLimitSet(gasLimit);
     }
@@ -658,7 +659,7 @@ contract OmniPortal is
      * @notice Set the maximum gas limit for xmsg
      */
     function _setXMsgMaxGasLimit(uint64 gasLimit) internal {
-        require(gasLimit > 0, "OmniPortal: no zero max gas");
+        require(gasLimit > xmsgMinGasLimit, "OmniPortal: not above min");
         xmsgMaxGasLimit = gasLimit;
         emit XMsgMaxGasLimitSet(gasLimit);
     }

contracts/core/test/octane/Staking.t.sol

@@ -12,6 +12,9 @@ contract Staking_Test is Test {
     /// @dev Matches Staking.CreateValidator event
     event CreateValidator(address indexed validator, bytes pubkey, uint256 deposit);
 
+    /// @dev Matches Staking.Delegate event
+    event Delegate(address indexed delegator, address indexed validator, uint256 amount);
+
     address owner;
     StakingHarness staking;
 
@@ -89,6 +92,42 @@ contract Staking_Test is Test {
         vm.prank(validator);
         staking.createValidator{ value: deposit }(pubkey);
     }
+
+    function test_delegate() public {
+        // requires min delegation
+        address validator = makeAddr("validator");
+        uint256 minDelegation = staking.MinDelegation();
+
+        vm.deal(validator, minDelegation);
+
+        vm.expectRevert("Staking: insufficient deposit");
+        staking.delegate{ value: minDelegation - 1 }(validator);
+
+        // requires self-delegation
+        vm.expectRevert("Staking: only self delegation");
+        vm.prank(validator);
+        staking.delegate{ value: minDelegation }(makeAddr("someone else"));
+
+        // if allowlist enabled, must be in allowlist
+        vm.prank(owner);
+        staking.enableAllowlist();
+
+        vm.expectRevert("Staking: not allowed val");
+        vm.prank(validator);
+        staking.delegate{ value: minDelegation }(validator);
+
+        // succeeds
+        address[] memory validators = new address[](1);
+        validators[0] = validator;
+        vm.prank(owner);
+        staking.allowValidators(validators);
+
+        vm.expectEmit();
+        emit Delegate(validator, validator, minDelegation);
+
+        vm.prank(validator);
+        staking.delegate{ value: minDelegation }(validator);
+    }
 }
 
 /**

contracts/core/test/token/OmniBridgeL1.t.sol

@@ -48,6 +48,14 @@ contract OmniBridgeL1_Test is Test {
         );
     }
 
+    function test_initialize() public {
+        address impl = address(new OmniBridgeL1(address(token)));
+        address proxy = address(new TransparentUpgradeableProxy(impl, proxyAdmin, ""));
+
+        vm.expectRevert("OmniBridge: no zero addr");
+        OmniBridgeL1(proxy).initialize(owner, address(0));
+    }
+
     function test_bridge() public {
         address to = makeAddr("to");
         uint256 amount = 1e18;

contracts/core/test/token/OmniGasPump.t.sol

@@ -97,6 +97,10 @@ contract OmniGasPump_Test is Test {
         uint64 omniChainId = portal.omniChainId();
         vm.deal(recipient, initialBalance);
 
+        // no zero recipient
+        vm.expectRevert("OmniGasPump: no zero addr");
+        pump.fillUp(address(0));
+
         // requires fee
         vm.expectRevert("OmniGasPump: insufficient fee");
         pump.fillUp(recipient);
@@ -255,6 +259,28 @@ contract OmniGasPump_Test is Test {
         pump.fillUp(recipient);
     }
 
+    function test_withdraw() public {
+        vm.deal(address(pump), 10 ether);
+
+        // no zero address
+        vm.prank(owner);
+        vm.expectRevert("OmniGasPump: no zero addr");
+        pump.withdraw(address(0));
+
+        // only owner
+        address notOwner = address(0x456);
+        vm.prank(notOwner);
+        vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, notOwner));
+        pump.withdraw(owner);
+
+        // success
+        address to = makeAddr("to");
+        vm.prank(owner);
+        pump.withdraw(to);
+        assertEq(address(pump).balance, 0);
+        assertEq(to.balance, 10 ether);
+    }
+
     /// @notice Test that GasPump.quote is accurate
     function testFuzz_quote(uint32 _targetOMNI) public view {
         uint256 targetOMNI = bound(_targetOMNI, 0.1 ether, 10 ether);

contracts/core/test/xchain/OmniPortal_admin.t.sol

@@ -280,4 +280,40 @@ contract OmniPortal_admin_Test is Base {
 
         assertEq(portal.xsubValsetCutoff(), 1);
     }
+
+    function test_setXMsgGasLimits() public {
+        // only owner
+        address notOwner = address(0x456);
+
+        vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, notOwner));
+        vm.prank(notOwner);
+        portal.setXMsgMinGasLimit(1);
+
+        vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, notOwner));
+        vm.prank(notOwner);
+        portal.setXMsgMaxGasLimit(1);
+
+        vm.startPrank(owner);
+        uint64 min = portal.xmsgMinGasLimit();
+        uint64 max = portal.xmsgMaxGasLimit();
+
+        // min must be below max
+        vm.expectRevert("OmniPortal: not below max");
+        portal.setXMsgMinGasLimit(max);
+
+        // max must be above min
+        vm.expectRevert("OmniPortal: not above min");
+        portal.setXMsgMaxGasLimit(min);
+
+        // success
+        uint64 newMin = min - 1;
+        portal.setXMsgMinGasLimit(newMin);
+        assertEq(portal.xmsgMinGasLimit(), newMin);
+
+        uint64 newMax = max + 1;
+        portal.setXMsgMaxGasLimit(newMax);
+        assertEq(portal.xmsgMaxGasLimit(), newMax);
+
+        vm.stopPrank();
+    }
 }