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

Contract can be re-initialized by malicious party if the underlying implementation get upgraded / changed.

asefewwexa

Title

Contract can be re-initialized by malicious party if the underlying implementation get upgraded / changed.

Line of code

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L30

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/UUPSProxied.sol#L20

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L368

Vulnerability Details

The contract Starklane inherits from Openzeppelin's UUPSOwnableProxied and the initialize() function is guarded by the modifier onlyInit()

contract Starklane is IStarklaneEvent, UUPSOwnableProxied, StarklaneState, StarklaneEscrow, StarklaneMessaging, CollectionManager {
​
// Mapping (collectionAddress => bool)
mapping(address => bool) _whiteList;
address[] _collections;
bool _enabled;
bool _whiteListEnabled;
​
​
/**
@notice Initializes the implementation, only callable once.
​
@param data Data to init the implementation.
*/
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, if the implementation changes when the contract is upgraded, malicious user can recall init to reset the owner address and take control over the bridge and cause massive loss of fund.

// onlyInit
modifier onlyInit() {
address impl = _getImplementation();
require(!_initializedImpls[impl], "Already init");
_initializedImpls[impl] = true;
​
_;
}

Impact

As the attacker sets his address as the owner. Malicious attacker can set worthless l2 nft to valuable l1 nft and bridge l2 nft to l1 nft to unlock the l1 nft using the function Starklane::setL1L2CollectionMapping

function setL1L2CollectionMapping(
address collectionL1,
snaddress collectionL2,
bool force
) external onlyOwner {
_setL1L2AddressMapping(collectionL1, collectionL2, force);
emit L1L2CollectionMappingUpdated(collectionL1, snaddress.unwrap(collectionL2));
}

Tools Used

Manual Review

Recommendations

Access control should be implemented in initialize() function.

Updates

Lead Judging Commences

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

invalid-bridge-initialize-frontrun

If frontrun at the first deployment, protocol will deploy again, no real impact: informational. Moreover it is already deployed and initialize on mainnet. For the upgrades, `initialize` can/will change for the next update since the owner is already set. A lot of protocol make that change. That’s why I consider it like a future feature and it is out of scope.

Support

FAQs

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