The emergencyWithdraw() and withdrawFromPosition() functions allow the owner of the related NFT token or approved user to withdraw the entire amount of deposited assets. In that scenarios, both functions internally call the _destroyPosition() function, which burns the intended NFT and attempts to set the ownerToId mapping of user to 0.

function _destroyPosition(uint256 tokenId, uint256 boostPoints) internal {
    // calls yieldBooster contract to deallocate the bspNFT's owner boost points if any
    if (boostPoints > 0) {
        IYieldBooster(yieldBooster()).deallocateAllFromPool(
            msg.sender,
            tokenId
        );
    }

    // burn bspNFT
    delete stakingPositions[tokenId];
    _burn(tokenId);
    ownerToId[ownerOf(tokenId)] = 0;
}

However, the ownerOf() function, implemented in the ERC721 contract, invokes the requireOwned() function, which will revert with an ERC721NonexistentToken error if the token has already been burnt, effectively preventing user from withdrawing the full amount of deposited assets.

function _requireOwned(uint256 tokenId) internal view returns (address) {
    address owner = _ownerOf(tokenId);
    if (owner == address(0)) {
        revert ERC721NonexistentToken(tokenId);
    }
    return owner;
}

The following Foundry test was used to prove the aforementioned issue:

function test_fullWithdrawBroken() public {
    //Admin adds the pool to the MasterBorpa
    masterBorpa.add(address(nftPool), 2);

    //User creates a position, becoming the owner of NFT 1
    vm.startPrank(user);
    mockERC20.approve(address(nftPool), 10e18);
    nftPool.createPosition(10e18, user);
    assertEq(nftPool.ownerToId(user), 1);
    vm.stopPrank();

    //User intends to withdraw all assets
    //Transaction reverts with ERC721NonexistentToken error
    vm.prank(user);
    vm.expectRevert();
    nftPool.emergencyWithdraw(1);

    vm.prank(user);
    vm.expectRevert();
    nftPool.withdrawFromPosition(1, 10e18);
}

Update the ownerToId mapping before burning the NFT token.

Remediation Plan

SOLVED: The Entangle team solved this issue as recommended.

The deallocate() function of the xBorpa contract deallocates a sender's amount of xBorpa to the specified YieldBooster contract. It first calls an internal _deallocate() function and then calls the deallocate() function on the YieldBooster contract. Notably, this function is protected by the nonReentrant modifier.

function deallocate(
    address usageAddress,
    uint256 amount,
    bytes calldata usageData
) external nonReentrant {
    _deallocate(msg.sender, usageAddress, amount);
    IYieldBooster(usageAddress).deallocate(msg.sender, amount, usageData);
}

The deallocate() in YieldBooster contract is responsible for deallocating boost points and calling unboost() on the NFT pool if the non-fungible token exists.

function deallocate(address userAddress, uint256 amount, bytes calldata data) external nonReentrant onlyXBorpa {
    (address poolAddress, uint256 tokenId) = abi.decode(data, (address, uint256));
    _deallocate(userAddress, poolAddress, tokenId, amount);

    // should only be called if bspNFT has not been burned, to avoid having stuck xBorpa on it
    if (INFTPool(poolAddress).exists(tokenId)) {
        // allocated xBorpa is removed (as boost points) from the bspNFT
        INFTPool(poolAddress).unboost(tokenId, amount);
    }
}

The unboost() function removes a specified amount of boost points from a position. It calls _harvestPosition(), which converts Borpa to xBorpa tokens if the calculated xBorpaRewards is greater than zero.

function unboost(
    uint256 tokenId,
    uint256 amount
) external nonReentrant {
    _requireOnlyYieldBooster();

    _updatePool();
    address nftOwner = ownerOf(tokenId);
    _harvestPosition(tokenId, nftOwner, nftOwner);

    StakingPosition storage position = stakingPositions[tokenId];

    // update position
    uint256 boostPoints = position.boostPoints - amount;
    position.boostPoints = boostPoints;
    _updateBoostMultiplierInfoAndRewardDebt(position);
    emit SetBoost(tokenId, boostPoints);
}

if (xBorpaRewards > 0) {
    xBorpaRewards = _safeConvertTo(xBorpaRec, xBorpaRewards);
}

The _safeConvertTo() calls the the convertTo() function from the xBorpa contract which uses a nonReentrant modifier.

function convert(uint256 amount) external nonReentrant {
    _convert(amount, msg.sender);
}

The convertTo() function's nonReentrant modifier causes the entire transaction to revert because the entry point of this flow (the initial deallocate() function) also has this modifier. Therefore, when convertTo() is called, the nonReentrant guard triggers, leading to a reentrancy protection violation and the subsequent revert of the transaction.

The following Foundry test was used in order to prove the aforementioned issue:

function test_brokenDeallocation() public {
    deal(address(borpa), user, 10e18, true);

    //User converts borpa tokens to xBorpa
    vm.startPrank(user);
    borpa.approve(address(xBorpa), 10e18);
    xBorpa.convert(10e18);
    vm.stopPrank();

    //User allocates xBorpa tokens in the choosen pool
    vm.startPrank(user);
    xBorpa.approveUsage(address(yieldBooster), 9e18);
    bytes memory usageData = abi.encode(address(mockPool), 0);
    xBorpa.allocate(address(yieldBooster), 9e18, usageData);
    vm.stopPrank();

    //User intends to deallocate their xBorpa tokens
    vm.startPrank(user);
    vm.expectRevert(ReentrancyGuard.ReentrancyGuardReentrantCall.selector);
    xBorpa.deallocate(address(yieldBooster), 9e18, usageData);
}

Remove the nonReentrant modifier from the safeConvertTo() function from xBorpaToken.

Remediation Plan

SOLVED: The Entangle team solved this issue. The mentioned modifier was replaced with onlyRole(SYSTEM).

When users want to participate in the lottery, they need to burn their Borpa tokens. This is handled by the burn() function of the BorpaToken contract. It calls the creditEntries() function from the ChestManager contract to record the user's lottery entries.

