teleportr: add TeleportrDeposit and TeleportrDisburser contracts by cfromknecht · Pull Request #2145 · ethereum-optimism/optimism

cfromknecht · 2022-02-04T23:31:53Z

Description
This PR adds two new contracts, L1/teleportr/TeleportrDeposit.sol and L2/teleportr/TeleportrDisburser.sol, that will form the core of the new teleportr service. The TeleportrDeposit contract is modeled off the existing BridgeDeposit.sol contract, but adds the ability to set a minDepositAmount and emit a depositId for each EtherReceived event to uniquely identify them in our postgres instance. The TeleportrDisburser contract implements the L2 disbursement side of the bridge, which supports batched disbursements and also tracks the deposit ID's to make implementation of the teleportr driver simpler.

Some questions I have:

Is the canReceive modified necessary? Or can we just emulate this by setting maxDepositAmount = 0.
What is the appropriate gas limit to set on the each disbursement receiver's call? Should it be adjustable by the owner?

I will hold off on updating the docs until we have some rough consensus on the design :)

Metadata

Fixes ENG-1931

changeset-bot · 2022-02-04T23:31:55Z

⚠️ No Changeset found

Latest commit: c7ca41a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

tynes · 2022-02-05T00:08:06Z

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

+    receive()
+        external
+        payable
+        isLowerThanMaxDepositAmount


It feels like a bit of an over optimization to have this many modifiers and composing them all together, perhaps we can move the functionality directly into the function itself or combine them into a single modifier?

Agree, esp. since they are only used once, it's more readable to inline the checks.

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

tynes · 2022-02-05T00:11:37Z

Link to original contract for those who are interested: https://github.com/0xclem/teleportr/blob/main/contracts/BridgeDeposit.sol
Perhaps we should include a link in the contract code itself as a shoutout?

maurelian · 2022-02-05T01:40:51Z

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

+    {
+        uint256 _depositId = totalDeposits + 1;
+        emit EtherReceived(_depositId, msg.sender, msg.value);
+        totalDeposits = _depositId;
+    }


The following is cleaner IMO

Suggested change

{

uint256 _depositId = totalDeposits + 1;

emit EtherReceived(_depositId, msg.sender, msg.value);

totalDeposits = _depositId;

}

{

totalDeposits += 1;

emit EtherReceived(totalDeposits, msg.sender, msg.value);

}

Sounds good, changed. For my own curiosity, does this approach not use an extra SLOAD?

If we were using foundry it would be really easy to take a gas snapshot and see, would need to look at the execution trace or bytecode to know for sure

I'd support using foundry if we put this in it's own package (which I think we should do anyways).

foundry works in this package with forge test --hardhat

Oh, amazing!

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

maurelian · 2022-02-05T02:00:21Z

packages/contracts/contracts/L2/teleportr/TeleportrDisbursement.sol

+    modifier onlyOwner() {
+        require(msg.sender == owner, "only accessible to owner");
+        _;
+    }


Also needs setOwner() and getOwner() methods like the other contract.

made owner public and added setOwner 👍

maurelian · 2022-02-05T02:01:35Z

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

+        emit OwnerSet(address(0), msg.sender);
+        emit MaxDepositAmountSet(0, _maxDepositAmount);
+        emit MinDepositAmountSet(0, _minDepositAmount);
+        emit MaxBalanceSet(0, _maxBalance);
+        emit CanReceiveDepositSet(_canReceiveDeposit);


Do we really need all these events? Is anyone actually watching for them?

It is a lot of events but it does follow best practices re: allowing an offchain actor to observe all events and recompute the state of the contract

I removed the events, but if we want to add them back as per @tynes comment I can do that

Honestly I think @tynes makes a good point.
At least we can remove CanReceiveDepositSet now.

This contract has a backdoor in it where all the money can be drained so we should make it transparent in its operations. We should definitely have an event for withdrawBalance - it could help with ensuring security of the key we are using

maurelian · 2022-02-05T02:05:03Z

packages/contracts/contracts/L2/teleportr/TeleportrDisbursement.sol

