Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Native ETH in v1 #1354

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions contracts/scripts/DeployLocal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,11 @@ contract DeployLocal is Script {
// Deploy WETH for testing
new WETH9();

// Fund the sovereign account for the BridgeHub parachain. Used to reward relayers
// Fund the gateway proxy contract. Used to reward relayers
// of messages originating from BridgeHub
uint256 initialDeposit = vm.envUint("BRIDGE_HUB_INITIAL_DEPOSIT");
uint256 initialDeposit = vm.envUint("GATEWAY_PROXY_INITIAL_DEPOSIT");

address bridgeHubAgent = IGateway(address(gateway)).agentOf(bridgeHubAgentID);
address assetHubAgent = IGateway(address(gateway)).agentOf(assetHubAgentID);

payable(bridgeHubAgent).safeNativeTransfer(initialDeposit);
payable(assetHubAgent).safeNativeTransfer(initialDeposit);
payable(gateway).safeNativeTransfer(initialDeposit);

// Deploy MockGatewayV2 for testing
new MockGatewayV2();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {ParaID} from "../src/Types.sol";
import {SafeNativeTransfer} from "../src/utils/SafeTransfer.sol";
import {stdJson} from "forge-std/StdJson.sol";

contract FundAgent is Script {
contract FundGateway is Script {
using SafeNativeTransfer for address payable;
using stdJson for string;

Expand All @@ -26,17 +26,10 @@ contract FundAgent is Script {
address deployer = vm.rememberKey(privateKey);
vm.startBroadcast(deployer);

uint256 initialDeposit = vm.envUint("BRIDGE_HUB_INITIAL_DEPOSIT");
uint256 initialDeposit = vm.envUint("GATEWAY_PROXY_INITIAL_DEPOSIT");
address gatewayAddress = vm.envAddress("GATEWAY_PROXY_CONTRACT");

bytes32 bridgeHubAgentID = vm.envBytes32("BRIDGE_HUB_AGENT_ID");
bytes32 assetHubAgentID = vm.envBytes32("ASSET_HUB_AGENT_ID");

address bridgeHubAgent = IGateway(gatewayAddress).agentOf(bridgeHubAgentID);
address assetHubAgent = IGateway(gatewayAddress).agentOf(assetHubAgentID);

payable(bridgeHubAgent).safeNativeTransfer(initialDeposit);
payable(assetHubAgent).safeNativeTransfer(initialDeposit);
payable(gatewayAddress).safeNativeTransfer(initialDeposit);

vm.stopBroadcast();
}
Expand Down
41 changes: 33 additions & 8 deletions contracts/src/Assets.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {CoreStorage} from "./storage/CoreStorage.sol";
import {SubstrateTypes} from "./SubstrateTypes.sol";
import {ParaID, MultiAddress, Ticket, Costs} from "./Types.sol";
import {Address} from "./utils/Address.sol";
import {SafeNativeTransfer} from "./utils/SafeTransfer.sol";
import {AgentExecutor} from "./AgentExecutor.sol";
import {Agent} from "./Agent.sol";
import {Call} from "./utils/Call.sol";
Expand All @@ -21,6 +22,7 @@ import {Token} from "./Token.sol";
/// @title Library for implementing Ethereum->Polkadot ERC20 transfers.
library Assets {
using Address for address;
using SafeNativeTransfer for address payable;
using SafeTokenTransferFrom for IERC20;

/* Errors */
Expand All @@ -34,6 +36,7 @@ library Assets {
error TokenAlreadyRegistered();
error TokenMintFailed();
error TokenTransferFailed();
error TokenAmountTooLow();

function isTokenRegistered(address token) external view returns (bool) {
return AssetsStorage.layout().tokenRegistry[token].isRegistered;
Expand All @@ -60,7 +63,7 @@ library Assets {
) external view returns (Costs memory costs) {
AssetsStorage.Layout storage $ = AssetsStorage.layout();
TokenInfo storage info = $.tokenRegistry[token];
if (!info.isRegistered) {
if (!info.isRegistered && token != address(0)) {
revert TokenNotRegistered();
}

Expand Down Expand Up @@ -110,15 +113,17 @@ library Assets {

TokenInfo storage info = $.tokenRegistry[token];

if (!info.isRegistered) {
revert TokenNotRegistered();
}

if (info.foreignID == bytes32(0)) {
if (!info.isRegistered && token != address(0)) {
revert TokenNotRegistered();
}
return _sendNativeToken(
token, sender, destinationChain, destinationAddress, destinationChainFee, maxDestinationChainFee, amount
);
} else {
if (!info.isRegistered) {
revert TokenNotRegistered();
}
return _sendForeignToken(
info.foreignID,
token,
Expand All @@ -144,7 +149,18 @@ library Assets {
AssetsStorage.Layout storage $ = AssetsStorage.layout();

// Lock the funds into AssetHub's agent contract
_transferToAgent($.assetHubAgent, token, sender, amount);
if (token != address(0)) {
// ERC20
_transferToAgent($.assetHubAgent, token, sender, amount);
ticket.etherAmount = 0;
} else {
// Native ETH
if (msg.value < amount) {
revert TokenAmountTooLow();
}
payable($.assetHubAgent).safeNativeTransfer(amount);
ticket.etherAmount = amount;
}

ticket.dest = $.assetHubParaID;
ticket.costs = _sendTokenCosts(destinationChain, destinationChainFee, maxDestinationChainFee);
Expand Down Expand Up @@ -211,6 +227,7 @@ library Assets {

ticket.dest = $.assetHubParaID;
ticket.costs = _sendTokenCosts(destinationChain, destinationChainFee, maxDestinationChainFee);
ticket.etherAmount = 0;

// Construct a message payload
if (destinationChain == $.assetHubParaID && destinationAddress.isAddress32()) {
Expand Down Expand Up @@ -254,14 +271,15 @@ library Assets {
// It means that registration can be retried.
// But register a PNA here is not allowed
TokenInfo storage info = $.tokenRegistry[token];
if(info.foreignID != bytes32(0)) {
if (info.foreignID != bytes32(0)) {
revert TokenAlreadyRegistered();
}
info.isRegistered = true;

ticket.dest = $.assetHubParaID;
ticket.costs = _registerTokenCosts();
ticket.payload = SubstrateTypes.RegisterToken(token, $.assetHubCreateAssetFee);
ticket.etherAmount = 0;

emit IGateway.TokenRegistrationSent(token);
}
Expand Down Expand Up @@ -293,7 +311,14 @@ library Assets {
function transferNativeToken(address executor, address agent, address token, address recipient, uint128 amount)
external
{
bytes memory call = abi.encodeCall(AgentExecutor.transferToken, (token, recipient, amount));
bytes memory call;
if (token != address(0)) {
// ERC20
call = abi.encodeCall(AgentExecutor.transferToken, (token, recipient, amount));
} else {
// Native ETH
call = abi.encodeCall(AgentExecutor.transferNative, (payable(recipient), amount));
}
(bool success,) = Agent(payable(agent)).invoke(executor, call);
if (!success) {
revert TokenTransferFailed();
Expand Down
22 changes: 11 additions & 11 deletions contracts/src/Gateway.sol
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ contract Gateway is IGateway, IInitializable, IUpgradable {
error InvalidProof();
error InvalidNonce();
error NotEnoughGas();
error FeePaymentToLow();
error FeePaymentTooLow();
error Unauthorized();
error Disabled();
error AgentAlreadyCreated();
Expand All @@ -99,6 +99,7 @@ contract Gateway is IGateway, IInitializable, IUpgradable {
error InvalidAgentExecutionPayload();
error InvalidConstructorParams();
error TokenNotRegistered();
error TokenAmountTooLow();

// Message handlers can only be dispatched by the gateway itself
modifier onlySelf() {
Expand Down Expand Up @@ -240,11 +241,11 @@ contract Gateway is IGateway, IInitializable, IUpgradable {

// Add the reward to the refund amount. If the sum is more than the funds available
// in the channel agent, then reduce the total amount
uint256 amount = Math.min(refund + message.reward, address(channel.agent).balance);
uint256 amount = Math.min(refund + message.reward, address(this).balance);

// Do the payment if there funds available in the agent
// Do the payment if there funds available in the gateway
if (amount > _dustThreshold()) {
_transferNativeFromAgent(channel.agent, payable(msg.sender), amount);
payable(msg.sender).safeNativeTransfer(amount);
}

emit IGateway.InboundMessageDispatched(message.channelID, message.nonce, message.id, success);
Expand Down Expand Up @@ -530,18 +531,17 @@ contract Gateway is IGateway, IInitializable, IUpgradable {
uint256 fee = _calculateFee(ticket.costs);

// Ensure the user has enough funds for this message to be accepted
if (msg.value < fee) {
revert FeePaymentToLow();
uint256 totalEther = fee + ticket.etherAmount;
if (msg.value < totalEther) {
revert FeePaymentTooLow();
}

channel.outboundNonce = channel.outboundNonce + 1;

// Deposit total fee into agent's contract
payable(channel.agent).safeNativeTransfer(fee);

// The fee is already collected into the gateway contract
// Reimburse excess fee payment
if (msg.value > fee) {
payable(msg.sender).safeNativeTransfer(msg.value - fee);
if (msg.value > totalEther) {
payable(msg.sender).safeNativeTransfer(msg.value - totalEther);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this refund process still required since Gateway now is the vault of Ether? I'd suggest to remove it.

Or for security do we need to add some reentrantlock on outbound calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to port the functionality across as is. I changed the code to consider dust like we do in fees so that we do not transfer if the amount is too low. But I think it is good to refund the customer incase of any error in the input, we will not lock and hold more funds than we mint on the other side of the bridge.

Copy link
Contributor

@yrong yrong Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alistair-singh I added a PR #1356 with tests to demonstrate the reentrancy behavior through the refund process.

Adding the ReentrancyGuard did enhance the security somehow. On the other hand, it will also cost a bit more gas, and it seems our bridge can't be exploited in this way. So I'm still not sure if it's necessary.

Please let me know what you think.

Copy link
Contributor Author

@alistair-singh alistair-singh Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! How is ReentracyGuard different from the nonentrancy keyword in solidity 0.28 that we use in Contracts v2?

Copy link
Contributor

@yrong yrong Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


// Generate a unique ID for this message
Expand Down
7 changes: 2 additions & 5 deletions contracts/src/GatewayProxy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ contract GatewayProxy is IInitializable {
}
}

// Prevent users from unwittingly sending ether to the gateway, as these funds
// would otherwise be lost forever.
receive() external payable {
revert NativeCurrencyNotAccepted();
}
alistair-singh marked this conversation as resolved.
Show resolved Hide resolved
// Allow the Gateway proxy to receive ether in order to pay out rewards and refunds
receive() external payable {}
}
1 change: 1 addition & 0 deletions contracts/src/Types.sol
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ struct Ticket {
ParaID dest;
Costs costs;
bytes payload;
uint128 etherAmount;
alistair-singh marked this conversation as resolved.
Show resolved Hide resolved
}

struct TokenInfo {
Expand Down
1 change: 0 additions & 1 deletion contracts/test/Bitfield.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ contract BitfieldTest is Test {

string memory json = vm.readFile(string.concat(vm.projectRoot(), "/test/data/beefy-validator-set.json"));
uint32 setSize = uint32(json.readUint(".validatorSetSize"));
bytes32 root = json.readBytes32(".validatorRoot");
uint256[] memory bitSetArray = json.readUintArray(".participants");

uint256[] memory initialBitField = bw.createBitfield(bitSetArray, setSize);
Expand Down
Loading
Loading