ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

The upgrading process will revert when initializing with new Owner

alqaqa

Vulnerability Details

When upgrading UUPS upgradable contracts, the Proxy contract owner calls upgradeToAndCall() to change its implementation. And fire the initializing function of the new implementation in his context via delegate call.

When doing this thing in Bridge, we can see that it takes these variables as the initializing inputs.

function initialize(bytes calldata data) public onlyInit {
(
❌️ address owner,
IStarknetMessaging starknetCoreAddress,
uint256 starklaneL2Address,
uint256 starklaneL2Selector
) = abi.decode(
data,
(address, IStarknetMessaging, uint256, uint256)
);
_enabled = false;
_starknetCoreAddress = starknetCoreAddress;
​
❌️ _transferOwnership(owner);
​
setStarklaneL2Address(starklaneL2Address);
setStarklaneL2Selector(starklaneL2Selector);
}

As we can see we are taking the owner address as an input when upgrading our Bridge contract to the new implementation, gives him the ownership of the contract, and then sets StarklaneL2Address and Selector, and this is the problem.

The problem here is that when calling setStarklaneL2Address there is a modifier onlyOwner.

State.sol#L42-L49

function setStarklaneL2Address(
uint256 l2Address
)
public
❌️ onlyOwner
{
_starklaneL2Address = Cairo.snaddressWrap(l2Address);
}

Since we are using UUPS/ERC1967 upgradable proxy standard, we are calling _authorizeUpgrade(), to authorize the upgrading and we only let the owner to be able to upgrade in our contract (Bridge). So the msg.sender will be the owner of the contract.

UUPSProxied.sol#L31-L37

function _authorizeUpgrade(
address
)
internal
override
❌️ onlyOwner
{ }

Since it is allowed to change the owner when upgrading the contract as we illustrated, the upgrading process will end up reverting because of onlyOwner check when calling setStarklaneL2Address() as the owner is not the msg.sender now.

This will end up failing the upgrading process at the last check, which is not actual behavior. Leading to the failure of the upgradability process and the loss money paid as the Gas Cost for deploying.

Impact

  • Failing of the upgradability process.

  • Lossing the gas cost paid for the upgrading process.

Tools Used

Manual review

Recommendations

Change the ownership in the last of the Bridge::initialize() function.

diff --git a/apps/blockchain/ethereum/src/Bridge.sol b/apps/blockchain/ethereum/src/Bridge.sol
index e62c7ce..3a68209 100644
--- a/apps/blockchain/ethereum/src/Bridge.sol
+++ b/apps/blockchain/ethereum/src/Bridge.sol
@@ -59,10 +59,10 @@ contract Starklane is IStarklaneEvent, UUPSOwnableProxied, StarklaneState, Stark
_enabled = false;
_starknetCoreAddress = starknetCoreAddress;
- _transferOwnership(owner);
-
setStarklaneL2Address(starklaneL2Address);
setStarklaneL2Selector(starklaneL2Selector);
+
+ _transferOwnership(owner);
}
Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid-initialize-owner-has-to-be-msg-sender

No real impact. It even prevents to set an invalid owner. Future versions/upgrades are out-of-scope since this function can/will change to do not modify the owner at every upgrades.

Appeal created

alqaqa Submitter
9 months ago
n0kto Lead Judge
8 months ago
n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid-initialize-owner-has-to-be-msg-sender

No real impact. It even prevents to set an invalid owner. Future versions/upgrades are out-of-scope since this function can/will change to do not modify the owner at every upgrades.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.