Add polygon adapters/spoke pool #57
Conversation
contracts/Polygon_SpokePool.sol
Outdated
| address _crossDomainAdmin, | ||
| address _hubPool, | ||
| address timerAddress | ||
| ) SpokePool(_crossDomainAdmin, _hubPool, 0x4200000000000000000000000000000000000006, timerAddress) { |
There was a problem hiding this comment.
why do you have this address here? this is not the address of WETH on polygon.
contracts/Polygon_SpokePool.sol
Outdated
| } | ||
|
|
||
| function processMessageFromRoot( | ||
| uint256, |
There was a problem hiding this comment.
perhaps add a comment saying this variable is unused and what it is
contracts/Polygon_SpokePool.sol
Outdated
| **************************************/ | ||
|
|
||
| function _bridgeTokensToHubPool(RelayerRefundLeaf memory relayerRefundLeaf) internal override { | ||
| PolygonIERC20(relayerRefundLeaf.l2TokenAddress).safeIncreaseAllowance(relayerRefundLeaf.amountToReturn); |
There was a problem hiding this comment.
in the Optimism_adapter we use IERC20(l1Token).approve(address(l1StandardBridge), amount);. would it be better to change that to use safeIncreaseAllowance over approve?
There was a problem hiding this comment.
Sure, I can do that.
There was a problem hiding this comment.
I don't really understand how the PolygonTokenBridger will work. Does each user need to deploy their own Bridger?
Ah I see one Bridger is deployed alongside one spoke pool this makes sense. This just means we will need a bot to pull funds to the spoke pool.
contracts/PolygonTokenBridger.sol
Outdated
| // Because Polygon only allows withdrawals from a particular address to go to that same address on mainnet, we need to | ||
| // have some sort of contract that can guarantee identical addresses on Polygon and Ethereum. | ||
| // Note: this contract is intended to be completely immutable, so it's guaranteed that the contract on each side is | ||
| // configured identically as long as it is created via create2. |
There was a problem hiding this comment.
What does create2 actually do? A comment would help here
| } | ||
|
|
||
| function _requireAdminSender() internal view override { | ||
| require(callValidated, "Must call processMessageFromRoot"); |
There was a problem hiding this comment.
This is a clever way to check that calls are only made via the fx tunnel. But could you not replace this call with a require that the msg.sender is the fxChild and remove the callValidated Boolean variable?
There was a problem hiding this comment.
Yes! This was something that I considered. The particular case that I'm worried about (and maybe I shouldn't be) is that the polygon fxChild contract could make other calls into contracts (now or in the future) on behalf of mainnet callers. If 1) those methods happen to be the same name/args as one of our admin methods or 2) (more likely) there's a way to partially or fully customize the selector used (hash of method name + arg types), someone could make a malicious call that comes from that contract but hasn't had its sender validated.
The types of attacks I'm worried about are ones in the style of the poly network hack, where someone is able to use user-inputs to force a contract to call another contract in a way that it wasn't intended to. https://twitter.com/kelvinfichter/status/1425217065690992641?s=20&t=OL5ecSja3ii-LktomykOOg.
I doubt this is possible on polygon today, but their contracts are upgradable and I just figure it's better to be extra careful.
Does that make sense? Thoughts?
There was a problem hiding this comment.
I think that's a reasonable concern and one worth documenting! Two comments:
- I think you should add the
callValidatedflipping on/ff as a modifier for readability - You should add a comment explaining why checking msg.sender == fxChild is not sufficient because of the reasons you explained above
| emit TokensRelayed(l1Token, l2Token, amount, to); | ||
| } | ||
|
|
||
| // Added to enable the Optimism_Adapter to receive ETH. used when unwrapping WETH. |
There was a problem hiding this comment.
Nit: change Optimism to Polygon
nicholaspai
left a comment
nicholaspai
left a comment
There was a problem hiding this comment.
Just left one suggestion for resolving the issue (which may be a non issue) or calling TokenBridger.retrieve
| } | ||
|
|
||
| // Mainnet side. | ||
| function retrieve(IERC20 token) public nonReentrant { |
There was a problem hiding this comment.
It's annoying that we will need a separate bot to execute this method but we could also call it in the executeRelayerRefund and executeSlowRelay methods which are overridable. This would be similar to how OptimismSpokePool works which also has the related problem of receiving ETH over the bridge but needing to have WETH to send to recipients.
There was a problem hiding this comment.
The retrieve method would need to be called on the mainnet side, right? executeRelayerRefund/executeSlowRelay are SpokePool methods, which would only happen on polygon, not mainnet right?
There was a problem hiding this comment.
Good point. I thought this TokenBridger contract is meant to assist with tokens sent from HubPool --> Polygon_SpokePool, and back, so couldn't we use the above strategy to at least automate token retrievals on the Polygon side?
There was a problem hiding this comment.
I don't think we need any assistance on the polygon side. I thought the tokens get deposited/sent automatically, no?
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Wanted to get feedback on this general pattern before writing tests/mocks.