fix[L04] Enforce chainId requirements in PolygonTokenBridger #115
Conversation
nicholaspai
added
the
OZ Audit - March
| // the polygon chainId 137. | ||
| uint256 public immutable l2ChainId; | ||
|
|
||
| modifier onlyChainId(uint256 chainId) { |
There was a problem hiding this comment.
nit instead of using modifier, does calling internal method _requireChainId directly in send and retrieve use less gas? I think its readable either way
There was a problem hiding this comment.
My understanding is that modifiers are usually inlined, so I expect that it would be the same either way.
| const polygonTokenBridger = await ( | ||
| await getContractFactory("PolygonTokenBridger", owner) | ||
| ).deploy(hubPool.address, weth.address); | ||
| ).deploy(hubPool.address, weth.address, chainId, chainId); |
There was a problem hiding this comment.
I think you could test this by deploying new PolygonTokenBridgers with incorrect l1/l2 chainIDs and just testing that send and retrieve revert if the chain IDs are set unexpectedly
There was a problem hiding this comment.
Agree, will add a test for this!
chrismaree
changed the title
| // This should only happen on the mainnet side where ETH is sent to the contract directly by the bridge. | ||
| _requireChainId(l1ChainId); | ||
| l1Weth.deposit{ value: address(this).balance }(); | ||
| } else { |
There was a problem hiding this comment.
more succinct: _requireChainId(functionCallStackOriginatesFromOutsideThisContract() ? l1ChainId : l2ChainId)
There was a problem hiding this comment.
Only reason I don't want to do this is that we still need to do the l1Weth.deposit call in a separate branch, which makes this feel a bit messier since we're doing the same branch twice.
nicholaspai
left a comment
nicholaspai
left a comment
There was a problem hiding this comment.
LGTM with one nit suggestion (feel free to ignore it)
) * improve: Update solc version to 8.13 * feat: Changes after running 8.13 wargames * Update Optimism_Adapter.json * Init commit: arbitrum rinkeby * finish arbitrum war games * fix[N06] Fix misleading comments (#104) * fix[N04] Add additional documentation Signed-off-by: chrismaree <christopher.maree@gmail.com> * nit Signed-off-by: chrismaree <christopher.maree@gmail.com> * fix[N06] Fix missleading comments Signed-off-by: chrismaree <christopher.maree@gmail.com> * Apply suggestions from code review Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com> * nit Signed-off-by: chrismaree <christopher.maree@gmail.com> Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com> * fix[N08] Propose fixes to some naming issues (#105) * fix[N02} Move all structs to the same place Signed-off-by: chrismaree <christopher.maree@gmail.com> * fix[N03] Fixed inconsistant token metadata versioning Signed-off-by: chrismaree <christopher.maree@gmail.com> * nit Signed-off-by: chrismaree <christopher.maree@gmail.com> * nit Signed-off-by: chrismaree <christopher.maree@gmail.com> * fix[N08] Propose fixes to some naming issues Signed-off-by: chrismaree <christopher.maree@gmail.com> * fix[L04] Enforce chainId requirements in PolygonTokenBridger (#115) * fix: Arbitrum Adapter needs to pay for L2 gas * Add helper scripts * Update Arbitrum_Adapter.sol Co-authored-by: Chris Maree <christopher.maree@gmail.com> Co-authored-by: Matt Rice <matthewcrice32@gmail.com>
Problem:
The PolygonTokenBridger contract's primary functions are only intended to be called either on l1 or
l2, but not both. In fact, calling the functions on the wrong chain could result in unexpected behavior
and unnecessary confusion.
In the best case, the functions will simply revert if called from the wrong chain because they will
attempt to interact with other contracts that do not exist on that chain. For example, calling the
receive function (by sending the contract some native asset) could trigger reverts on Polygon, but not
on Ethereum, because there is a WETH contract at the l1Weth address on the latter but not the former.
However, in the worst case, it is possible that such calls will not revert, but result in lost funds instead.
For example, if a WETH-like contract was later deployed to the l1Weth address on Polygon, then the
call would not revert. Instead, tokens would be sent to that contract and could remain stuck there.
Although the inline documentation details which function should be called on which chain, consider
having the functions in this contract actively enforce these requirements via limiting execution to the
correct block.chainid.
Solution
Add l1 and l2 chain ids to the polygon bridger, so create2 still guarantees matching addresses on both chains, but certain methods can be disabled depending on the env.