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

Initializing the Starklane contract with the owner being different from the caller will revert the initialization

draiakoo

Summary

Initializing the Starklane contract with the owner being different from the caller will revert the initialization

Vulnerability Details

When the Starklane is initialized it is needed to pass in some data. In this data there is encoded the owner that will be set to the contract, the Starknet core address and the starknet bridge contract and the selector of the function to call when transfering messages. Apparently, it seems that the caller can set the owner to any address he wants. However, it does not work that way because when it will execute either setStarklaneL2Address or setStarklaneL2Selector will check that the msg.sender is the previously set owner.

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);
​
// @audit-issue if the owner set previously != msg.sender the init function will fail
setStarklaneL2Address(starklaneL2Address);
setStarklaneL2Selector(starklaneL2Selector);
}
​
function setStarklaneL2Address(
uint256 l2Address
)
public
onlyOwner
{
_starklaneL2Address = Cairo.snaddressWrap(l2Address);
}
​
function setStarklaneL2Selector(
uint256 l2Selector
)
public
onlyOwner
{
_starklaneL2Selector = Cairo.felt252Wrap(l2Selector);
}

Hence if the owner passed in the data is a different address from the caller of the initialize function it will revert.

Impact

The impact of this issue is not severe but it still forces that the owner has to be the caller of the initialize function.

Tools Used

Manual review

Recommendations

  • If the intended behaviour is that the owner HAS to be the caller of the initialize function then it makes no sense to pass the owner in the data, it is as easy as set the owner to the msg.sender.

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

  • If the intended behaviour is that it should be possible to set the owner to an address that is not the caller, the following code should be changed.

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);
+ _starklaneL2Address = Cairo.snaddressWrap(starklaneL2Address);
+ _starklaneL2Selector = Cairo.felt252Wrap(starklaneL2Selector);
}
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

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

Give us feedback!