Skip to content

Commit

Permalink
fix(contracts/core): increment l1 deposits by withdraw amount
Browse files Browse the repository at this point in the history
  • Loading branch information
kevinhalliday committed Nov 4, 2024
1 parent b4a803e commit 88a63c9
Show file tree
Hide file tree
Showing 17 changed files with 155 additions and 145 deletions.
2 changes: 1 addition & 1 deletion contracts/allocs/devnet.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion contracts/allocs/staging.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion contracts/bindings/admin.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion contracts/bindings/allocpredeploys.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion contracts/bindings/omnibridgel1.go

Large diffs are not rendered by default.

129 changes: 65 additions & 64 deletions contracts/bindings/omnibridgenative.go

Large diffs are not rendered by default.

38 changes: 19 additions & 19 deletions contracts/core/.gas-snapshot
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
Admin_Test:test_pause_unpause() (gas: 26472157)
Admin_Test:test_pause_unpause_bridge() (gas: 21501844)
Admin_Test:test_pause_unpause_xcall() (gas: 26426087)
Admin_Test:test_pause_unpause_xsubmit() (gas: 26425838)
Admin_Test:test_upgrade() (gas: 30507332)
AllocPredeploys_Test:test_num_allocs() (gas: 1181152549)
AllocPredeploys_Test:test_predeploys() (gas: 1181134337)
AllocPredeploys_Test:test_preinstalls() (gas: 1181850775)
AllocPredeploys_Test:test_proxies() (gas: 1408777576)
Admin_Test:test_pause_unpause() (gas: 26454845)
Admin_Test:test_pause_unpause_bridge() (gas: 21484532)
Admin_Test:test_pause_unpause_xcall() (gas: 26408775)
Admin_Test:test_pause_unpause_xsubmit() (gas: 26408526)
Admin_Test:test_upgrade() (gas: 30490020)
AllocPredeploys_Test:test_num_allocs() (gas: 1181198831)
AllocPredeploys_Test:test_predeploys() (gas: 1181135133)
AllocPredeploys_Test:test_preinstalls() (gas: 1181851571)
AllocPredeploys_Test:test_proxies() (gas: 1408778372)
FeeOracleV1_Test:test_bulkSetFeeParams() (gas: 173154)
FeeOracleV1_Test:test_feeFor() (gas: 122830)
FeeOracleV1_Test:test_setBaseGasLimit() (gas: 32375)
Expand Down Expand Up @@ -48,20 +48,20 @@ Inbox_request_Test:test_request_singleToken() (gas: 426934)
Inbox_request_Test:test_request_two() (gas: 671803)
InitializableHelper_Test:test_disableInitalizers() (gas: 181686)
InitializableHelper_Test:test_getInitialized() (gas: 178023)
OmniBridgeL1_Test:test_bridge() (gas: 233678)
OmniBridgeL1_Test:test_initialize() (gas: 1820095)
OmniBridgeL1_Test:test_bridge() (gas: 228802)
OmniBridgeL1_Test:test_initialize() (gas: 1749710)
OmniBridgeL1_Test:test_pauseAll() (gas: 49376)
OmniBridgeL1_Test:test_pauseBridging() (gas: 59591)
OmniBridgeL1_Test:test_pauseBridging() (gas: 54880)
OmniBridgeL1_Test:test_pauseWithdraws() (gas: 48992)
OmniBridgeL1_Test:test_stub() (gas: 143)
OmniBridgeL1_Test:test_withdraw() (gas: 1386987)
OmniBridgeNative_Test:test_bridge() (gas: 156805)
OmniBridgeNative_Test:test_claim() (gas: 287756)
OmniBridgeNative_Test:test_pauseAll() (gas: 52904)
OmniBridgeNative_Test:test_pauseBridging() (gas: 44449)
OmniBridgeNative_Test:test_pauseWithdraws() (gas: 61348)
OmniBridgeL1_Test:test_withdraw() (gas: 1390996)
OmniBridgeNative_Test:test_bridge() (gas: 156870)
OmniBridgeNative_Test:test_claim() (gas: 287668)
OmniBridgeNative_Test:test_pauseAll() (gas: 52853)
OmniBridgeNative_Test:test_pauseBridging() (gas: 44379)
OmniBridgeNative_Test:test_pauseWithdraws() (gas: 61240)
OmniBridgeNative_Test:test_stub() (gas: 143)
OmniBridgeNative_Test:test_withdraw() (gas: 279800)
OmniBridgeNative_Test:test_withdraw() (gas: 279580)
OmniGasPump_Test:testFuzz_quote(uint32) (runs: 256, μ: 63941, ~: 63991)
OmniGasPump_Test:test_fillUp() (gas: 237060)
OmniGasPump_Test:test_pause() (gas: 62825)
Expand Down
11 changes: 9 additions & 2 deletions contracts/core/script/admin/Admin.s.sol
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,9 @@ contract Admin is Script {
address omni = address(b.omni());
address l1Bridge = b.l1Bridge();
uint64 l1ChainId = b.l1ChainId();
uint256 l1BridgeBalance = b.l1BridgeBalance();
uint256 l1Deposits = WithL1BridgeBalanceView(address(b)).l1BridgeBalance();

require(l1Deposits == 0, "l1Deposits not 0");

vm.startBroadcast(deployer);
address impl = address(new OmniBridgeNative());
Expand All @@ -283,7 +285,7 @@ contract Admin is Script {
require(b.owner() == owner, "owner changed");
require(b.l1ChainId() == l1ChainId, "l1ChainId changed");
require(address(b.omni()) == omni, "omni changed");
require(b.l1BridgeBalance() == l1BridgeBalance, "l1BridgeBalance changed");
require(b.l1Deposits() == l1Deposits, "l1Deposits changed");
require(b.l1Bridge() == l1Bridge, "l1Bridge changed");

new BridgeNativePostUpgradeTest().run();
Expand Down Expand Up @@ -357,3 +359,8 @@ contract Admin is Script {
require(EIP1967Helper.getImplementation(proxy) == impl, "upgrade failed");
}
}

/// @dev Helper interface for native bridge ore l1BridgeBalance -> l1Deposits rename.
interface WithL1BridgeBalanceView {
function l1BridgeBalance() external view returns (uint256);
}
2 changes: 1 addition & 1 deletion contracts/core/script/admin/BridgeL1PostUpgradeTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ contract BridgeL1PostUpgradeTest is Test {
portal.omniChainId(),
ConfLevel.Finalized,
Predeploys.OmniBridgeNative,
abi.encodeCall(OmniBridgeNative.withdraw, (payor, to, amount, bridgeBalance + amount)),
abi.encodeCall(OmniBridgeNative.withdraw, (payor, to, amount)),
b.XCALL_WITHDRAW_GAS_LIMIT()
)
)
Expand Down
12 changes: 6 additions & 6 deletions contracts/core/script/admin/BridgeNativePostUpgradeTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -41,29 +41,30 @@ contract BridgeNativePostUpgradeTest is Test {
l1ChainId = b.l1ChainId();
owner = b.owner();
portal = new MockPortal();
uint256 l1Deposits = b.l1Deposits();

// change portal to mock portal
vm.prank(owner);
b.setup(l1ChainId, address(portal), l1Bridge);
b.setup(l1ChainId, address(portal), l1Bridge, l1Deposits);
}

