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

Multiple off-by-one errors in the bridge

bauchibred

Summary

The bridge contract contains multiple off-by-one errors affecting token deposits and transfers. These errors prevent valid Starknet addresses from receiving tokens and incorrectly reject maximum payload lengths, potentially breaking bridging functionality for certain users and transactions.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L78-L144

function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
//@audit
if (!Cairo.isFelt252(snaddress.unwrap(ownerL2))) {
revert CairoWrapError();
}
if (!_enabled) {
revert BridgeNotEnabledError();
}
​
//snip
emit DepositRequestInitiated(req.hash, block.timestamp, payload);
}

This function is used to deposit tokens in the escrow and initiates the
transfer to Starknet, issue here however is the fact that a valid ownerL2 address might not be able to receive these tokens on the Starknet chain, this is because the isFelt252 check is non-inclusive, i.e https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/sn/Cairo.sol#L51-L60

function isFelt252(
uint256 val
)
internal
pure
returns (bool)
{
return val < SN_MODULUS;
}
​

But SN_MODULUS is defined as https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/sn/Cairo.sol#L14-L21

/*
Modulus used for computation in Starknet. This is the maximum value.
https://github.com/starkware-libs/cairo/blob/08efd7158bf1ef012fa6b42b3429a398e1a75e26/crates/cairo-lang-runner/src/casm_run/mod.rs#L52
2 251 + 17 2 192 + 1;
*/
uint256 constant SN_MODULUS =
3618502788666131213697322783095070105623107215331596699973092056135872020481;

Per the comment we can see that this edge value: 3618502788666131213697322783095070105623107215331596699973092056135872020481 is also to be considered a felt32, however since the check is non inclusive an address that matches this would always revert.

Now there also exist the check here while depositing in the bridge, i.e https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L134-L136

if (payload.length >= MAX_PAYLOAD_LENGTH) {
revert TooManyTokensError();
}

Problem with this is the fact that the payload.length should be allowed to be = MAX_PAYLOAD_LENGTH since that't the max accepted, but currently it always reverts when that's the case.

Coded POCs

Attach the tests below here

function test_depositTokensWithMaxAcceptedFelt252ShouldFail() public {
uint256[] memory ids = new uint256[](1);
ids[0] = 1;
uint256 salt = 0x1;
snaddress to = snaddress.wrap(SN_MODULUS);
vm.expectRevert(CairoWrapError.selector);
IStarklane(bridge).depositTokens{value: 30000}(
salt,
address(erc721C1),
to,
ids,
false
);
}
function test_depositTokensWithMaxAcceptedPayloadLengthWouldFail() public {
IERC721MintRangeFree(erc721C1).mintRangeFree(alice, 0, MAX_PAYLOAD_LENGTH);
uint256[] memory ids = new uint256[](MAX_PAYLOAD_LENGTH);
for (uint256 i = 0; i < MAX_PAYLOAD_LENGTH; i++) {
ids[i] = i;
}
uint256 salt = 0x1;
snaddress to = Cairo.snaddressWrap(0x1);
vm.startPrank(alice);
IERC721(erc721C1).setApprovalForAll(address(bridge), true);
vm.expectRevert(TooManyTokensError.selector);
IStarklane(bridge).depositTokens{value: 30000}(
salt,
address(erc721C1),
to,
ids,
false
);
vm.stopPrank();
}

Impact

The bridging functionality would be permanently broken for said valid felt32 user.

For the second case, valid payload lengths would be assumed as invalid and the attempt at bridging would be broken.

Tools Used

Manual review

Recommendations

Consider applying these changes:

function isFelt252(
uint256 val
)
internal
pure
returns (bool)
{
- return val < SN_MODULUS;
+ return val <= SN_MODULUS;
}
​

Also

- if (payload.length >= MAX_PAYLOAD_LENGTH) {
+ if (payload.length > MAX_PAYLOAD_LENGTH) {
revert TooManyTokensError();
}
Updates

Lead Judging Commences

n0kto Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
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.