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

Attacker can successfully steal tokens deposited in `Escrow.sol` by manipulating `address(this)`

Baptizer

Summary

The usual flow is that only true token owners that deposited into escrow.solshould be able to withdraw their respective tokens. However, an attacker can hijack this withdrawal by manipulating address(this). Thus, an attacker can maliciously steal all the NFTs deposited into the contract.

Vulnerability Details

Here is the withdrawal function:

function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
​
address from = address(this);
​
if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id);
} else {
// TODO:
// Check here if the token supply is currently 0.
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
​
_escrow[collection][id] = address(0x0);
​
return true;
}

The Vulnerable part of this function is address from = address(this). Note that in the usual ERC721 format, address fromis the address that will transfer NFTs. This line assigns the power of address fromto address(this).

The devs wrote this line with a presupposition that the person who deposited will be the address(this)both at the point of deposit and the point of withdrawal. However, this can be broken.

After a user deposits, an attacker can masquerade as the address(this)and successfully transfer the tokens.

Impact

This vulnerability impacts users by allowing attackers to successfully withdraw what they never deposited. This vulnerability is so critical that attackers can withdraw ALL the tokens users deposited.

This loss is in two ways:

  • innocent users losing their tokens

  • the protocol losing community trust and probably run out of business as no one will want to use it

Proof of Concept

  • Alice, a user, deposits token 1 into the escrow

  • Attacker resets address(this)to a wallet he owns

  • Attacker sends token 1from his first wallet into his second wallet

  • Alice has lost token 1and can no longer withdraw it from the escrow

Proof of Code

To show that this exploit is an actual severe security risk, here is a proof of 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/Escrow.sol";
import "./utils/Users.sol";
import "./token/ERC721MintFree.sol";
import "./token/IERC721MintRangeFree.sol";
contract EscrowTest is Test {
​
EscrowPublic escrow;
address erc721;
​
address payable internal alice;
address payable hacker2ndWallet; // this is the address they will successfully send the stolen tokens to
​
function setUp() public {
escrow = new EscrowPublic();
erc721 = address(new ERC721MintFree("name 1", "S1"));
​
Users genusers = new Users();
address payable[] memory users = genusers.create(5);
alice = users[0];
hacker2ndWallet = users[1];
​
vm.prank(alice);
IERC721(erc721).setApprovalForAll(address(escrow), true);
vm.prank(hacker2ndWallet);
IERC721(erc721).setApprovalForAll(address(escrow), true);
}
​
//
function test_hackWithdraw() public {
IERC721MintRangeFree(erc721).mintRangeFree(alice, 0, 10);
​
uint256[] memory ids = new uint256[](2);
ids[0] = 5;
ids[1] = 8;
​
vm.startPrank(alice);
​
escrow.depositIntoEscrow(CollectionType.ERC721, erc721, ids);
​
vm.stopPrank();
address hacker = address(this); // this is where an attacker can reset "address(this)" to their own address.
​
vm.startPrank(hacker); // attacker trying to withdraw what they never deposited
​
bool wasInEscrow = escrow.withdrawFromEscrow(CollectionType.ERC721, erc721, hacker2ndWallet, 5);
assertTrue(wasInEscrow);
vm.stopPrank();
​
assertEq(IERC721(erc721).ownerOf(5), hacker2ndWallet);
}
​
​
​
}
​
/**
@title Escrow interface exposed for test.
*/
contract EscrowPublic is StarklaneEscrow {
​
/**
@notice test _depositIntoEscrow.
*/
function depositIntoEscrow(
CollectionType collectionType,
address collection,
uint256[] memory ids
)
public
{
_depositIntoEscrow(collectionType, collection, ids);
}
​
/**
@notice test _withdrawFromEscrow.
*/
function withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
public
returns (bool)
{
return _withdrawFromEscrow(collectionType, collection, to, id);
}
}

The attacker in the test above reset address(this)to one of his wallet addresses - address hacker = address(this). Then, he successfully transferred the token to his hacker2ndWallet.

Here is the result of the test in Foundry:

Ran 1 test for test/Escrow.t.sol:EscrowTest
[PASS] test_hackWithdraw() (gas: 414477)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.20ms (1.13ms CPU time)
​
Ran 1 test suite in 33.98ms (4.20ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

It passed!

Tools Used

  • Manual review

  • Foundry

Recommendations

It is better to set address from as an argument within the withdrawal function (and other interdependent functions as necessary).

Such that users can set their addresses before calling the function. This way, address(this)cannot be switched mid-way.

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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