function _testWithdraw() internal {
address to = makeAddr("to");
uint256 amount = 1e18;
address payor = makeAddr("payor");
uint256 l1BridgeBalance = 100e18;
uint256 l1Deposits = b.l1Deposits();

vm.expectCall(to, amount, "");

portal.mockXCall({
sourceChainId: l1ChainId,
sender: address(l1Bridge),
to: address(b),
data: abi.encodeCall(OmniBridgeNative.withdraw, (payor, to, amount, l1BridgeBalance)),
data: abi.encodeCall(OmniBridgeNative.withdraw, (payor, to, amount)),
gasLimit: 100_000
});

assertEq(b.l1BridgeBalance(), l1BridgeBalance);
assertEq(b.l1Deposits(), l1Deposits + amount);
assertEq(b.claimable(payor), 0);
}

Expand Down Expand Up @@ -94,7 +95,6 @@ contract BridgeNativePostUpgradeTest is Test {
address to = makeAddr("to");
uint256 amount = 1e18;
address payor = makeAddr("payor");
uint256 l1BridgeBalance = 100e18;

// will revert on withdraw
address noReceiver = address(new NoReceive());
Expand All @@ -105,7 +105,7 @@ contract BridgeNativePostUpgradeTest is Test {
sourceChainId: l1ChainId,
sender: address(l1Bridge),
to: address(b),
data: abi.encodeCall(OmniBridgeNative.withdraw, (payor, noReceiver, amount, l1BridgeBalance)),
data: abi.encodeCall(OmniBridgeNative.withdraw, (payor, noReceiver, amount)),
gasLimit: 100_000
});

