From 4a7768b7f4f425948a772a6729b98460dcc3da89 Mon Sep 17 00:00:00 2001 From: GitGuru7 <128375421+GitGuru7@users.noreply.github.com> Date: Tue, 30 Apr 2024 14:37:10 +0530 Subject: [PATCH 1/3] fix: VEN-4 --- .../Cross-chain/OmnichainExecutorOwner.sol | 16 ++++++++++++ .../Cross-chain/OmnichainProposalSender.sol | 1 + .../IOmnichainGovernanceExecutor.sol | 7 +++++ tests/Cross-chain/Omnichain.ts | 26 ++++++++++++++++--- 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/contracts/Cross-chain/OmnichainExecutorOwner.sol b/contracts/Cross-chain/OmnichainExecutorOwner.sol index e4876034..989f241c 100644 --- a/contracts/Cross-chain/OmnichainExecutorOwner.sol +++ b/contracts/Cross-chain/OmnichainExecutorOwner.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.25; import { AccessControlledV8 } from "../Governance/AccessControlledV8.sol"; import { IOmnichainGovernanceExecutor } from "./interfaces/IOmnichainGovernanceExecutor.sol"; +import { ensureNonzeroAddress } from "@venusprotocol/solidity-utilities/contracts/validators.sol"; /** * @title OmnichainExecutorOwner @@ -45,6 +46,21 @@ contract OmnichainExecutorOwner is AccessControlledV8 { __AccessControlled_init(accessControlManager_); } + /** + * @notice Sets the source message sender address + * @param srcChainId_ The LayerZero id of a source chain + * @param srcAddress_ The address of the contract on the source chain + * @custom:access Controlled by AccessControlManager + * @custom:event Emits SetTrustedRemoteAddress with source chain Id and source address + */ + function setTrustedRemoteAddress(uint16 srcChainId_, bytes calldata srcAddress_) external { + _checkAccessAllowed("setTrustedRemoteAddress(uint16,bytes)"); + require(srcChainId_ != 0, "ChainId must not be zero"); + ensureNonzeroAddress(address(uint160(bytes20(srcAddress_)))); + require(srcAddress_.length == 20, "Source address must be 20 bytes long"); + OMNICHAIN_GOVERNANCE_EXECUTOR.setTrustedRemoteAddress(srcChainId_, srcAddress_); + } + /** * @notice Invoked when called function does not exist in the contract * @param data_ Calldata containing the encoded function call diff --git a/contracts/Cross-chain/OmnichainProposalSender.sol b/contracts/Cross-chain/OmnichainProposalSender.sol index e0fe3467..87f231ed 100644 --- a/contracts/Cross-chain/OmnichainProposalSender.sol +++ b/contracts/Cross-chain/OmnichainProposalSender.sol @@ -263,6 +263,7 @@ contract OmnichainProposalSender is ReentrancyGuard, BaseOmnichainControllerSrc _ensureAllowed("setTrustedRemoteAddress(uint16,bytes)"); require(remoteChainId_ != 0, "OmnichainProposalSender: chainId must not be zero"); ensureNonzeroAddress(address(uint160(bytes20(newRemoteAddress_)))); + require(newRemoteAddress_.length == 20, "OmnichainProposalSender: remote address must be 20 bytes long"); bytes memory oldRemoteAddress = trustedRemoteLookup[remoteChainId_]; trustedRemoteLookup[remoteChainId_] = abi.encodePacked(newRemoteAddress_, address(this)); emit SetTrustedRemoteAddress(remoteChainId_, oldRemoteAddress, trustedRemoteLookup[remoteChainId_]); diff --git a/contracts/Cross-chain/interfaces/IOmnichainGovernanceExecutor.sol b/contracts/Cross-chain/interfaces/IOmnichainGovernanceExecutor.sol index 2f95389b..9e0774c3 100644 --- a/contracts/Cross-chain/interfaces/IOmnichainGovernanceExecutor.sol +++ b/contracts/Cross-chain/interfaces/IOmnichainGovernanceExecutor.sol @@ -7,4 +7,11 @@ interface IOmnichainGovernanceExecutor { * @param addr The address to which ownership will be transferred */ function transferOwnership(address addr) external; + + /** + * @notice Sets the source message sender address + * @param srcChainId_ The LayerZero id of a source chain + * @param srcAddress_ The address of the contract on the source chain + */ + function setTrustedRemoteAddress(uint16 srcChainId_, bytes calldata srcAddress_) external; } diff --git a/tests/Cross-chain/Omnichain.ts b/tests/Cross-chain/Omnichain.ts index 0e846aba..2d31b5ba 100644 --- a/tests/Cross-chain/Omnichain.ts +++ b/tests/Cross-chain/Omnichain.ts @@ -81,7 +81,6 @@ describe("Omnichain: ", async function () { "setSendVersion(uint16)", "setReceiveVersion(uint16)", "forceResumeReceive(uint16,bytes)", - "setTrustedRemoteAddress(uint16,bytes)", "setPrecrime(address)", "setMinDstGas(uint16,uint16,uint256)", "setPayloadSizeLimit(uint16,uint256)", @@ -266,6 +265,12 @@ describe("Omnichain: ", async function () { ).to.be.revertedWith("OmnichainProposalSender: destination chain is not a trusted source"); }); + it("Reverts if remote address is more than 20 bytes", async function () { + await expect(sender.connect(signer1).setTrustedRemoteAddress(remoteChainId, remotePath + "99")).to.be.revertedWith( + "OmnichainProposalSender: remote address must be 20 bytes long", + ); + }); + it("Reverts with Daily Transaction Limit Exceed", async function () { await sender.connect(signer1).setMaxDailyLimit(remoteChainId, 0); const payload = await getPayload(NormalTimelock.address); @@ -306,9 +311,22 @@ describe("Omnichain: ", async function () { }); it("Function registry should not emit event if function is added twice", async function () { - expect( - await executorOwner.connect(deployer).upsertSignature(["setTrustedRemoteAddress(uint16,bytes)"], [true]), - ).to.not.emit(executorOwner, "FunctionRegistryChanged"); + await expect(executorOwner.connect(deployer).upsertSignature(["pause()"], [true])).to.not.emit( + executorOwner, + "FunctionRegistryChanged", + ); + }); + + it("Reverts if invalid parameters passed in trusted remote", async function () { + await expect(executorOwner.connect(signer1).setTrustedRemoteAddress(0, localPath)).to.be.revertedWith( + "ChainId must not be zero", + ); + await expect( + executorOwner.connect(signer1).setTrustedRemoteAddress(localChainId, ethers.constants.AddressZero), + ).to.be.revertedWithCustomError(executor, "ZeroAddressNotAllowed"); + await expect( + executorOwner.connect(signer1).setTrustedRemoteAddress(localChainId, localPath + "99"), + ).to.be.revertedWith("Source address must be 20 bytes long"); }); it("Reverts if EOA called owner function of Executor", async function () { From 70de9ffb550cafbe7b1d5cbc35dba8b24e89ede3 Mon Sep 17 00:00:00 2001 From: GitGuru7 <128375421+GitGuru7@users.noreply.github.com> Date: Tue, 30 Apr 2024 15:01:23 +0530 Subject: [PATCH 2/3] fix: VEN-8 --- .../OmnichainGovernanceExecutor.sol | 4 ++ tests/Cross-chain/Omnichain.ts | 44 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/contracts/Cross-chain/OmnichainGovernanceExecutor.sol b/contracts/Cross-chain/OmnichainGovernanceExecutor.sol index 604b7a53..af404a02 100644 --- a/contracts/Cross-chain/OmnichainGovernanceExecutor.sol +++ b/contracts/Cross-chain/OmnichainGovernanceExecutor.sol @@ -278,6 +278,10 @@ contract OmnichainGovernanceExecutor is ReentrancyGuard, BaseOmnichainController uint64 nonce_, bytes calldata payload_ ) public payable override onlyOwner nonReentrant { + require( + keccak256(trustedRemoteLookup[srcChainId_]) == keccak256(srcAddress_), + "OmnichainGovernanceExecutor::retryMessage: not a trusted remote" + ); super.retryMessage(srcChainId_, srcAddress_, nonce_, payload_); } diff --git a/tests/Cross-chain/Omnichain.ts b/tests/Cross-chain/Omnichain.ts index 2d31b5ba..78c4f208 100644 --- a/tests/Cross-chain/Omnichain.ts +++ b/tests/Cross-chain/Omnichain.ts @@ -631,6 +631,50 @@ describe("Omnichain: ", async function () { expect(await executor.failedMessages(localChainId, srcAddress, 1)).equals(ethers.constants.HashZero); }); + it("Revert retry message on destination if trusted remote is changed", async function () { + let data = executor.interface.encodeFunctionData("pause", []); + const srcAddress = ethers.utils.solidityPack(["address", "address"], [sender.address, executor.address]); + await expect( + signer1.sendTransaction({ + to: executorOwner.address, + data: data, + }), + ).to.emit(executor, "Paused"); + const payload = await getPayload(NormalTimelock.address); + expect( + await sender.connect(signer1).execute(remoteChainId, payload, adapterParams, ethers.constants.AddressZero, { + value: nativeFee, + }), + ).to.emit(sender, "ExecuteRemoteProposal"); + expect(await executor.failedMessages(localChainId, srcAddress, 1)).not.equals(ethers.constants.HashZero); + + data = executor.interface.encodeFunctionData("unpause", []); + await expect( + signer1.sendTransaction({ + to: executorOwner.address, + data: data, + }), + ).to.emit(executor, "Unpaused"); + + const addressOne = "0x0000000000000000000000000000000000000001"; + + //change trusted remote + await executorOwner.connect(signer1).setTrustedRemoteAddress(localChainId, addressOne); + + data = executor.interface.encodeFunctionData("retryMessage", [ + localChainId, + srcAddress, + 1, + await payloadWithId(payload), + ]); + await expect( + signer1.sendTransaction({ + to: executorOwner.address, + data: data, + }), + ).to.be.reverted; + }); + it("Retry messages that failed due to low gas at the destination using the Endpoint.", async function () { // low destination gas const adapterParamsLocal = ethers.utils.solidityPack(["uint16", "uint256"], [remoteChainId, 10000]); From ebe888c73e1ad9b77eea9bec1b2596c75a96cc6f Mon Sep 17 00:00:00 2001 From: GitGuru7 <128375421+GitGuru7@users.noreply.github.com> Date: Tue, 30 Apr 2024 15:04:48 +0530 Subject: [PATCH 3/3] fix: VEN-9 --- contracts/Cross-chain/OmnichainProposalSender.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/Cross-chain/OmnichainProposalSender.sol b/contracts/Cross-chain/OmnichainProposalSender.sol index 87f231ed..d060c848 100644 --- a/contracts/Cross-chain/OmnichainProposalSender.sol +++ b/contracts/Cross-chain/OmnichainProposalSender.sol @@ -122,7 +122,7 @@ contract OmnichainProposalSender is ReentrancyGuard, BaseOmnichainControllerSrc * @param payload_ The payload to be sent to the remote chain * It's computed as follows: payload = abi.encode(targets, values, signatures, calldatas, proposalType) * @param adapterParams_ The params used to specify the custom amount of gas required for the execution on the destination - * @param zroPaymentAddress_ The address of the ZRO token holder who would pay for the transaction + * @param zroPaymentAddress_ The address of the ZRO token holder who would pay for the transaction. This must be either address(this) or tx.origin * @custom:event Emits ExecuteRemoteProposal with remote chain id, proposal ID and payload on success * @custom:event Emits StorePayload with last stored payload proposal ID ,remote chain id , payload, adapter params , values and reason for failure * @custom:access Controlled by Access Control Manager