+    uint256 public totalDisbursements;
+
+    constructor() {
+        owner = msg.sender;


We probably don't want the actual owner to be the deployer key. If we add a means of transferring ownership, then this might be OK.

Same comment goes for the other constructor.

Sounds good, I will check out how our other contracts do this today

packages/contracts/contracts/L2/teleportr/TeleportrDisbursement.sol

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

maurelian

See inline comments, some are blocking.

Additional considerations:

Auth: I think we need to properly spec how we want to handle authorization. The approach here (onlyOwner) is different from most of our other contracts which use the AddressManager contract. That might be fine, but we should think about how we want to handle it.
Upgradability: Do we need it here? Probably not, we can just replace the contracts.
Gas to forward: The description asks some good questions:

What` is the appropriate gas limit to set on the each disbursement receiver's call?

2300 seems like the default/standard, I'll ask around if that's changed.

Should it be adjustable by the owner?

IMO no, if it's not enough we can just replace the contract, because we want to minimize SLOADs on that path.

Organization: I think there's a good argument to be made that this could be in a separate package. It has no deps with the current contract package.
Testing, deployment scripts, and NatSpec comments We should have some of those IMO.

tynes · 2022-02-05T22:01:14Z

Regarding upgradability, I don't think we want to place this behind a proxy to save the sload + delegatecall but I do think its a good idea to put an entry in the address manager for this contract so that offchain actors can always first look up the address of the teleporter contract. Or we could name it using ens

maurelian · 2022-02-10T20:27:26Z

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

+    event EtherReceived(
+        uint256 indexed depositId,
+        address indexed emitter,
+        uint256 amount


We can save about 150 gas by adding the indexed kw to this last arg (source).

maurelian · 2022-02-10T20:41:46Z

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

+    modifier isOwner() {
+        require(msg.sender == libAddressManager.owner(), "Caller is not owner");
+        _;
+    }


Note that libAddressManager.owner() is a very sensitive address that we should only access for upgrades or occasionally modifying parameters.

I'd suggest:

use the isOwner modifier on the setter functions

create a different role (withdrawer), modifier (isWithdrawer) and setter function (setWithdrawer() isOwner) for the withdrawBalance() call. The address for this role can then be used more often in a hot wallet (as I assume we'll want to drain this contract on a semi-regular basis).

maurelian · 2022-02-10T20:42:41Z

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

+        address _owner = libAddressManager.owner();
+        uint256 _balance = address(this).balance;
+        emit BalanceWithdrawn(_owner, _balance);
+        payable(_owner).transfer(_balance);


Further to the comment on the isOwner() modifier above, we may or may not want to send withdrawals payments to this address.

This is a direct copy of the BridgeDeposit.sol contract apart from the name change, some code moves to make the linter happy, and promoting various public methods to external to satisfy slither.

codecov-commenter · 2022-02-19T22:48:14Z

Codecov Report

Merging #2145 (c7ca41a) into develop (67a0414) will decrease coverage by 0.30%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2145      +/-   ##
===========================================
- Coverage    73.35%   73.04%   -0.31%     
===========================================
  Files           84       86       +2     
  Lines         2770     2846      +76     
  Branches       474      486      +12     
===========================================
+ Hits          2032     2079      +47     
- Misses         738      767      +29

Flag	Coverage Δ
batch-submitter	`62.63% <ø> (ø)`
contracts	`90.88% <100.00%> (+0.39%)`	⬆️
core-utils	`87.80% <ø> (ø)`
data-transport-layer	`36.89% <ø> (ø)`
sdk	`57.71% <ø> (-2.34%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...tracts/contracts/L1/teleportr/TeleportrDeposit.sol	`100.00% <100.00%> (ø)`
...acts/contracts/L2/teleportr/TeleportrDisburser.sol	`100.00% <100.00%> (ø)`
packages/sdk/src/adapters/standard-bridge.ts	`23.95% <0.00%> (-4.44%)`	⬇️
packages/sdk/src/adapters/eth-bridge.ts	`50.00% <0.00%> (-3.85%)`	⬇️
packages/sdk/src/cross-chain-messenger.ts	`69.91% <0.00%> (-2.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67a0414...c7ca41a. Read the comment docs.

maurelian · 2022-02-20T18:47:33Z

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

+        require(msg.value >= minDepositAmount, "Deposit amount is too small");
+        require(msg.value <= maxDepositAmount, "Deposit amount is too big");
+        require(address(this).balance <= maxBalance, "Contract max balance exceeded");


Nice, this is much more readable than modifiers IMO.

maurelian · 2022-02-22T14:17:15Z

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol

+        minDepositAmount = _minDepositAmount;
+        maxDepositAmount = _maxDepositAmount;
+        maxBalance = _maxBalance;
+        totalDeposits = 0;


FYI this is a noop. State vars are initialized to zero:

The concept of “undefined” or “null” values does not exist in Solidity, but newly declared variables always have a default value dependent on its type.

Source: Types — Solidity 0.8.13 documentation

maurelian

LGTM.
Just left a couple comments to inform, but nothing is blocking.

Security considerations taken into account during my review:

Possibility of draining funds:
I verified that all functions that modify key state vars use onlyOwner for auth. The only public function is receive().
Stuck funds:
It is possible to have funds not paid out in the disbursement function, which would be left in the contract, but there are manual work around for this.

Ref #2136 --------- Co-authored-by: Arun Dhyani <dhyaniarun7@gmail.com>

github-actions bot added 2-reviewers labels Feb 4, 2022

cfromknecht marked this pull request as ready for review February 4, 2022 23:32

tynes reviewed Feb 4, 2022

View reviewed changes

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol Outdated Show resolved Hide resolved

tynes reviewed Feb 5, 2022

View reviewed changes

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol Outdated Show resolved Hide resolved

maurelian reviewed Feb 5, 2022

View reviewed changes

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol Outdated Show resolved Hide resolved

maurelian reviewed Feb 5, 2022

View reviewed changes

packages/contracts/contracts/L2/teleportr/TeleportrDisbursement.sol Outdated Show resolved Hide resolved

maurelian reviewed Feb 5, 2022

View reviewed changes

packages/contracts/contracts/L2/teleportr/TeleportrDisbursement.sol Outdated Show resolved Hide resolved

maurelian reviewed Feb 5, 2022

View reviewed changes

packages/contracts/contracts/L1/teleportr/TeleportrDeposit.sol Outdated Show resolved Hide resolved

maurelian reviewed Feb 5, 2022

View reviewed changes

cfromknecht force-pushed the teleportr-contracts branch 2 times, most recently from bbd27c4 to ea9f9f0 Compare February 7, 2022 18:45

cfromknecht changed the title ~~teleportr: add TeleportrDeposit and TeleportrDisbursement contracts~~ teleportr: add TeleportrDeposit and TeleportrDisburser contracts Feb 7, 2022

cfromknecht force-pushed the teleportr-contracts branch 4 times, most recently from 5e6b4f7 to faee154 Compare February 9, 2022 01:45

maurelian reviewed Feb 10, 2022

View reviewed changes

cfromknecht force-pushed the teleportr-contracts branch 3 times, most recently from a8db6be to c458879 Compare February 18, 2022 17:48

feat: add base TeleportrDeposit.sol contract

80dde9f

This is a direct copy of the BridgeDeposit.sol contract apart from the name change, some code moves to make the linter happy, and promoting various public methods to external to satisfy slither.

cfromknecht added 9 commits February 18, 2022 11:36

feat: make contract variables public, remove getters

9222ed1

feat: remove TeleportDeposit destory method

fdc3006

feat: inline/simplify receive modifiers

60a04f3

feat: add minDepositAmount to TeleportrDeposit.sol

99532fd

feat: emit depositId in TeleportrDeposit.EtherReceived event

3bba6e1

feat: make Teleportr contract inherit from Ownable

451064a

feat: add NatSpec comments and tests to TeleportrDeposit

f639979

feat: add L2 TeleportrDisburser contract

509348f

feat: add missing Initializable contract docs

42c9be9

cfromknecht force-pushed the teleportr-contracts branch from c458879 to 42c9be9 Compare February 18, 2022 18:36

mslipper added 2 commits February 19, 2022 13:44

Fix slither warnings

f123c52

Remove .only

c7ca41a

maurelian reviewed Feb 20, 2022

View reviewed changes

maurelian reviewed Feb 22, 2022

View reviewed changes

maurelian approved these changes Feb 22, 2022

View reviewed changes

mslipper merged commit 1ca7df7 into ethereum-optimism:develop Feb 22, 2022

cfromknecht deleted the teleportr-contracts branch February 22, 2022 18:17

theochap pushed a commit that referenced this pull request Dec 10, 2025

fix(supervisor/rpc): localUnsafe always points to genesis (#2145)

fd23751

Ref #2136 --------- Co-authored-by: Arun Dhyani <dhyaniarun7@gmail.com>

Conversation

cfromknecht commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tynes commented Feb 5, 2022

Uh oh!

maurelian Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurelian Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maurelian left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tynes commented Feb 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurelian Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurelian Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurelian left a comment

Choose a reason for hiding this comment

cfromknecht commented Feb 4, 2022 •

edited

Loading

changeset-bot bot commented Feb 4, 2022 •

edited

Loading

maurelian Feb 5, 2022 •

edited

Loading

maurelian Feb 10, 2022 •

edited

Loading

maurelian left a comment •

edited

Loading

maurelian Feb 10, 2022 •

edited

Loading

maurelian Feb 10, 2022 •

edited

Loading

codecov-commenter commented Feb 19, 2022 •

edited

Loading