function burn(uint256 _amount) external {
    totalLotteryBurnedAmount = totalLotteryBurnedAmount + _amount;
    _burn(msg.sender, _amount);
    bBalances[msg.sender] = bBalances[msg.sender] + _amount;
    IChestManager(chestManager).creditEntries(msg.sender, _amount);
}

The creditEntries() function in the ChestManager contract handles adding the user’s entries into the current chest. It determines where the new entries should start based on the previous one, creates a range for the new entries, and stores them in the chest’s information.

function creditEntries(address user, uint256 amount) external onlyBorpa {
    uint256 len = entriesLenghtInChest(currentChest);

    uint256 from;
    if (len > 0)
        from = chestsInfo[currentChest].entries[len - 1].data.to + 1;
    else from = 1;

    uint256 to = from + amount - 1;
    EntryData memory entryData = EntryData(from, to);
    CommonEntry memory entry = CommonEntry(entryData, user);

    chestsInfo[currentChest].entries.push(entry);
    chestsInfo[currentChest].playerEntries[user].push(entryData);

    emit CreditEntries(user, amount, from, to);
}

An admin can set the winning ticket for a specific chest using the setWinningTicket() function. This function calculates the winning ticket by taking the modulo of a provided _winningTicket with the end range value of last entry. However, the vulnerability arises due to how totalEntriesRange() calculates the total range of entries.

function setWinningTicket(
    uint256 chestId,
    bytes32 _winningTicket
) external onlyRole(DEFAULT_ADMIN_ROLE) {
    uint256 totalEntries = totalEntriesRange(chestId);
    chestsInfo[chestId].winningTicket =  (uint256(_winningTicket) % totalEntries) + 1;
}

The function incorrectly references currentChest instead of the id provided as a parameter. This results in an attempt to access entries[len - 1] of the currentChest rather than the intended chest id, causing an array out-of-bounds error when the length of entries in currentChest does not correspond to the id's entries. Additionally, even if the length of entries does allow setting the winning ticket without reverting, the totalEntriesRange() function will return an improper value. This improper value can lead to the winning ticket being outside the actual valid range for the intended chest, resulting in a situation where no valid entries match the winning ticket. Consequently, nobody would be able to claim the granted rewards.

The following Foundry test was used to prove the aforementioned issue:

function test_improperChestReference() public{
    uint256 currentChest = manager.currentChest();
    assertEq(currentChest, 0);

    //Users burnt Borpa Token 4 times
    uint256 totalEntries = manager.entriesLenghtInChest(0);
    assertEq(totalEntries, 4);

    //Total amount range in chest is 100e18
    uint256 totalRange = manager.totalEntriesRange(0);
    assertEq(totalRange, 100e18);

    //Babburuzu deposits dispatched fees, equal to current phase threshold
    vm.prank(admin);
    manager.setDepositor(address(this));
    vm.stopPrank();
    usdc.transfer(address(manager), 30000e6);
    manager.deposit(30000e6);

    //New chest has been created
    currentChest = manager.currentChest();
    assertEq(currentChest, 1);

    //Admin attempts to set the winning ticket for the elapsed chest
    //Transaction reverts due to array out-of-bounds access
    bytes32 wte = bytes32(uint256(1));
    vm.expectRevert();
    vm.prank(admin);
    manager.setWinningTicket(0, wte);

    //Users burnt Borpa Token 4 times
    vm.prank(user1);
    borpa.burn(10e18);
    vm.prank(user2);
    borpa.burn(30e18);
    vm.prank(user3);
    borpa.burn(35e18);
    vm.prank(user1);
    borpa.burn(50e18);

    //Admin attempts to set the winning ticket for the elapsed chest
    wte = bytes32(uint256(1));
    vm.prank(admin);
    manager.setWinningTicket(0, wte);

    //Total amount range in chest is 125e18
    totalRange = manager.totalEntriesRange(0);
    assertEq(totalRange, 125e18);
}

The totalEntriesRange() function should be corrected to reference the id parameter provided to it, ensuring it calculates the range based on the specified chest rather than the currentChest.

Remediation Plan

SOLVED: The Entangle team solved this issue as recommended.

During the security assessment of the NFTPool contract, it was identified that the createPosition() function allows any address to initiate a staking position, leading to the unintended minting of NFTPool tokens. This issue arises because there is currently no access control mechanism in place, permitting unauthorized parties to call this function and generate NFTPool tokens at will. The absence of proper authorization introduces a significant security risk, potentially impacting the integrity and value of the NFTPool ecosystem.

function createPosition(uint256 amount, address receiver) external nonReentrant {
  _updatePool();

  if (amount == 0) {
    revert NFTPool__E4();
  }
  amount = _transferThere(cdt, msg.sender, amount);

Given that the rest of the functions in the contract include an authorization mechanism ensuring that only the Gateway contract or the owner of the referenced NFT can call them, it was confirmed with the Entangle team that createPosition() should indeed be protected to ensure only the Gateway contract can invoke it.

The following Foundry test was used to prove the aforementioned issue:

function testcreatePosition() public {
    vm.expectRevert(MasterBorpa.MasterBorpa__E3.selector);
    nftPool.createPosition(0, address(this));

    // Needs to add the pool to the master borpa
    masterBorpa.add(address(nftPool), 100);

    vm.expectRevert(NFTPool.NFTPool__E4.selector);
    nftPool.createPosition(0, address(this));

    mockERC20.approve(address(nftPool), 1);
    nftPool.createPosition(1, address(this));
}

To run it, just execute the following forge command:

forge test  --mt testcreatePosition -vvvvvv

Observe that the test allows anyone with CDT (mockERC20 in the example) to call createPosition():

...
  [499215] NFTPoolTest::testcreatePosition()
...
    ├─ [241328] NFTPool::createPosition(1, NFTPoolTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   ├─ [1234] MasterBorpa::claimRewards()
    │   │   └─ ← [Return] 0
    │   ├─ emit PoolUpdated(lastRewardBlock: 1, accRewardsPerShare: 0)
    │   ├─ [30220] ERC20Mintable::transferFrom(NFTPoolTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], NFTPool: [0xA3CE162A39eb1954b41B6aeac19C4198D99AaB4D], 1)
    │   │   ├─ emit Transfer(from: NFTPoolTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: NFTPool: [0xA3CE162A39eb1954b41B6aeac19C4198D99AaB4D], value: 1)
    │   │   └─ ← [Return] true
    │   ├─ [530] ERC20Mintable::balanceOf(NFTPool: [0xA3CE162A39eb1954b41B6aeac19C4198D99AaB4D]) [staticcall]
    │   │   └─ ← [Return] 1
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: NFTPoolTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], tokenId: 1)
    │   ├─ [406] NFTPoolTest::onERC721Received(NFTPoolTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x0000000000000000000000000000000000000000, 1, 0x)
    │   │   └─ ← [Return] 0x150b7a0200000000000000000000000000000000000000000000000000000000
    │   ├─ emit CreatePosition(tokenId: 1, amount: 1)
    │   └─ ← [Stop] 
    └─ ← [Return] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.29ms (546.25µs CPU time)

