Skip to content

Commit

Permalink
Fix BridgedGovernor issues found in the audit
Browse files Browse the repository at this point in the history
  • Loading branch information
@CodeSandwich
CodeSandwich committed Sep 11, 2024
1 parent d080e51 commit f3d7e6f
Show file tree
Hide file tree
Showing 4 changed files with 392 additions and 37 deletions.
20 changes: 9 additions & 11 deletions scripts/DeployAxelarBridgedGovernor.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
pragma solidity ^0.8.20;

import {console, Script} from "forge-std/Script.sol";
import {AxelarBridgedGovernor, BridgedGovernorProxy, Call} from "src/BridgedGovernor.sol";
import {AxelarBridgedGovernor, GovernorProxy, Call} from "src/BridgedGovernor.sol";
import {IAxelarGasService} from "axelar/interfaces/IAxelarGasService.sol";
import {IAxelarGMPGateway} from "axelar/interfaces/IAxelarGMPGateway.sol";
import {AddressToString} from "axelar/libs/AddressString.sol";
Expand All @@ -27,13 +27,12 @@ IAxelarGMPGateway constant BSC_TESTNET_GATEWAY =
// vm.startBroadcast();
// AxelarBridgedGovernor logic =
// new AxelarBridgedGovernor(BSC_TESTNET_GATEWAY, SEPOLIA_CHAIN_NAME, owner);
// BridgedGovernorProxy governor = new BridgedGovernorProxy(address(logic), new Call[](0));
// GovernorProxy governor = new GovernorProxy(address(logic), new Call[](0));
// vm.stopBroadcast();
// console.log("Deployed AxelarBridgedGovernor:", address(governor));
// }
// }


// Gateway and ddresses taken from https://docs.axelar.dev/resources/contract-addresses/testnet

// Run on BSC testnet
Expand Down Expand Up @@ -64,14 +63,14 @@ contract DeployGovernor is Script {

vm.startBroadcast();
AxelarBridgedGovernor logic = new AxelarBridgedGovernor(gateway, sourceChain, owner);
BridgedGovernorProxy governor = new BridgedGovernorProxy(address(logic), new Call[](0));
GovernorProxy governor = new GovernorProxy(address(logic), new Call[](0));
vm.stopBroadcast();
console.log("Deployed AxelarBridgedGovernor:", address(governor));
}
}

