it is possible that the wrapper in setupAsset() is not set, this will cause the setup to malfunction for native assets

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/AssetFacet.sol#L143

Vulnerability details

Issue: there is a possibility for the wrapper in setupAsset() to not be set. This is medium because there are many interactions that rely on wrapper being set correctly which might cause malfunction in the protocol.

Consequences: this will cause the setup to malfunction for native assets due to the fact that the wrapper will point to address(0).

Affected Code

File: AssetFacet.sol
132:   function setupAsset(
133:     ConnextMessage.TokenId calldata _canonical,
134:     address _adoptedAssetId,
135:     address _stableSwapPool 
136:   ) external onlyOwner {
137:     // Sanity check: needs approval
138:     if (s.approvedAssets[_canonical.id]) revert AssetFacet__addAssetId_alreadyAdded();
139: 
140:     // Update approved assets mapping
141:     s.approvedAssets[_canonical.id] = true;
142: 
143:     address supported = _adoptedAssetId == address(0) ? address(s.wrapper) : _adoptedAssetId; // @audit-info [MED] 
144: 
145:     // Update the adopted mapping
146:     s.adoptedToCanonical[supported].domain = _canonical.domain;
147:     s.adoptedToCanonical[supported].id = _canonical.id;
148: 
149:     // Update the canonical mapping
150:     s.canonicalToAdopted[_canonical.id] = supported;
151: 
152:     // Emit event
153:     emit AssetAdded(_canonical.id, _canonical.domain, _adoptedAssetId, supported, msg.sender);
154: 
155:     // Add the swap pool
156:     _addStableSwapPool(_canonical, _stableSwapPool);
157:   }

Recommended Mitigation Steps

Consider adding a check for the wrapper to not be address(0) when _adoptedAssetId is address(0).

[jakekidd]

Resolved: connext/monorepo@fec801d

[0xleastwood]

I disagree with the severity of this issue. While this would potentially be safer, I think a better check would be to check this in DiamondInit.init instead and avoid any repeated checks in setupAsset. Even then, this finding assumes that the contracts were incorrectly setup in the first place. I think this is rather unlikely and seems to fit the criteria of QA better.

[0xleastwood]

Merging with #263.