ArkProject: NFT Bridge

Summary

L1 bridge can be taken over by an attacker if upgradeTo() is used to upgrade the contract's implementation. This is because of how initialize() is implemented in Bridge.sol

Vulnerability Details

The initialize() function in Bridge.sol is as follows :

It has the onlyInit modifier :

The problem with this logic is that the initialize() function needs to be called again when the contract is upgraded to a new implementation. Not only is this unnecessary but it also opens the contract to the possibility of the bridge being taken over by an attacker if the contract is upgraded using the upgradeTo() function of UUPSUpgradeable.sol

When the contract is upgraded by the owner using the upgradeTo() function, the contract can be initialized again since _initializedImpls[impl] will return false in the onlyInit modifier. Thus, an attacker can back-run the owners upgradeTo() transaction or front-run the owner's initialize() transaction to call initialize(), thus setting theirself as the owner of the contract and therefore taking over any assets that the bridge posseses.

Impact

Permanent loss of assets in given scenario. However, the owner can use upgradeToAndCall() instead to mitigate the possibility of this attack. Therefore, the attack requires the bridge owner to be unaware of this little detail.

Since the impact is High and the likelihood of the owner lacking knowledge of this vulnerability is Low - Medium, I believe Medium severity would be appropriate for this issue.

Tools Used

Manual Review

Recommendations

Calling the initializer every time the contract is upgraded not only makes the contract vulnerable to this attack, but also is completely uneccessary since the owner can independently alter all of the variables that are set in this function anyways.

Instead, you can replace the _initializedImpls[] mapping with the simple bool initialized variable to only allow the function to be called once.