Expand Down
7 changes: 2 additions & 5 deletions contracts/core/src/token/OmniBridgeL1.sol
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ contract OmniBridgeL1 is OmniBridgeCommon {
require(to != address(0), "OmniBridge: no bridge to zero");

uint64 omniChainId = omni.omniChainId();
bytes memory xcalldata =
abi.encodeCall(OmniBridgeNative.withdraw, (payor, to, amount, token.balanceOf(address(this)) + amount));
bytes memory xcalldata = abi.encodeCall(OmniBridgeNative.withdraw, (payor, to, amount));

require(
msg.value >= omni.feeFor(omniChainId, xcalldata, XCALL_WITHDRAW_GAS_LIMIT), "OmniBridge: insufficient fee"
Expand All @@ -103,9 +102,7 @@ contract OmniBridgeL1 is OmniBridgeCommon {
*/
function bridgeFee(address payor, address to, uint256 amount) public view returns (uint256) {
return omni.feeFor(
omni.omniChainId(),
abi.encodeCall(OmniBridgeNative.withdraw, (payor, to, amount, token.balanceOf(address(this)) + amount)),
XCALL_WITHDRAW_GAS_LIMIT
omni.omniChainId(), abi.encodeCall(OmniBridgeNative.withdraw, (payor, to, amount)), XCALL_WITHDRAW_GAS_LIMIT
);
}
}
29 changes: 14 additions & 15 deletions contracts/core/src/token/OmniBridgeNative.sol
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ contract OmniBridgeNative is OmniBridgeCommon {
/**
* @notice Emitted on setup(...)
*/
event Setup(uint64 l1ChainId, address omni, address l1Bridge);
event Setup(uint64 l1ChainId, address omni, address l1Bridge, uint256 l1Deposits);

/**
* @notice xcall gas limit for OmniBridgeL1.withdraw
Expand All @@ -57,15 +57,15 @@ contract OmniBridgeNative is OmniBridgeCommon {
/**
* @notice Total OMNI tokens deposited to OmniBridgeL1.
*
* If l1BridgeBalance == totalL1Supply, all OMNI tokens are on Omni's EVM.
* If l1BridgeBalance == 0, withdraws to L1 are blocked.
* If l1Deposits == totalL1Supply, all OMNI tokens are on Omni's EVM.
* If l1Deposits == 0, withdraws to L1 are blocked.
*
* Without validator rewards, totalL1Deposits == 0 would mean all OMNI tokens are on Ethereum.
* Without validator rewards, l1Deposits == 0 would mean all OMNI tokens are on Ethereum.
* However with validator rewards, some OMNI may remain on Omni.
*
* This balance is synced on each withdraw to Omni, and decremented on each bridge to back L1.
*/
uint256 public l1BridgeBalance;
uint256 public l1Deposits;

/**
* @notice The address of the OmniBridgeL1 contract deployed to Ethereum.
Expand All @@ -90,19 +90,15 @@ contract OmniBridgeNative is OmniBridgeCommon {
* @param payor The address of the account with OMNI on L1.
* @param to The address to receive the OMNI on Omni.
* @param amount The amount of OMNI to withdraw.
* @param l1Balance The OMNI balance of the L1 bridge contract, synced on each withdraw.
*/
function withdraw(address payor, address to, uint256 amount, uint256 l1Balance)
external
whenNotPaused(ACTION_WITHDRAW)
{
function withdraw(address payor, address to, uint256 amount) external whenNotPaused(ACTION_WITHDRAW) {
XTypes.MsgContext memory xmsg = omni.xmsg();

require(msg.sender == address(omni), "OmniBridge: not xcall"); // this protects against reentrancy
require(xmsg.sender == l1Bridge, "OmniBridge: not bridge");
require(xmsg.sourceChainId == l1ChainId, "OmniBridge: not L1");

l1BridgeBalance = l1Balance;
l1Deposits += amount;

(bool success,) = to.call{ value: amount }("");

Expand All @@ -124,10 +120,10 @@ contract OmniBridgeNative is OmniBridgeCommon {
function _bridge(address to, uint256 amount) internal {
require(to != address(0), "OmniBridge: no bridge to zero");
require(amount > 0, "OmniBridge: amount must be > 0");
require(amount <= l1BridgeBalance, "OmniBridge: no liquidity");
require(amount <= l1Deposits, "OmniBridge: no liquidity");
require(msg.value >= amount + bridgeFee(to, amount), "OmniBridge: insufficient funds");

l1BridgeBalance -= amount;
l1Deposits -= amount;

// if fee is overpaid, forward excess to portal.
// balance of this contract should continue to reflect funds bridged to L1.
Expand Down Expand Up @@ -183,11 +179,14 @@ contract OmniBridgeNative is OmniBridgeCommon {
* @param l1ChainId_ The chain id of the L1 network.
* @param omni_ The address of the OmniPortal contract.
* @param l1Bridge_ The address of the L1 OmniBridge contract.
* @param l1Deposits_ The number of tokens deposied to L1 bridge contract at setup
* (to account for genesis prefunds)
*/
function setup(uint64 l1ChainId_, address omni_, address l1Bridge_) external onlyOwner {
function setup(uint64 l1ChainId_, address omni_, address l1Bridge_, uint256 l1Deposits_) external onlyOwner {
l1ChainId = l1ChainId_;
omni = IOmniPortal(omni_);
l1Bridge = l1Bridge_;
emit Setup(l1ChainId_, omni_, l1Bridge_);
l1Deposits = l1Deposits_;
emit Setup(l1ChainId_, omni_, l1Bridge_, l1Deposits_);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ contract OmniBridgeNativeFixtures is Test {
);

vm.prank(owner);
nativebridge.setup(l1ChainId, address(portal), l1bridge);
nativebridge.setup(l1ChainId, address(portal), l1bridge, 0);
vm.deal(address(nativebridge), totalSupply);
}
}

/// @dev A wrapper around OmniBridgeNative, with public state setters.
contract OmniBridgeNativeHarness is OmniBridgeNative {
function setL1BridgeBalance(uint256 balance) public {
l1BridgeBalance = balance;
function setL1Deposits(uint256 balance) public {
l1Deposits = balance;
}

function setClaimable(address claimant, uint256 amount) public {
Expand Down
2 changes: 1 addition & 1 deletion contracts/core/test/token/OmniBridgeL1.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ contract OmniBridgeL1_Test is Test {
portal.omniChainId(),
ConfLevel.Finalized,
Predeploys.OmniBridgeNative,
abi.encodeCall(OmniBridgeNative.withdraw, (payor, to, amount, token.balanceOf(address(b)) + amount)),
abi.encodeCall(OmniBridgeNative.withdraw, (payor, to, amount)),
b.XCALL_WITHDRAW_GAS_LIMIT()
)
)
Expand Down
Loading

0 comments on commit 88a63c9

Please sign in to comment.