Ran 1 test suite in 922.05ms (8.29ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

To mitigate this issue effectively, it is strongly advised to implement an authorization mechanism within the createPosition() function. This mechanism should restrict access so that only authorized contracts, specifically the Gateway contract, can invoke createPosition().

Remediation Plan

SOLVED: The Entangle team solved this issue as recommended.

During the analysis of the NFTPool contract, a mapping named ownerToId was discovered, which was intended to associate each address with a single NFT token, as indicated by the comment:

The NFTPool contract contained a mapping called ownerToId to keep track of the owner of each NFT token with the following comment indicating that any address could have one and only one NFT token:

/// @dev Mapping from owner to tokenId, since everyone should have only one position
mapping(address => uint256 id) public ownerToId;

Despite this intention, there was no corresponding logic within the contract's codebase to enforce this restriction. As the contract inherits from OpenZeppelin's ERC721 library, which inherently supports token transferability unless explicitly disabled, any address could freely transfer their NFTPool tokens. This unrestricted transferability allowed users to accumulate more than one NFT token, thus violating the precondition defined by Entangle that each address should possess only one NFT token.

It's important to note that while transferring the NFT tokens changes the ownership of the ERC721 token itself, the ownerToId mapping within the NFTPool contract was not updated accordingly. This discrepancy further undermines the intended single-token ownership model, as the mapping could potentially reflect incorrect ownership information after token transfers.

The following Foundry test was used to prove the aforementioned issue:

// NFTPool owners should not have more than one NFTPool token
// Result: alice has 2 NFTPool tokens
function testNFTPoolTransfers() public {
    // Needs to add the pool to the master borpa
    masterBorpa.add(address(nftPool), 100);

    // Create the first position
    mockERC20.approve(address(nftPool), 1);
    nftPool.createPosition(1, address(this));

    // Create the second position
    mockERC20.approve(address(nftPool), 1);
    nftPool.createPosition(1, alice);

    // Transfer the first NFTPool token to Alice
    nftPool.safeTransferFrom(address(this), alice, 1);

    assertEq(nftPool.balanceOf(alice), 2);
}

To run it, just execute the following forge command:

forge test --mt testNFTPoolTransfers -vvvvvv

Observe that at the end of the test, the user alice has a balance of 2 NFTPool tokens:

...
 [604920] NFTPoolTest::testNFTPoolTransfers()
...
    ├─ [935] NFTPool::balanceOf(0xEB4665750b1382DF4AeBF49E04B429AAAc4d9929) [staticcall]
    │   └─ ← [Return] 2
    ├─ [0] VM::assertEq(2, 2) [staticcall]
    │   └─ ← [Return] 
    └─ ← [Return] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.77ms (174.17µs CPU time)

Ran 1 test suite in 729.45ms (2.77ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

To address this vulnerability and align with the expected behavior, it is recommended to implement additional logic within the NFTPool contract. This logic should enforce the restriction that each address can own only one NFT token at any given time. This can be achieved by introducing checks during token transfer operations to ensure that a recipient address does not already own an NFT token before allowing the transfer to proceed. Additionally, if feasible within the project's requirements, consider disabling the transferability of NFTPool tokens altogether to prevent unintended token ownership changes.

Remediation Plan

SOLVED: The Entangle team solved this issue by only allowing the NFT transfers to address(0).

The unzap() function of the BorpaGateway contract performs a swap with the wrong assumption that the DEX router has enough allowance of tokenToSwap. Therefore, this would effectivelly prevent anyone from using such functionallity. See the unzap() function below trying to call swapExactTokensForTokensSupportingFeeOnTransferTokens() with amountForSwap.

function unzap(
    uint256 withdrawAmount,
    address token0,
    address token1,
    uint128 sid
) external {
  if (token0 != borpa && token1 != borpa) {
    revert BorpaGateway__E2();
  }
  quit(withdrawAmount, token0, token1, sid, true);

  address tokenToSwap;
  if (token0 == borpa) {
    tokenToSwap = token1;
  } else {
    tokenToSwap = token0;
  }

  uint256 amountForSwap = IERC20(tokenToSwap).balanceOf(address(this));

  address[] memory path = new address[](2);
  path[0] = tokenToSwap;
  path[1] = borpa;

  IBorpaRouter(dexRouter)
    .swapExactTokensForTokensSupportingFeeOnTransferTokens(
      amountForSwap,
      0,
      path,
      _msgSender(),
      block.timestamp
    );
...

The following Foundry test was used to prove the aforementioned issue:

function testUnzapNeedsApprove() public {
    uint128 sid = uint128(1);

    // Start by doing a valid zap()
    testZapValid();

    emit log_named_decimal_uint("User BORPA balance before UNZAP", borpa.balanceOf(address(this)), 18);
    emit log_named_decimal_uint("User WETH balance before UNZAP", IERC20(WETH).balanceOf(address(this)), 18);

    // Should fail due to lack of allowance
    vm.expectRevert("TransferHelper: TRANSFER_FROM_FAILED");
    gateway.unzap(1, address(borpa), WETH, sid);

    emit log_named_decimal_uint("User BORPA balance after UNZAP", borpa.balanceOf(address(this)), 18);
    emit log_named_decimal_uint("User WETH balance after UNZAP", IERC20(WETH).balanceOf(address(this)), 18);

    vm.startPrank(address(gateway));
    IERC20(WETH).approve(gateway.dexRouter(), 1 ether);
    vm.stopPrank();
    
    // Should work now
    gateway.unzap(1, address(borpa), WETH, sid);
}

To run it, just execute the following forge command:

forge test --mt testUnzapNeedsApprove -vv --fork-url https://rpc.ankr.com/eth

Observe that the test passes because:

1. The first unzap() call fails due to the lack of approval.

2. After impersonating the BorpaGateway contract and approving the DEX router, the unzap() call successfully works.

Consider approving the dexRouter for transferring tokenToSwap tokens in the name of BorpaGateway before performing the swap in the unzap() function.

Remediation Plan

SOLVED: The Entangle team solved this issue as recommended.

When the Babburuzu contract dispatches fees, the deposit() function from the ChestManager contract is called to handle the incoming funds.

function deposit(uint256 amount) onlyDepositor external {
  uint256 currentChestMem = currentChest;
  ChestInfo storage info = chestsInfo[currentChestMem]; 
  if (amount + info.currentBalance >= info.openingThreshold) {
    info.currentBalance = info.openingThreshold;
    createChest(currentChestMem + 1);
  } else {
    info.currentBalance += amount;
  }
  emit Deposit(amount);
}

If the dispatched reward exceeds the opening threshold of the current chest, the excess amount does not get transferred to the next chest. Instead, the current chest’s balance is capped at its opening threshold, and a new chest is created for subsequent deposits. As a result, any amount exceeding the current chest's threshold is essentially ignored and not included in the next chest.

The following Foundry test was used in order to prove the aforementioned issue:

function test_excessFundsIgnored() public {
    //The opening value of chest 0 is set to 30000 USDC
    (,, uint256 opening,,) = manager.chestsInfo(0);
    assertEq(opening, 30000e6);

    //Simulate deposits for 29999 USDC
    vm.prank(admin);
    manager.setDepositor(address(this));
    vm.stopPrank();
    IERC20(USDC).transfer(address(manager), 29999e6);
    manager.deposit(29999e6);

    uint256 chest = manager.currentChest();
    assertEq(chest, 0);

    //Babburuzu dispatches 100 USDC fees to the ChestManager
    IERC20(USDC).transfer(address(manager), 100e6);
    manager.deposit(100e6);

    (,,, uint256 currentBalance0,) = manager.chestsInfo(0);
    (,,, uint256 currentBalance1,) = manager.chestsInfo(1);

    //Current balance of chest 0 equals to the opening value
    assertEq(currentBalance0, 30000e6);

    //Current balance of chest 1 equals to the initial value calculated as a feesForNextChest
    assertEq(currentBalance1, 9000e6);

    //Admin grabs a chest
    vm.prank(admin);
    manager.grabChestTreasury(0);

    //The USDC balance after is greater than registered chest balance
    uint256 balanceAfter = IERC20(USDC).balanceOf(address(manager));
    (,,, uint256 chestBalance,) = manager.chestsInfo(1);
    assertGt(balanceAfter, chestBalance);
}

Modify the mentioned flow to allocate any excess amount exceeding the current chest's threshold to the next chest, ensuring that no funds are ignored.

Remediation Plan

SOLVED: The Entangle team solved this issue.

The enter(), quit() and unzap() functions of the BorpaGateway contract are responsible for providing and removing user’s liquidity to UniswapV2 pool or doing intended swaps, however they set 0 as a minimum acceptable amount.

(, , uint256 lpAmount) = IBorpaRouter(dexRouter).addLiquidity( 
    token0,
    token1,
    amount0AfterFee,
    amount1AfterFee,
    0, 
    0,
    address(this),
    block.timestamp
);

IBorpaRouter(dexRouter).removeLiquidity(token0, token1, lpBalance, 0, 0, rec, block.timestamp);

IBorpaRouter(dexRouter)
    .swapExactTokensForTokensSupportingFeeOnTransferTokens(
        amountForSwap,
        0,
        path,
        _msgSender(),
        block.timestamp
    );

Similarly, the _sellTokenFor() function implemented in Babburuzu contract, used in buyback mechanism, internally calls swapExactETHForTokensSupportingFeeOnTransferTokens() or swapExactTokensForTokensSupportingFeeOnTransferTokens() function without any slippage protection.

if (tokenIn == WETH) {
    IBorpaRouter(router)
        .swapExactETHForTokensSupportingFeeOnTransferTokens{
        value: amount
    }(0, paths[WETH][tokenOut], address(this), block.timestamp);
} else if (tokenOut == WETH) {
    IBorpaRouter(router)
        .swapExactTokensForETHSupportingFeeOnTransferTokens(
            amount,
            0,
            paths[tokenIn][WETH],
            address(this),
            block.timestamp
        );
} else { 
    IBorpaRouter(router)
        .swapExactTokensForTokensSupportingFeeOnTransferTokens(
            amount,
            0,
            paths[tokenIn][tokenOut],
            address(this),
            block.timestamp
        );
}

Without specifying slippage tolerance, these functions are susceptible to front-running attacks and unexpected losses due to unfavorable token exchange rates. The minimum acceptable amount parameters are hardcoded to zero in these calls. These parameters are intended to serve as minimum amount protections for the assets being withdrawn. deposited or swapped on the liquidity pool, acting as safeguards against excessive slippage. By setting these minimums to zero, the function fails to enforce any slippage protection, resulting in a potential vulnerability where transactions could be front-run, leading to significantly less tokens as the operation output.

A minimum amount parameter should be provided as an argument in the enter and quite functions. In the case of _sellTokenFor() function, consider implementing a mechanism to retrieve the required price from an off-chain oracle. Use this price, reduced by a tolerable value, to define the minimum amount.

Remediation Plan

PARTIALLY SOLVED: The Entangle team added a slippage protection in the enter(), zap(), and _unzap() functions implemented in BorpaGateway and accepted the risk for Babburuzu contract.

As indicated in the Transfer NFTPool Tokens Allow Users To Have More Than One NFT finding, the NFTPool contract allows token holders to transfer NFTs between each other. This does not only allow addresses to have more than one token, but it was observed that the mapping in charge of keeping track of token ownership (ownerToId) was not being updated upon transfers.

If NFTPool token transfers should be allowed, it would be necessary to override the OpenZeppelin's ERC721 library functions in charge of such transfers so they properly update the ownerToId.

The following Foundry test was used in order to prove the aforementioned issue:

// ownerToId should be updated after transferring a NFTPool token
// Fail: ownerToId IS NOT UPDATED
function testFailownerToIdTransferedNFT() public {
    // Needs to add the pool to the master borpa
    masterBorpa.add(address(nftPool), 100);

    mockERC20.approve(address(nftPool), 1);
    nftPool.createPosition(1, address(this));
    assertEq(nftPool.ownerToId(address(this)), 1);
    assertEq(nftPool.ownerToId(attacker), 0);

    nftPool.safeTransferFrom(address(this), attacker, 1);

    mockERC20.transfer(attacker, 1);

    assertEq(nftPool.ownerToId(address(this)), 0);
    assertEq(nftPool.ownerToId(attacker), 1);
}

To run it, just execute the following forge command:

forge test --mt testFailownerToIdTransferedNFT -vvvvvv

Observe that the test passes because the last two assertions fail. This is because after transferring the token ID 1, the ownerToId mapping is outdated and does not reflect the actual owner:

...
 [517713] NFTPoolTest::testFailownerToIdTransferedNFT()
...
    ├─ [0] VM::assertEq(1, 0) [staticcall]
    │   └─ ← [Revert] assertion failed: 1 != 0
    └─ ← [Revert] assertion failed: 1 != 0

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.43ms (554.88µs CPU time)

Ran 1 test suite in 928.72ms (8.43ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

To address this vulnerability and align with the expected behavior, it is recommended to implement additional logic within the NFTPool contract to update the ownerToId upon transfers or disable the transferability of NFTPool tokens altogether to prevent unintended token ownership changes.

Remediation Plan

SOLVED: The Entangle team solved this issue by only allowing the NFT transfers to address(0).

The zap() function of the BorpaGateway contract contains a path argument that allows users to use more than 2 elements. However, while the _zap() function inside uses the whole path argument (accepting any length), the enter() function inside it, will only use the first and second elements:

function zap(uint256 amountIn, uint128 sid, address[] calldata path) external {
  (uint256 amount0, uint256 amount1) = _zap(amountIn, path);
  enter(
    path[0],
    path[1],
    amount0,
    amount1,
    sid,
    true
    );
}

Notice that the internal function _zap() does not verify the length nor takes only the first and second elements:

function _zap(
    uint256 amountIn,
    address[] calldata path
) internal returns (uint256, uint256) {
    if (path[0] != borpa) {
        revert BorpaGateway__E2();
    }
    IERC20(borpa).safeTransferFrom(_msgSender(), address(this), amountIn);
    uint256 toSwap = deflateAmount(amountIn) / 2;
    IERC20(borpa).approve(dexRouter, toSwap);

    IBorpaRouter(dexRouter)
        .swapExactTokensForTokensSupportingFeeOnTransferTokens(
            toSwap,
            0,
            path,
            address(this),
            block.timestamp
        );
  uint256 amount = IERC20(path[1]).balanceOf(address(this));
  return (toSwap, amount);
}

Observe that the _zap() function is also assuming the path argument has a length of 2 as it is filling the amount variable using the balance of path[1].

This would potentially imply an unintended behavior as users might think they are using the whole path to enter the staking position.

The following Foundry test was used in order to prove the aforementioned issue:

function testZapThree() public {
    deal(DAI, address(gateway), 10 ether);

    uint128 sid = uint128(1);
    address[] memory fakePath = new address[](3);
    fakePath[0] = address(borpa);
    fakePath[1] = DAI;
    fakePath[2] = WETH;
    
    // Insufficient allowance
    vm.expectRevert();
    gateway.zap(100, sid, fakePath);

    deal(address(borpa), address(this), 10 ether);
    borpa.approve(address(gateway), 10 ether);

    emit log_named_decimal_uint("User BORPA balance before ZAP", borpa.balanceOf(address(this)), 18);
    emit log_named_decimal_uint("User DAI balance before ZAP", IERC20(DAI).balanceOf(address(this)), 18);
    emit log_named_decimal_uint("User WETH balance before ZAP", IERC20(WETH).balanceOf(address(this)), 18);

    emit log_named_decimal_uint("\nGW BORPA balance before ZAP", borpa.balanceOf(address(gateway)), 18);
    emit log_named_decimal_uint("GW DAI balance before ZAP", IERC20(DAI).balanceOf(address(gateway)), 18);
    emit log_named_decimal_uint("GW WETH balance before ZAP", IERC20(WETH).balanceOf(address(gateway)), 18);
    
    gateway.zap(10 ether, sid, fakePath);
    
    emit log_named_decimal_uint("\n\nUser BORPA balance after ZAP", borpa.balanceOf(address(this)), 18);
    emit log_named_decimal_uint("User DAI balance after ZAP", IERC20(DAI).balanceOf(address(this)), 18);
    emit log_named_decimal_uint("User WETH balance after ZAP", IERC20(WETH).balanceOf(address(this)), 18);

    emit log_named_decimal_uint("\nGW BORPA balance after ZAP", borpa.balanceOf(address(gateway)), 18);
    emit log_named_decimal_uint("GW DAI balance after ZAP", IERC20(DAI).balanceOf(address(gateway)), 18);
    emit log_named_decimal_uint("GW WETH balance after ZAP", IERC20(WETH).balanceOf(address(gateway)), 18);
}

To run it, just execute the following forge command:

forge test --mt testZapThree -vv --fork-url https://rpc.ankr.com/eth

Observe that a path of three elements was used to call the zap() function (and also the _zap() inside it) while the enter() function inside will only use the first and second elements:

[PASS] testZapThree() (gas: 1364184)
Logs:
  dealing tokens
  User BORPA balance before ZAP: 10.000000000000000000
  User DAI balance before ZAP: 0.000000000000000000
  User WETH balance before ZAP: 0.000000000000000000
  
GW BORPA balance before ZAP: 0.000000000000000000
  GW DAI balance before ZAP: 10.000000000000000000
  GW WETH balance before ZAP: 0.000000000000000000
  

User BORPA balance after ZAP: 0.000000000000000000
  User DAI balance after ZAP: 0.000000000000000000
  User WETH balance after ZAP: 0.000000000000000000
  
GW BORPA balance after ZAP: 0.049500000000000000
  GW DAI balance after ZAP: 9.799995149000001599
  GW WETH balance after ZAP: 0.000000001465227982

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 19.65s (7.61s CPU time)

Ran 1 test suite in 20.25s (19.65s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

For the sake of consistency, consider restricting the zap() function to be called with a path argument of only 2 elements as presupposed by the _zap() and enter() function calls inside.

Remediation Plan

SOLVED: The Entangle team solved this issue by limiting the path length to 2.

In order to allow for the transfer of tokens from one address, the protocol calls the approve() function using the IERC20 interface in several places.

• Some tokens, to protect against approval race conditions, do not allow approving an amount M > 0 when an existing amount N > 0 is already approved.

• The approve call does not return a boolean.

• The function reverts if the approval value is larger than uint96.

Even when approve is used on well-defined tokens (e.g., USDC or CDT LPT) that properly follow the ERC-20 standard, it can still raise concerns about race conditions. Additionally, BorpaGateway calls approve on token0 and token1 provided by the user. If one of these tokens is set to an address such as USDT (as intended according to the CDT pool mentioned in the documentation), the function will revert because this token does not strictly follow the ERC-20 standard.

Consider replacing every occurrence of approve with safeIncreaseAllowance() and safeDecreaseAllowance() to prevent race conditions in token approvals and ensure robustness regardless of the tokens used.

Remediation Plan

SOLVED: The Entangle team solved this issue as recommended.

The allPlayerEntriesInChest() function is designed to return all player entries in a specified chest by chest ID and user address.

function allPlayerEntriesInChest(uint256 chestId, address user) external view returns (EntryData[] memory) {
  uint256 len = playerEntriesLengthInChest(chestId, user);
  EntryData[] memory entries = new EntryData[](len);
  for (uint256 i = 0; i < len; i++) {
      entries[i] = chestsInfo[currentChest].playerEntries[msg.sender][i]; 
  }
  return entries;
}

However, it incorrectly retrieves entries for the msg.sender instead of the provided user address and only checks entries for the currentChest rather than the specified chestId. This leads to the function not returning the intended user's entries for the desired chest.

Update the allPlayerEntriesInChest() function to use the user parameter instead of msg.sender and the chestId parameter instead of currentChest.

Remediation Plan

SOLVED: The Entangle team solved the issue by updating the allPlayerEntriesInChest() function so it uses the user parameter instead of msg.sender and the chestId parameter instead of currentChest.

The massUpdatePools() function from MasterBorpa contract iterates through all active pools to update their reward states.

function _massUpdatePools() internal {
    uint256 length = _activePools.length();
    for (uint256 index = 0; index < length; ++index) {
        _updatePool(_activePools.at(index));
    }
}

This can lead to an out-of-gas error due to high gas consumption when there are many active pools.

Consider modifying the massUpdatePools() function to accept an array of pool indexes for targeted updates. If this array is provided, update only the specified pools. If the array length is zero, default to updating all active pools, mitigating the risk of out-of-gas errors.

Remediation Plan

RISK ACCEPTED: The Entangle team accepted the risk of this issue.

The buybackAndBurn() function from the Babburuzu contract is responsible for converting USDC tokens to BorpaTokens and then burning them.

function buybackAndBurn() public onlyAdminOrSystem {
    uint256 tokenBalance = IERC20(USDC).balanceOf(address(this));
    IERC20(USDC).approve(router, tokenBalance);
    if (tokenBalance > 0) {
        _sellTokenFor(USDC, tokenBalance, BORPA);
    }

    tokenBalance = IERC20(BORPA).balanceOf(address(this));
    if (tokenBalance > 0) {
        IBorpaToken(BORPA).systemBurn(tokenBalance);
    }
}

In the BorpaToken contract, the designated function systemBurn() was created to increase the balance of totalSystemBurnedAmount if the burn is performed by internal system operations (in this case, the deflation mechanism). However, the buybackAndBurn() function incorrectly calls the burn function, which will create the lottery entries for the Babburuzu address.

Ensure that the buybackAndBurn() function correctly calls the systemBurn() function.

Remediation Plan

SOLVED: The Entangle team solved this issue as recommended.

The setYieldBooster() function of the MasterBorpa contract emits an event to indicate that a new yield booster address has been configured. It emits the event SetYieldBooster which is supposed to show the previous and new/current yield booster address.

function setYieldBooster(
    address _yieldBooster
) external onlyRole(DEFAULT_ADMIN_ROLE) {
    yieldBooster = _yieldBooster;

    emit SetYieldBooster(yieldBooster, _yieldBooster);
}

Notice that the event is incorrectly emitting with both fields using the new address set.

Consider emitting the event before assigning the new value to the yieldBooster variable, so it shows the old and the new yield booster address value.

Remediation Plan

RISK ACCEPTED: The Entangle team accepted the risk of the issue. The yieldBooster is plan to be set only once.

The BorpaGateway contract is designed to be used with the UUPS proxy pattern. However, it lacks storage gaps. Storage gaps are essential for ensuring that new state variables can be added in future upgrades without affecting the storage layout of inheriting child contracts. Without it, any addition of new state variables in future contract versions can lead to storage collisions.

Consider adding a storage gap as the last storage variable to BorpaGateway contract.

Remediation Plan

SOLVED: The Entangle team solved by adding gap variable to the storage.

The recoverNative function of the ChestManager contract uses Solidity's .transfer() method to send Ether (or native tokens) to recipients.

function recoverNative(
  uint256 amount
) external onlyRole(DEFAULT_ADMIN_ROLE) {
  payable(msg.sender).transfer(amount);
}

While transfer() is designed to prevent reentrancy attacks by limiting the gas sent to 2300, it poses significant limitations, particularly when interacting with contracts that require more gas to execute their fallback functions, such as multi-signature wallets. This limitation prevents multi-signature wallets from being used as recipients in these transactions, as they typically require more gas to accept and process incoming transfers.

The following Foundry test was used in order to prove the aforementioned issue:

function testSCWrecoverNative() public {
    // Fund the ChestManager contract with a donation of 20 Ether
    Donator newDonator = new Donator{value: 20 ether}();
    newDonator.donate(address(manager));

    // Create the new SmartContractWallet and make it ChestManger the admin
    SCW scw = new SCW();
    vm.prank(admin);
    manager.grantRole(DEFAULT_ADMIN_ROLE, address(scw));

    // The new admin cannot recover funds because recoverNative() uses .transfer
    vm.expectRevert();
    vm.prank(address(scw));
    manager.recoverNative(20 ether);
}

Replace the .transfer() method with Address.sendValue(), a function provided by OpenZeppelin's Address utility library. The sendValue() safely sends Ether while providing all available gas (minus a stipend for the transaction itself), thus accommodating contracts that consume more gas in their fallback functions.

Remediation Plan

SOLVED: The Entangle team solved this issue by using the sendValue() function.

During the security assessment, it was found that the createChest() function in the ChestManager contract is always emitting and returning a value named chest which is always address 0. See the function below and notice how the variable chest is used (in the emitted event and returned).

function createChest(uint256 level) private returns (address chest) {
    uint256 nowThreshold = chestsInfo[level - 1].openingThreshold;
    uint256 newOpeningThreshold = nowThreshold * 13300 / 10000;

    chestsInfo[level].openingThreshold = newOpeningThreshold;
    chestsInfo[level].createdAt = block.timestamp;

    currentChest += 1;
    uint256 feesForNextChest = nowThreshold * NEXT_CHEST_FEE / FEE_DENOMINATOR; // 30% of previous
    chestsInfo[level].currentBalance = feesForNextChest;

    emit ChestCreated(level, chest, newOpeningThreshold, feesForNextChest);
    // @audit-info current chest (returned and in the emitted event) is always address(0)
}

Consider using the chest variable or removing it altogether.

Remediation Plan

SOLVED: The Entangle team solved the issue by removing the chest variable.

The grabChestTreasury() function from the ChestManager contract enables an admin to grab the treasury associated with a specific chest.

function grabChestTreasury(uint256 chestId) external onlyRole(DEFAULT_ADMIN_ROLE) {
    uint256 amountBuyBack = calculateBuyback(chestId);
    uint256 amountToSend = _calculateGrabAmount(chestId, amountBuyBack);

    if (amountToSend == 0) {
        revert ChestManager__E1(chestId);
    }

    IERC20(USDC).safeTransfer(msg.sender, amountToSend);

    IERC20(USDC).safeTransfer(depositor, amountBuyBack);
    IBabburuzu(depositor).buybackAndBurn();

    emit LootClaimed(chestId, msg.sender, amountToSend);
}

However, there is no mechanism to track if a chest's treasury has already been grabbed, allowing the same chest's treasury to be repeatedly accessed and transferred. This can lead to unintended double-spending if the same chestId is provided again.

Consider introducing a state-tracking mapping that records the status of each chestId to ensure that a chest's treasury can only be grabbed once.

Remediation Plan

SOLVED: The Entangle team solved this issue. The chestLooted mapping was implemented for the mentioned purpose.

At least all public and external functions that are not view or pure should have NatSpec comments. During the security review, it was observed that the multiple publicly-accessible functions did not have any NatSpec comments.

Moreover, the test suite's coverage is insufficient, representing a less-than-ideal practice and failing to ensure thorough validation of function behaviors and overall contract functionality.

Consider adding NatSpec documentation to all functions, specially those that are publicly-accessible.

Additionally, expanding the test suite's coverage is essential to effectively validate function functionalities and ensure thorough contract testing. Enhancing the test suite represents a best practice, instilling confidence in contract behavior, mitigating the risk of undetected bugs, and streamlining the development process. By addressing these areas, the project can foster transparency, reliability, and robustness, thereby enhancing its overall quality and user experience.

Remediation Plan

ACKNOWLEDGED: The Entangle team acknowledged the issue.

Throughout the code in scope, there are several instances where the imports, errors, and events are declared but never used.

In Babburuzu.sol:

• import {IBorpaPair} from "../contracts/interfaces/IBorpaPair.sol"

• error Babburuzu__E1(); // Transfer failed

In BorpaGateway.sol:

• error BorpaGateway__E1(); // Invalid swap data

• import {AccessControl} from "@openzeppelin/contracts/access/AccessControl.sol";

In NFTPool.sol:

• import {IBabburuzu} from "./interfaces/IBabburuzu.sol";

• import {INFTHandler} from "./interfaces/INFTHandler.sol";

• error NFTPool__E7(); // Not implemented

• error NFTPool__E8(); // Token already exists

In XBorpaToken.sol:

• event CancelRedeem(address indexed userAddress, uint256 xBorpaAmount);

Consider removing all unused imports, errors and events.

Remediation Plan

ACKNOWLEDGED: The Entangle team acknowledged the issue.

The files in scope currently use floating pragma version ^0.8.20, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.20, and less than 0.9.0. It is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Lock the pragma version to the same version used during development and testing.

Remediation Plan

SOLVED: The pragma was updated to the fixed 0.8.24 version.

Global state variables that are set in the constructor and never updated again should be declared as immutable to save gas.

• Babburuzu.BORPA

• BAbburuzu.WETH

• Babburuzu.USDC

• Babburuzu.chestManager

• Babburuzu.router

  
  

• ChestManager.USDC

• ChestManager.borpa

  
  

• MasterBorpa.borpa

  
  

• NFTPool.factory

  
  

• NFTPoolFactory.borpa

• NFTPoolFactory.xBorpa

• NFTPoolFactory.borpaMaster

• NFTPoolFactory.babburuzu

  
  

• xBorpaToken.borpa

  
  

• YieldBooster.xBorpa

The immutable keyword was added in Solidity in 0.6.5. State variables can be marked immutable, which causes them to be read-only, but only assignable in the constructor.

Consider adding the immutable attribute to the mentioned global state variables.

Remediation Plan

ACKNOWLEDGED: The Entangle team indicated that variables will be set during initialization in the next version, instead of constructor.

The _sellTokenFor() function implemented in Babburuzu contract supports three types of swaps:

• swapping ETH for tokens,

• tokens for ETH,

• tokens for other tokens.

  
  

function _sellTokenFor(
    address tokenIn,
    uint256 amount,
    address tokenOut
) internal {
    if (tokenIn != tokenOut && tokenIn != BORPA) {
        if (tokenIn == WETH) {
            IBorpaRouter(router)
                .swapExactETHForTokensSupportingFeeOnTransferTokens{
                value: amount
            }(0, paths[WETH][tokenOut], address(this), block.timestamp);
        } else if (tokenOut == WETH) {
            IBorpaRouter(router)
                .swapExactTokensForETHSupportingFeeOnTransferTokens(
                    amount,
                    0,
                    paths[tokenIn][WETH],
                    address(this),
                    block.timestamp
                );
        } else {
            IBorpaRouter(router)
                .swapExactTokensForTokensSupportingFeeOnTransferTokens(
                    amount,
                    0,
                    paths[tokenIn][tokenOut],
                    address(this),
                    block.timestamp
                );
        }

However, this function is internally called only in the buybackAndBurn() function, which only handles scenarios where either the input token (tokenIn) or the output token (tokenOut) is WETH, making the third swap case (tokens for other tokens) redundant.

Consider removing the unused branch.

Remediation Plan

SOLVED: The Entangle team solved this issue.

The onlyPool modifier and the internal _sortTokens() function in the Babburuzu contract are never used in the codebase. Additionally, the internal function _requireOnlyOwner() in NFTPool is also never used.

For better clarity, consider removing the unused code.

Remediation Plan

SOLVED: The Entangle team solved this issue.

Two typographical errors were found in a function name and a comment:

• On ChestManager.sol:

function entriesLenghtInChest(uint256 id) public view returns (uint256) {

entriesLenghtInChest() should be entriesLengthInChest().

• On YieldBooster.sol:

error YieldBooster__E4();       // Unathorized: force deallocation status

Unathorized should be Unauthorized.

To maintain clarity and trustworthiness, it is essential to rectify any typographical errors present within the contracts. Correcting such errors minimizes the likelihood of confusion and reinforces confidence in the accuracy and integrity of the documentation.

Remediation plan

ACKNOWLEDGED: The Entangle team acknowledged this finding.

It has been observed that important functionalities are missing emitting events.

Events are a method of informing the transaction initiator about the actions taken by the called function. It logs its emitted parameters in a specific log history, which can be accessed outside of the contract using some filter parameters. Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

All functions updating important parameters should emit events.

Remediation plan

ACKNOWLEDGED: The Entangle team acknowledged this finding.

In programming, "magic numbers" refer to the use of unexplained numerical or string values directly in code, without any clear indication of their purpose or origin. The use of magic numbers can lead to confusion and make your code more difficult to understand, maintain, and update.

To improve the readability and maintainability of your smart contracts, it is recommended to avoid using magic numbers and instead use named constants or variables to represent these values. By doing so, you provide clear context for the values, making it easier for developers to understand their purpose and significance.

An example not using scientific notation nor constants was in ChestManager.sol:

function createChest(uint256 level) private returns (address chest) {
  uint256 nowThreshold = chestsInfo[level - 1].openingThreshold;
  uint256 newOpeningThreshold = nowThreshold * 13300 / 10000;
...

Another example in Babburuzu.sol:

function dispatch(uint256 amount) external onlyAdminOrSystem {
  IERC20(USDC).safeTransferFrom(msg.sender, address(this), amount);

  // 20% to dev wallet
  uint256 toTransfer;
  toTransfer = amount * 20 / 100;
...

And another example not using scientific notation nor a constant in NFTPool.sol:

function _harvestPosition(uint256 tokenId, address to, address xBorpaRec) internal {
...
  xBorpaRewards = pending * X_BORPA_REWARDS_SHARES /
  10000
  ;
...

Avoid the use of magic numbers and replace them with named constants or variables, making it easier for developers to understand and work with the codebase. For large numbers, consider using scientific notation (e.g.: 10e4).

Remediation Plan

ACKNOWLEDGED: The Entangle team acknowledged the issue.

During the security assessment, an empty revert() statement was found to be missing an error message. This was found on NFTPoolFactory.sol:

if (
    borpaMaster == address(0) 
    || borpa == address(0)
    || xBorpa == address(0)
    || cdtToken == address(0)
    || gateway == address(0)
) {
    revert();
}

It is recommended to always use descriptive reason strings or custom errors for revert paths.

Remediation Plan

ACKNOWLEDGED: The Entangle team acknowledged the issue.

The project is using Solidity version greater than 0.8.18, which supports named mappings. Using named mappings can improve the readability and maintainability of the code by making the purpose of each mapping clearer. This practice helps developers and auditors understand the mappings' intent more easily.

Consider refactoring the mappings to use named arguments, which will enhance code readability and make the purpose of each mapping more explicit.

For example, on Babburuzu, instead of declaring:

mapping(address => mapping(address => address[])) public paths;

It could be declared as:

mapping(address tokenFrom => mapping(address tokenTo => address[])) public paths;

Remediation Plan

SOLVED: The Entangle team solved the issue by using named mappings as suggested.

During the security assessment, a function in the ChestManager contract was found to contain some commented out code (presumably from testing).

function totalPlayerRange(
    address user
) external view returns (uint256 totalRange) {
    // console.log("hrer");
    for (uint256 i; i <= currentChest; i++) {
        // console.log(i);
        totalRange += totalPlayerRangeInChest(i, user);
    }
}

It is recommended to remove any commented out code that does not need to go in production.

Remediation Plan

SOLVED: The Entangle team solved the issue by removing the commented out code.