contract ContractCaller {
address public immutable owner;
address public immutable owner;
IAxelarGMPGateway public immutable gateway;
IAxelarGasService public immutable gasService;

Expand All @@ -85,12 +84,12 @@ contract ContractCaller {
}

function setRecipient(string calldata destinationChain_, address recipient_) public {
require(msg.sender == owner, "Only owner");
destinationChain = destinationChain_;
recipient = recipient_;
require(msg.sender == owner, "Only owner");
destinationChain = destinationChain_;
recipient = recipient_;
}

function callContract(bytes calldata payload) payable public {
function callContract(bytes calldata payload) public payable {
require(msg.sender == owner, "Only owner");
string memory recipient_ = AddressToString.toString(recipient);
if (msg.value > 0) {
Expand All @@ -102,11 +101,10 @@ contract ContractCaller {
}
}


contract ContractCall is Script {
function run() public {
ContractCaller caller = ContractCaller(vm.envAddress("CALLER"));
uint256 fee = vm.envOr("FEE", uint(0));
uint256 fee = vm.envOr("FEE", uint256(0));
uint256 nonce = vm.envUint("NONCE");

Call[] memory calls = new Call[](1);
Expand Down
4 changes: 2 additions & 2 deletions scripts/DeployLZBridgedGovernor.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
import {SetConfigParam} from "layer-zero-v2/protocol/contracts/interfaces/IMessageLibManager.sol";
import {Constant} from "layer-zero-v2/messagelib/test/util/Constant.sol";
import {Strings} from "openzeppelin-contracts/utils/Strings.sol";
import {LZBridgedGovernor, BridgedGovernorProxy, Call} from "src/BridgedGovernor.sol";
import {LZBridgedGovernor, GovernorProxy, Call} from "src/BridgedGovernor.sol";

// Taken from layer-zero-v2/messagelib/contracts/uln/UlnBase.sol
struct UlnConfig {
Expand Down Expand Up @@ -152,7 +152,7 @@ contract DeployToBscTestnet is Script {
vm.startBroadcast();
address governorLogic =
address(new LZBridgedGovernor(BSC_TESTNET_ENDPOINT, SEPOLIA_EID, owner));
address governorProxy = address(new BridgedGovernorProxy(governorLogic, calls));
address governorProxy = address(new GovernorProxy(governorLogic, calls));
vm.stopBroadcast();

require(governorProxy == governor, "Invalid deployment address");
Expand Down
60 changes: 36 additions & 24 deletions src/BridgedGovernor.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,17 @@ struct Call {
uint256 value;
}

/// @notice Run the list of calls.
/// @notice Execute the list of calls.
/// If any of the calls reverts, reverts bubbling the error.
/// All the targets must be smart contracts, calling an EOA will revert.
/// @param calls The list of calls to run.
function runCalls(Call[] memory calls) {
/// @param calls The list of calls to execute.
function executeCalls(Call[] memory calls) {
for (uint256 i = 0; i < calls.length; i++) {
Call memory call = calls[i];
Address.functionCallWithValue(call.target, call.data, call.value);
if (call.data.length > 0 || Address.isContract(call.target)) {
Address.functionCallWithValue(call.target, call.data, call.value);
} else {
Address.sendValue(payable(call.target), call.value);
}
}
}

Expand All @@ -44,13 +47,22 @@ abstract contract Governor is UUPSUpgradeable {
/// @param nonce The nonce of the message.
event MessageExecuted(uint256 nonce);

receive() external payable {}

/// @notice Returns the current implementation address.
/// @return implementation_ The implementation address.
function implementation() public view returns (address implementation_) {
return _getImplementation();
}

/// @notice Executes the message.
/// @param nonce The message nonce, must be equal to `nextMessageNonce`.
/// @param calls The list of calls to run.
/// @param calls The list of calls to execute.
function _executeMessage(uint256 nonce, Call[] memory calls) internal {
require(nonce == nextMessageNonce, "Invalid message nonce");
executeCalls(calls);
require(nonce == nextMessageNonce, "Message execution reentrancy");
nextMessageNonce++;
runCalls(calls);
emit MessageExecuted(nonce);
}

Expand All @@ -59,7 +71,7 @@ abstract contract Governor is UUPSUpgradeable {
}
}

/// @notice The governor running calls received from its owner on another chain using LayerZero v2.
/// @notice The governor executing messages from its owner on another chain using LayerZero v2.
contract LZBridgedGovernor is Governor, ILayerZeroReceiver {
/// @notice The LayerZero v2 endpoint that is allowed to execute received messages.
address public immutable endpoint;
Expand All @@ -75,7 +87,7 @@ contract LZBridgedGovernor is Governor, ILayerZeroReceiver {
uint256 nonce;
/// @notice The minimum accepted `msg.value` passed with the message.
uint256 value;
/// @notice The list of calls to run.
/// @notice The list of calls to execute.
Call[] calls;
}

Expand Down Expand Up @@ -141,7 +153,7 @@ contract LZBridgedGovernor is Governor, ILayerZeroReceiver {
}
}

/// @notice The governor running calls received from its owner on another chain using Axelar.
/// @notice The governor executing messages from its owner on another chain using Axelar.
contract AxelarBridgedGovernor is Governor, IAxelarGMPExecutable {
/// @notice The Axelar gateway used by this contract.
IAxelarGMPGateway public immutable override gateway;
Expand All @@ -150,20 +162,20 @@ contract AxelarBridgedGovernor is Governor, IAxelarGMPExecutable {
/// @notice The owner address which is allowed to send messages.
address public immutable owner;

/// @notice The name of the chain from which the owner is allowed to send messages.
/// @return ownerChain_ The name of the chain.
function ownerChain() public view returns (string memory ownerChain_) {
return ShortStrings.toString(_ownerChain);
}

/// @notice The message passed over the bridge to the governor to execute.
struct Message {
/// @notice The message nonce, must be equal to `nextMessageNonce` when executed.
uint256 nonce;
/// @notice The list of calls to run.
/// @notice The list of calls to execute.
Call[] calls;
}

/// @notice The name of the chain from which the owner is allowed to send messages.
/// @return ownerChain_ The name of the chain.
function ownerChain() public view onlyProxy returns (string memory ownerChain_) {
return ShortStrings.toString(_ownerChain);
}

/// @param gateway_ The Axelar gateway used by this contract.
/// @param ownerChain_ The name of the chain from which the owner is allowed to send messages.
/// @param owner_ The owner address which is allowed to send messages.
Expand All @@ -185,24 +197,24 @@ contract AxelarBridgedGovernor is Governor, IAxelarGMPExecutable {
string calldata sourceChain,
string calldata sender,
bytes calldata payload
) public {
) public onlyProxy {
require(Strings.equal(sourceChain, ownerChain()), "Invalid message source chain");
require(StringToAddress.toAddress(sender) == owner, "Invalid message sender");
if (!gateway.validateContractCall(commandId, sourceChain, sender, keccak256(payload))) {
revert NotApprovedByGateway();
}
require(Strings.equal(sourceChain, ownerChain()), "Invalid message source chain");
require(StringToAddress.toAddress(sender) == owner, "Invalid message sender");

Message memory message = abi.decode(payload, (Message));
_executeMessage(message.nonce, message.calls);
}
}

/// @notice The specialized proxy for `BridgedGovernor`.
contract BridgedGovernorProxy is ERC1967Proxy {
contract GovernorProxy is ERC1967Proxy {
/// @param logic The initial address of the logic for the proxy.
/// @param calls The list of `Call`s to run while executing the constructor.
/// @param calls The list of `Call`s to execute while running the constructor.
/// It should at least set up the initial LayerZero v2 configuration.
constructor(address logic, Call[] memory calls) ERC1967Proxy(logic, "") {
runCalls(calls);
constructor(address logic, Call[] memory calls) payable ERC1967Proxy(logic, "") {
executeCalls(calls);
}
}
Loading

0 comments on commit f3d7e6f

Please sign in to comment.