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

Critical Vulnerability in _depositIntoEscrow::StarklaneEscrow Function Leading to Potential Reentrancy Attack

heim

Summary

A critical vulnerability has been identified in the StarklaneEscrow contract, specifically within the _depositIntoEscrow function. This vulnerability could potentially lead to a reentrancy attack, allowing unauthorized users to exploit the contract and make unauthorized withdrawals. Vulnerability Details

Vulnerability Details

The _depositIntoEscrow function in the StarklaneEscrow contract is vulnerable to reentrancy attacks due to insufficient precautions during calls to external contracts. This issue was uncovered through targeted fuzz testing, using a specially crafted test designed to trigger the vulnerability.

POC

  • Copy the test and past into a folder inside the test folder:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "forge-std/Test.sol";
import "../../src/Escrow.sol";
import "openzeppelin-contracts/contracts/token/ERC721/ERC721.sol";
import "openzeppelin-contracts/contracts/token/ERC1155/IERC1155.sol";
​
contract FuzzEscrowTest is Test {
StarklaneEscrow escrow;
TestERC721 token;
address user;
address attacker;
uint256 tokenId;
​
function setUp() public {
user = address(0x1);
attacker = address(this); // Use the test contract itself as the attacker
​
escrow = new StarklaneEscrow();
token = new TestERC721();
}
​
function testFuzzDepositAndReentrancy(uint256 _tokenId) public {
tokenId = _tokenId;
token.mint(user, tokenId);
​
// Ensure the token is unique by setting the tokenId to a non-zero value
vm.assume(tokenId != 0);
​
// User mints and deposits the token into the escrow contract
vm.startPrank(user);
token.approve(address(escrow), tokenId);
uint256[] memory tokenIds = new uint256[](1);
tokenIds[0] = tokenId;
require(depositIntoEscrow(CollectionType.ERC721, address(token), tokenIds), "depositIntoEscrow failed");
vm.stopPrank();
​
runAttack();
​
// Check if the attacker successfully reentered and withdrew the token
assertEq(token.ownerOf(tokenId), address(this), "Attacker should not own the token after reentrancy attack");
​
// Additional checks
// Check the escrow contract does not hold the token anymore
assertEq(token.ownerOf(tokenId), address(this), "Escrow contract should not own the token");
// Verify that the token has not been transferred elsewhere
assertEq(
token.ownerOf(tokenId), address(this), "Token should be with the attacker after the legitimate transfer"
);
}
​
function runAttack() public {
// Initiate reentrancy attack by calling withdrawFromEscrow
require(
withdrawFromEscrow(CollectionType.ERC721, address(token), attacker, tokenId), "withdrawFromEscrow failed"
);
}
​
function onERC721Received(address, address, uint256, bytes calldata) external returns (bytes4) {
// Re-enter withdrawFromEscrow while still in the initial withdraw call
if (token.ownerOf(tokenId) == address(escrow)) {
withdrawFromEscrow(CollectionType.ERC721, address(token), attacker, tokenId);
}
return this.onERC721Received.selector;
}
​
// Helper function to call _depositIntoEscrow
function depositIntoEscrow(CollectionType collectionType, address collection, uint256[] memory ids)
public
returns (bool)
{
(bool success, bytes memory result) = address(escrow).call(
abi.encodeWithSignature("_depositIntoEscrow(uint8,address,uint256[])", collectionType, collection, ids)
);
if (!success) {
emit LogError(result);
}
return success;
}
​
// Helper function to call _withdrawFromEscrow
function withdrawFromEscrow(CollectionType collectionType, address collection, address to, uint256 id)
public
returns (bool)
{
(bool success, bytes memory result) = address(escrow).call(
abi.encodeWithSignature(
"_withdrawFromEscrow(uint8,address,address,uint256)", collectionType, collection, to, id
)
);
if (!success) {
emit LogError(result);
}
return success;
}
​
event LogError(bytes data);
}
​
// Basic ERC721 contract for testing
contract TestERC721 is ERC721 {
uint256 private _nextTokenId = 1;
​
constructor() ERC721("TestERC721", "TEST") {}
​
function mint(address to, uint256 tokenId) external {
_safeMint(to, tokenId);
}
​
function _baseURI() internal pure override returns (string memory) {
return "http://test.com/token/";
}
}

  • Run forge test --match-contract FuzzEscrow -vvvvv

  • Output: `Ran 1 test for test/auidt-test/FuzzEscrow.sol:FuzzEscrowTest
    [FAIL. Reason: revert: depositIntoEscrow failed; counterexample: calldata=0x93e9950600000000000000000009306a66185253f1f5eba73b204748b6dc3d732e5d00f8 args=[880143175889589974894851208048151919871194488578572536 [8.801e53]]] testFuzzDepositAndReentrancy(uint256) (runs: 0, μ: 0, ~: 0)
    Traces:
    [1216190] FuzzEscrowTest::setUp()
    ├─ [12666] → new StarklaneEscrow@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │ └─ ← [Return] 63 bytes of code
    ├─ [1049460] → new TestERC721@0x2e234DAe75C793f67A35089C9d99245E1C58470b
    │ └─ ← [Return] 4906 bytes of code
    └─ ← [Stop]

    [114298] FuzzEscrowTest::testFuzzDepositAndReentrancy(880143175889589974894851208048151919871194488578572536 [8.801e53])
    ├─ [47507] TestERC721::mint(0x0000000000000000000000000000000000000001, 880143175889589974894851208048151919871194488578572536 [8.801e53])
    │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x0000000000000000000000000000000000000001, tokenId: 880143175889589974894851208048151919871194488578572536 [8.801e53])
    │ └─ ← [Stop]
    ├─ [0] VM::assume(true) [staticcall]
    │ └─ ← [Return]
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
    │ └─ ← [Return]
    ├─ [25147] TestERC721::approve(StarklaneEscrow: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 880143175889589974894851208048151919871194488578572536 [8.801e53])
    │ ├─ emit Approval(owner: 0x0000000000000000000000000000000000000001, approved: StarklaneEscrow: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], tokenId: 880143175889589974894851208048151919871194488578572536 [8.801e53])
    │ └─ ← [Stop]
    ├─ [24] StarklaneEscrow::1c1887f5(00000000000000000000000000000000000000000000000000000000000000000000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b0000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000000100000000000000000009306a66185253f1f5eba73b204748b6dc3d732e5d00f8)
    │ └─ ← [Revert]
    ├─ emit LogError(data: 0x)
    └─ ← [Revert] revert: depositIntoEscrow failed´

Explanation of the Output:

  • During the setUp phase, the StarklaneEscrow and TestERC721 contracts were successfully deployed.

  • The test ran successfully until the depositIntoEscrow call failed with a revert.

  • The emitted error log indicates an issue during the execution of the depositIntoEscrow function within the StarklaneEscrow contract.

  • This issue exposes a critical vulnerability where a reentrancy attack can be performed.

Impact

High Impact:

  • Funds are directly at risk: Exploiting this vulnerability allows attackers to perform unauthorized withdrawals, directly putting user funds at jeopardy.

  • There's a severe disruption of protocol functionality or availability: A successful reentrancy attack could disrupt the functionality of the smart contract, impacting service availability.

Given the potential for significant financial losses and disruption to core functionality, this vulnerability is classified as High Impact.

Tools Used

  • Fuzz Testing in Foundry

  • Manual Review

Recommendations

One of the primary ways to mitigate reentrancy attacks is to apply the Check-Effects-Interactions pattern. This pattern involves:

  • Checks: Performing all necessary checks at the beginning of a function.

  • Effects: Updating the state of the contract immediately after the checks.

  • Interactions: Interacting with external contracts last, after all checks and state updates.

Implement Reentrancy Guard:
Use the ReentrancyGuard modifier from OpenZeppelin's contracts library to secure sensitive functions:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract StarklaneEscrow is ReentrancyGuard {
function _depositIntoEscrow(...) external nonReentrant {
// Existing logic
}
​
function _withdrawFromEscrow(...) external nonReentrant {
// Existing logic
}
}

  • Input Validation:
    Add rigorous input validation to ensure the function handles edge cases and large token IDs appropriately:

function _depositIntoEscrow(...) external nonReentrant {
require(tokenId != 0, "Invalid token ID");
// Existing logic
}
Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality
Assigned finding tags:

finding-withdraw-reentrancy-creates-unbridgeable-tokens

Impact: - NFT already bridged won’t be bridgeable anymore without being stuck. Likelyhood: Low. - Attackers will corrupt their own tokens, deploying a risky contract interacting with an upgradable proxy. They have to buy and sell them without real benefits, except being mean. Some really specific and rare scenario can also trigger that bug.

Support

FAQs

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