Entangle Trillion - Entangle Labs

The depositLP function in the StargateSynthChef.sol contract of the Entangle LSD Protocol is designed to facilitate the deposit of LP tokens for farming. However, a critical oversight in the contract's implementation can lead to failed transactions and impede the deposit process. The depositLP function directly calls the deposit function of the Stargate LP Staking contract without first ensuring that the Stargate contract is allowed to transfer the LP tokens on behalf of the user. In the absence of a prior approve transaction, the safeTransferFrom call in the Stargate contract will fail because the contract lacks the necessary allowance to transfer the user's LP tokens.

[StargateSynthChef.sol#L127C1-L138C1](https://github.com/Entangle-Protocol/entangle-lsd-protocol/blob/48e025e4ddf330e8ac710e82114db704a76d2857/contracts/SynthChefs/StargateSynthChef.sol#L127C1-L138C1)

    /// @notice Deposit LP tokens to farm. Can only be called by the MASTER.
    /// @param poolId Entangle internal poolId
    /// @param lpAmount Amount of LP tokens to deposit
    function depositLP(uint32 poolId, uint256 lpAmount) external onlyRole(MASTER) isPoolExist(address(pools[poolId].LPToken)) {
        console.log("StargateChef_depositLP", poolId);
        if(lpAmount == 0) revert StargateSynthChef__E3();
        Pool memory pool = pools[poolId];
        console.log(address(lpStaking), lpAmount, pool.stargateLPStakingPoolID);

        lpStaking.deposit(pool.stargateLPStakingPoolID, lpAmount);
    }

it("It should DepositLp", async () => {
    for (let index = 0; index < StargateConfig.Pools.length; index++) {
        LP = await ethers.getContractAt("ERC20", await MasterChef.getLpToken(SIDs[index]));
        const lpAmountToDeposit = await LP.connect(admin).balanceOf(admin.address);
        let lpAmountBefore = await MasterChef.connect(admin).getLpTokenTotalBalance(SIDs[index]);
        console.log(lpAmountToDeposit);

        expect(lpAmountBefore).to.eq(0);

        await LP.connect(admin).approve(MasterChef.address, lpAmountToDeposit);
        const tx1 = await MasterChef.connect(admin).depositLP(SIDs[index], lpAmountToDeposit);
        await tx1.wait();

        let lpAmountAfterDeposit = await MasterChef.getLpTokenTotalBalance(SIDs[index]);
        expect(lpAmountAfterDeposit).to.be.greaterThan(lpAmountBefore);
        console.log(lpAmountAfterDeposit);
    }
});

Before the deposit call, the contract should check if the Stargate LP Staking contract has sufficient allowance to transfer the user's LP tokens. Also, It should approve the Stargate router.

Remediation Plan: The Entangle team solved the issue by calling approve function.

In the proposeBurnAndBridge/proposeBridgeExecuted function, we are directly getting the address parameter as an argument. However, It should be bytes instead of address. This will directly block integrations on the Cosmos (bech32) addresses.

[/contracts/BalanceManager.sol#L144](https://github.com/Entangle-Protocol/entangle-lsd-protocol/blob/48e025e4ddf330e8ac710e82114db704a76d2857/contracts/BalanceManager.sol#L144)

function proposeBurnAndBridge(uint64 destChainId, uint128 sid, uint256 synthAmount, address recipient) external {
    bytes4 selector = bytes4(keccak256("HandleBurnAndBridge(bytes)"));
    bytes memory functionParams = abi.encode(destChainId, sid, synthAmount, recipient);
    aggregationSpotter.propose(protocolId, eobChainId, abi.encode(eventProcessorAddress), selector, functionParams);
}

[BalanceManager.sol#L218](https://github.com/Entangle-Protocol/entangle-lsd-protocol/blob/48e025e4ddf330e8ac710e82114db704a76d2857/contracts/BalanceManager.sol#L218)

  function proposeBridgeExecuted(uint64 destChainId, uint128 sid, uint256 synthAmount, address recipient) external {
      bytes4 selector = bytes4(keccak256("HandleBridgeExecuted(bytes)"));
      bytes memory functionParams = abi.encode(destChainId, sid, synthAmount, recipient);
      aggregationSpotter.propose(protocolId, eobChainId, abi.encode(eventProcessorAddress), selector, functionParams);
  }

    function setUp() public {
        vm.createSelectFork(vm.envString("ETH_URL"));
        vm.rollFork(47442254);

        lsdSystem = new EntangleLSDBootstrapperETH();

        // boostrap the whole Liquid Vault system with Balance manager mock
        lsdSystem.bootstrap();

        protocolId = uint32(1) + (uint32(1) << 31);

        // sid deployed in LSD protocol
        sid = (uint128(lsdSystem.getChainID()) << 64) + (uint128(protocolId) << 32) + (uint128(1));

        aTokenWrapper = address(new ATokenWrapper());
    }

    function testBrokenCosmosAddress() external {
        lsdSystem.proposeBurnAndBridge(1,sid,100,0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
    }

Consider changing address variable with a byte array.

Remediation Plan: The Entangle team solved the issue by deprecating the feature.

The SynthFactory smart contract contains a critical vulnerability related to the sellForLP function. This function is designed to allow users to sell their synthetic assets (synths) for LP (Liquidity Provider) tokens. The price of the synth is fetched from the contract using the getPrice function, which is set by the setPrice function.
The vulnerability arises from the fact that the setPrice function allows the PRICE_MANAGER role to set the price of a synth to zero. When the price is set to zero, the sellForLP function calculates the amount of LP tokens to be received (lpAmount) as zero, regardless of the amount of synth being sold. This scenario leads to a situation where users can burn their synths without receiving any LP tokens in return, effectively resulting in a loss of funds.

Users can lose their synths without receiving any LP tokens if the price is maliciously or accidentally set to zero.

[EntangleDexV2.sol#L159](https://github.com/Entangle-Protocol/entangle-lsd-protocol/blob/48e025e4ddf330e8ac710e82114db704a76d2857/contracts/EntangleDexV2.sol#L159)

   function sellForLP(uint128 sid, uint256 synthAmount, address recipient) public isPausedSID(sid) onlyLpStakingSid(sid) nonReentrant {
       if (synthAmount == 0) revert EntangleDex__E5();
       synthFactory.burn(sid, synthAmount, msg.sender, 0);
       uint price = synthFactory.getPrice(sid);
       uint lpAmount = (synthAmount * price) / 10 ** 18;
       if (block.chainid == SidLibrary.chainId(sid)) {
           masterSynthChef.withdrawLP(sid, lpAmount, recipient);
       } else {
           emit LPStackingSynthSell(sid, lpAmount, synthAmount, recipient);
           balanceManager.proposeSellForLp(sid, synthAmount, recipient);
       }
   }

Implement a validation check in the setPrice function to ensure that the price cannot be set to zero. Consider setting a reasonable minimum price threshold.

Remediation Plan: The Entangle team solved the issue by preventing the price setter with "0".

The function buyForLP has an issue concerning the handling of LP token decimals. This function is intended to facilitate the purchase of synthetic assets using LP tokens. The vulnerability arises from the calculation of the amount of synthetic assets to mint. The calculation does not account for the decimal places of the LP token, leading to an incorrect amount of synthetic assets being minted. The formula used is (lpAmount 10 * 18) / price. Here, lpAmount is assumed to be in wei (with 18 decimals), but LP tokens can have a different number of decimals.

https://github.com/Entangle-Protocol/entangle-lsd-protocol/blob/48e025e4ddf330e8ac710e82114db704a76d2857/contracts/EntangleDexV2.sol#L148

function buyForLP(uint128 sid, uint lpAmount) public onlyLpStakingSid(sid) onlyHomeChain(sid) isPausedSID(sid) nonReentrant {
    if (lpAmount == 0) revert EntangleDex__E5();
    IERC20 lpToken = IERC20(masterSynthChef.getLpToken(sid));
    lpToken.safeTransferFrom(msg.sender, address(this), lpAmount);
    lpToken.approve(address(masterSynthChef), lpAmount);
    masterSynthChef.depositLP(sid, lpAmount);
    uint price = synthFactory.getPrice(sid);
    uint amount = (lpAmount * 10 ** 18) / price;
    synthFactory.mint(sid, amount, msg.sender, 0);
}

Before performing the calculation, retrieve the actual number of decimals for the LP token using a function like lpToken.decimals().

Remediation Plan: The Entangle team solved the issue by minting small amount of tokens.

In the burnAndBridge function of a smart contract, there is an issue due to the absence of validations for destChainId and recipient address. - The function does not check if the destChainId provided is part of a predetermined list of approved chains. Without this check, there is a risk of burning tokens and attempting to bridge to an unsupported or incorrect chain, leading to irrevocable loss of funds. - The function lacks a validation step for the recipient address. This absence could lead to scenarios where tokens are sent to an incorrect or invalid address, potentially resulting in the loss of those tokens.

[EntangleDexV2.sol#L178](https://github.com/Entangle-Protocol/entangle-lsd-protocol/blob/48e025e4ddf330e8ac710e82114db704a76d2857/contracts/EntangleDexV2.sol#L178)

   function burnAndBridge(uint64 destChainId, uint128 sid, uint256 synthAmount, address recipient) public onlyLpStakingSid(sid) isCrossChainAllowed(sid) isPausedSID(sid) nonReentrant {
       if (synthAmount == 0) revert EntangleDex__E5();
       synthFactory.burn(sid, synthAmount, msg.sender, 0);
       emit BurnAndBridge(destChainId, sid, synthAmount, recipient);
       balanceManager.proposeBurnAndBridge(destChainId, sid, synthAmount, recipient);
   }

    function testDepositWithdraw() external {
        address lpToken = lsdSystem.masterSynthChef().getLpToken(sid);
        address synth = address(lsdSystem.synthFactory().getSynth(sid));
        uint amount = 200e18;

        // deposit
        _mintSynth(lpToken, amount);

        assertEq(IERC20(lpToken).balanceOf(address(this)), 0, "LP not deposited");
        assertGt(IERC20(synth).balanceOf(address(this)), 0, "Synth not minted");

        lsdSystem.entangleDex().burnAndBridge(10000,sid,1,address(this));

    }

- Implement a mechanism to maintain a whitelist of supported destination chain IDs (destChainId). Before proceeding with the burn and bridge operation, verify that the provided destChainId is in the whitelist.

- Add a check to ensure the recipient address is a valid and non-zero address. This step is crucial to prevent transactions to incorrect or null addresses.

Remediation Plan: The Entangle team solved the issue by deprecating the feature.

The smart contract function proposeBurnAndBridge is designed to initiate significant operations such as burning synthetic assets and preparing them for cross-chain bridging. However, this function lacks any form of access control mechanism, making it publicly callable by any entity on the network. Without restrictions, such as those provided by role-based access control (RBAC) or ownership checks, the contract is vulnerable to unauthorized access and potential malicious manipulation.

This design flaw exposes the contract to various risks, including unauthorized burning of synthetic assets, unintended interactions with liquidity pools, and unauthorized initiation of cross-chain operations. The absence of access control mechanisms fundamentally compromises the contract's integrity and the security of its operations.

https://github.com/Entangle-Protocol/entangle-lsd-protocol/blob/48e025e4ddf330e8ac710e82114db704a76d2857/contracts/BalanceManager.sol#L144

   function proposeBurnAndBridge(uint64 destChainId, uint128 sid, uint256 synthAmount, address recipient) external {
       bytes4 selector = bytes4(keccak256("HandleBurnAndBridge(bytes)"));
       bytes memory functionParams = abi.encode(destChainId, sid, synthAmount, recipient);
       aggregationSpotter.propose(protocolId, eobChainId, abi.encode(eventProcessorAddress), selector, functionParams);
   }

    function testMaliciousCrossChainAction() external {
        balanceManager.proposeBurnAndBridge(100000000,sid,100,0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
    }

Introduce access control checks within the proposeBurnAndBridge function to ensure that only authorized addresses (e.g., MasterSynthChef or other designated administrators) can invoke it.

Remediation Plan: The Entangle team solved the issue by deprecating the feature.

The smart contract functions proposeSellForLp, proposeSellForLPAndWithdraw, proposeDepositLp, proposeWithdrawLp, and proposeCompound are integral to managing synthetic assets and liquidity pool (LP) interactions within the contract ecosystem. These functions facilitate critical operations such as selling synthetic assets for LP tokens, depositing and withdrawing LP tokens, and compounding LP balances on farms. However, a significant security concern arises from the absence of access control mechanisms in these functions, making them publicly callable without any restrictions.

This unrestricted access poses several risks, notably allowing any external actor to potentially initiate synthetic asset sales, LP deposits, withdrawals, and other sensitive operations without authorization.

   function proposeSellForLp(uint128 sid, uint256 synthAmount, address recipient) external {
       bytes4 selector = bytes4(keccak256("HandleLPStackingSynthSell(bytes)"));
       bytes memory functionParams = abi.encode(sid, synthAmount, recipient);
       aggregationSpotter.propose(protocolId, eobChainId, abi.encode(eventProcessorAddress), selector, functionParams);
   }

   function proposeSellForLPAndWithdraw(uint128 sid, uint256 lpAmount, uint256 synthAmount, uint64 chainId, address recipient) external {
       bytes4 selector = bytes4(keccak256("HandleLPStackingSynthSellAndWithdraw(bytes)"));
       bytes memory functionParams = abi.encode(sid, lpAmount, synthAmount, chainId, recipient);
       aggregationSpotter.propose(protocolId, eobChainId, abi.encode(eventProcessorAddress), selector, functionParams);
   }

   function proposeDepositLp(uint128 sid, uint256 lpAmount, uint256 totalLpBalance) external {
       bytes4 selector = bytes4(keccak256("HandleDepositLP(bytes)"));
       bytes memory functionParams = abi.encode(sid, lpAmount, totalLpBalance);
       aggregationSpotter.propose(protocolId, eobChainId, abi.encode(eventProcessorAddress), selector, functionParams);
   }

   function proposeWithdrawLp(uint128 sid, uint256 lpAmount, address recipient, uint256 totalLpBalance) external {
       bytes4 selector = bytes4(keccak256("HandleWithdrawLP(bytes)"));
       bytes memory functionParams = abi.encode(sid, lpAmount, recipient, totalLpBalance);
       aggregationSpotter.propose(protocolId, eobChainId, abi.encode(eventProcessorAddress), selector, functionParams);
   }

   function proposeCompound(uint128 sid, uint256 newLpBalanceOnFarm) external {
       bytes4 selector = bytes4(keccak256("HandleCompound(bytes)"));
       bytes memory functionParams = abi.encode(sid, newLpBalanceOnFarm);
       aggregationSpotter.propose(protocolId, eobChainId, abi.encode(eventProcessorAddress), selector, functionParams);
   }

   function testMaliciousCrossChainAction() external {
       balanceManager.proposeBurnAndBridge(100000000, sid, 100, 0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
       balanceManager.proposeSellForLp(sid, 100, 100, 0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
       balanceManager.proposeSellForLPAndWithdraw(sid, 100, 100, 9999, 100, 100, 0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
       balanceManager.proposeDepositLp(sid, 100, 100);
       balanceManager.proposeWithdrawLp(sid, 100, 100, 0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
       balanceManager.proposeWithdrawLp(sid, 100, 0xF403C135812408BFbE8713b5A23a04b3D48AAE31, 100);
       balanceManager.proposeCompound(sid, 1000);
   }

Integrate RBAC mechanisms to restrict function calls to authorized entities, such as contract administrators or designated accounts. Utilize OpenZeppelin's AccessControl library or similar to manage roles and permissions efficiently.

Remediation Plan: The Entangle team solved the issue by deprecating the feature.

The smart contract functions proposeMint and proposeBurn play crucial roles in managing the tokenomics of the ecosystem by handling the minting and burning of synthetic assets. Despite their significance, both functions currently lack essential access control mechanisms. This omission permits any external entity to propose minting or burning operations without restrictions, leading to potential unauthorized manipulation of the token supply.

This design flaw exposes the ecosystem to severe risks, including inflation or deflation of the token supply through unauthorized minting or burning, which could undermine the token's value and the overall economic model.

function proposeMint(uint128 sid, uint256 synthAmount, address recipient, uint256 totalSupply, uint256 opId) external {
    bytes4 selector = bytes4(keccak256("HandleMint(bytes)"));
    bytes memory functionParams = abi.encode(sid, synthAmount, recipient, totalSupply, opId);
    aggregationSpotter.propose(protocolId, eobChainId, abi.encode(eventProcessorAddress), selector, functionParams);
}

function proposeBurn(uint128 sid, uint256 synthAmount, address from, uint256 totalSupply, uint256 opId) external {
    bytes4 selector = bytes4(keccak256("HandleBurn(bytes)"));
    bytes memory functionParams = abi.encode(sid, synthAmount, from, totalSupply, opId);
    aggregationSpotter.propose(protocolId, eobChainId, abi.encode(eventProcessorAddress), selector, functionParams);
}

    function testBrokenAccessControl() external {
        balanceManager.proposeMint(sid,1000000,address(this),1000000000,1);
        balanceManager.proposeBurn(sid,1000000,address(this),1000000000,1);
    }

Introduce RBAC to restrict the ability to propose minting and burning operations to authorized roles only, such as contract administrators or designated accounts. Utilize OpenZeppelin's AccessControl library to manage roles and permissions effectively.

Remediation Plan: The Entangle team solved the issue by deprecating the feature.

The smart contract contains functions such as reinvest and withdraw that perform conversions between int128, int256, and uint256 without validating that these conversions are safe. Here's a step-by-step breakdown of how this could lead to vulnerabilities, using the reinvest function as an example:

• In the reinvest function, there's a line where a uint256 variable amounts[i] is converted to int128 for a function call:

  run3(
      ..., 
      EXACTLY, int128(int256(amounts[0])),
      ...
  );

  run3( ..., EXACTLY, int128(int256(amounts[0])), ... );

  
  

• Given uint256 can store values much larger than what int128 can hold, this conversion is unsafe without checks.

• Assume a scenario where amounts[0] holds a value greater than 2**127 - 1 (the maximum positive value int128 can represent).

• The direct casting to int128 would result in an overflow, leading to an incorrect, potentially negative value.

    it("Deposit -> wihdrawLP (tweaked)", async () => {
        /*  
            comment out balance manager part until it is deployed
        */
        const { amountUsdc, amountToken } = await fundProtoChef()
        const LP = new ethers.Contract(VelocoreConfig.Pools[0].LPTokenAddress, ERC20_ABI, depositorCompounder);

        await VelocoreChef.connect(depositorCompounder).deposit(poolId, [amountUsdc, amountToken])

        const lpAmountAfterDeposit = await MasterChef.getLpTokenTotalBalance(SID);
        // console.log(lpAmountAfterDeposit)
        // console.log(ethers.utils.formatEther(lpAmountAfterDeposit))
        expect(lpAmountAfterDeposit).to.be.gt(0);

        await MasterChef.connect(depositorCompounder).withdrawLP(SID, 115792089237316195423570985008687907853269984665640564039457584007913129639935 , depositorCompounder.address)
        const balanceAfterWithdraw = await MasterChef.getLpTokenTotalBalance(SID)
        expect(balanceAfterWithdraw).to.be.equal(0);

        const depositorLPBalance = await LP.balanceOf(depositorCompounder.address)
        expect(depositorLPBalance).to.equal(lpAmountAfterDeposit)

    });

Implement or use an existing library designed for safe type conversions, such as OpenZeppelin's SafeCast.

Remediation Plan: The Entangle team solved the issue by using SafeCast on the codebase.

The compounder contract in the given system possesses a critical vulnerability that could lead to its self-destruction if the externally owned account (EOA) is compromised. This vulnerability is primarily due to the implementation of delegatecall in the compound function.

    function compound(address protocolSynthChef, uint32 poolId, PreconfiguredCallConfig[] calldata callConfigs, bool reinvest) external returns (uint) {
        require(msg.sender == masterSynthChef, "auth failed");
        IProtocolSynthChef(protocolSynthChef).harvest(poolId);

        for (uint i = 0; i < callConfigs.length; ++i) {
            PreconfiguredCallConfig memory _call = callConfigs[i];
            if (_call.isDelegateCall) {
                (bool success, bytes memory data) = _call.callee.delegatecall(_call.payload);
                _revertIfFailed(success, data);
            } else {
                (bool success, bytes memory data) = _call.callee.call(_call.payload);
                _revertIfFailed(success, data);
            }
        }

        address[] memory underlyings = IProtocolSynthChef(protocolSynthChef).getPoolTokens(poolId);

        for (uint i = 0; i < underlyings.length; ++i) {
            uint balance = IERC20(underlyings[i]).balanceOf(address(this));
            if (balance != 0) {
                SafeERC20.safeTransfer(IERC20(underlyings[i]), protocolSynthChef, balance);
            }
        }

        if (reinvest)
            return IProtocolSynthChef(protocolSynthChef).reinvest(poolId);
        return 0;
    }

    function _revertIfFailed(bool success, bytes memory data) internal pure {
        if (!success) {
            assembly {
              revert(add(data,32),mload(data))
            }
        }
    }

    contract A {
      function kill (address payable to) public {
        selfdestruct(to);
      }
    }
    // call run will excute selfdestract on contract B & fund eth to toAddress
    contract B {
      function run (address target, string calldata func, address to) public {
        target.delegatecall(abi.encodeWithSignature(func, to));
      }
    }

Refrain from using delegatecall with dynamic inputs from users or EOAs. Restrict delegatecall to predefined, trusted contracts only.

Remediation Plan: The Entangle team accepted the risk of this issue.

In decentralized finance (DeFi), transactions are transparent on the blockchain, allowing observers to anticipate trades and potentially exploit them. A notable exploit is the "sandwich attack," which leverages the predictability of transaction execution to manipulate market prices for profit. This proof of concept outlines how the reinvest function in SynthChef contracts, when called with amountMin parameter set to zero, can be vulnerable to such attacks.

Step 1: Observer Identifies a Vulnerable Transaction

1. An MEV bot (the observer) monitors pending transactions in the mempool for calls to the reinvest function in SynthChef contracts where amountMin are set to zero.

Step 2: Attacker Prepares a Sandwich Attack

1. Upon identifying a vulnerable transaction, the attacker prepares two transactions:

   • Front-run Transaction: Buys asset Y (the asset being bought in the victim's trade) before the victim's transaction, anticipating a price increase.

   • Back-run Transaction: Sells asset Y after the victim's trade, aiming to profit from the price increase caused by the victim's transaction.

Step 3: Execution of the Sandwich Attack

1. The attacker submits the front-run transaction with a higher gas price to ensure it is mined before the victim's transaction.

2. The victim's transaction executes, buying asset Y as intended but at a higher price due to the front-run transaction, causing slippage.

3. The attacker submits the back-run transaction, also with a high gas price, to sell asset Y at the new, elevated price.

Step 4: Outcome and Impact

1. The attacker profits from the price difference between the front-run and back-run transactions, exploiting the lack of slippage protection in the victim's trade.

2. The victim experiences a worse trade outcome than expected, potentially losing a significant amount due to slippage and price manipulation.

Always set reasonable amountMin values in liquidity addition/removal functions to provide slippage protection.

Remediation Plan: The Entangle team solved the issue by adding minimum return check.

In the context of DeFi protocols, DexWrappers like UniswapHandler facilitate interactions with decentralized exchanges (DEXes) such as Uniswap by providing functions to swap tokens, add liquidity, and remove liquidity. Crucially, these interactions require specifying a minimum output amount (amountOutMin) for trades to protect against undesirable slippage and prevent exploitation through sandwich attacks. This proof of concept details how failing to set amountOutMin appropriately in the removeLiquidity and swapExactTokensForTokens functions exposes the contract to MEV (Miner Extractable Value) attacks.

Step 1: Attacker Identifies the Transaction

1. An attacker monitors the mempool for swapExactTokensForTokens calls made by UniswapHandler with amountOutMin set to zero.

Step 2: Preparing the Sandwich Attack

1. Front-run Transaction: Upon spotting a vulnerable swap transaction from DAI to MemeToken, the attacker buys MemeToken before the UniswapHandler transaction is processed, expecting the price of MemeToken to increase.

2. Back-run Transaction: The attacker prepares to sell MemeToken back into DAI after the UniswapHandler transaction, capitalizing on the increased price of MemeToken.

Step 3: Executing the Attack

1. The attacker submits the front-run transaction with a higher gas fee to ensure it is executed before the UniswapHandler transaction.

2. The UniswapHandler transaction executes, swapping DAI for MemeToken at an unfavorable rate due to the price manipulation caused by the front-run transaction.

3. The attacker executes the back-run transaction, selling MemeToken for DAI at the inflated price, profiting from the price difference created by the sandwich attack.

• Modify the removeLiquidity and swapExactTokensForTokens functions to require a non-zero amountOutMin parameter, reflective of acceptable slippage for the operation.

• Utilize on-chain data to dynamically calculate an appropriate amountOutMin based on current market conditions and the specific requirements of the trade or liquidity removal.

Remediation Plan: The issue is marked as “Not Applicable” due to the related code-base is deprecated.

The MasterSpotter contract, which is part of a broader system designed to handle various blockchain events, includes the function HandleTokenMintAndSwap. This function is intended to process specific events related to token minting and swapping. Upon review, it is observed that the function contains a placeholder comment "TODO handle?" and does not implement any logic beyond decoding the event parameters. This incomplete implementation suggests that the functionality for handling the TokenMintAndSwap event is not fully developed, potentially leading to unprocessed events and missed actions that could be critical for the intended operation of the system.

    // #region Balance Spotter Handlers
    // ??? Huh will this ever be called
    function HandleTokenMintAndSwap(bytes calldata b) public onlyRole(SPOTTER) {
        (, , , , bytes memory eventParams) = abi.decode(b, (bytes32, uint256, uint256, bytes32, bytes));

        EntangleTestBridgeEvents.TokenMintAndSwap memory tokenMintAndSwap = EntangleTestBridgeEvents
            .DecodeTokenMintAndSwap(eventParams);
        // TODO handle?
        return;
    }

Review the intended behavior and necessary actions for the TokenMintAndSwap events within the context of the MasterSpotter contract's role in the system. Develop and integrate the required logic to ensure that these events are processed appropriately, executing any necessary actions such as updating internal states, interacting with other contracts, or triggering further events.

Remediation Plan: The issue is marked as “Not Applicable” due to the related code-base is deprecated.

interface IERC677Receiver {
    function onTokenTransfer(address _sender, uint _value, bytes calldata _data) external;
}

interface IERC777TokensRecipient {
    function tokensReceived(address operator, address from, address to, uint256 amount, bytes calldata userData, bytes calldata operatorData) external;
}

contract AttackerContract is IERC677Receiver, IERC777TokensRecipient {
    EntangleTestBridge public victim;
    IERC20 public attackToken;

    constructor(address _victimAddress, address _tokenAddress) {
        victim = EntangleTestBridge(_victimAddress);
        attackToken = IERC20(_tokenAddress);
    }

    // Fallback function to initiate the attack
    function onTokenTransfer(address _sender, uint _value, bytes calldata _data) external override {
        // Re-entrancy attack logic
        victim.withdraw(attackToken, _value);
    }

    function tokensReceived(address operator, address from, address to, uint256 amount, bytes calldata userData, bytes calldata operatorData) external override {
        // Re-entrancy attack logic
        victim.withdraw(attackToken, amount);
    }

    function attack(uint256 amount) external {
        attackToken.transferFrom(msg.sender, address(victim), amount);
        // The callback from ERC677 or ERC777 token transfer will trigger the re-entrancy
    }
}

Attack Execution

4. Initiate the Attack: The attacker calls the attack function of the AttackerContract with the amount of tokens they wish to exploit. This function transfers ERC677/ERC777 tokens to the EntangleTestBridge, which triggers the onTokenTransfer or tokensReceived callback in AttackerContract.

5. Callback and Re-entrancy: The onTokenTransfer or tokensReceived function in AttackerContract is automatically called due to the token transfer, and it immediately invokes the vulnerable withdraw function in EntangleTestBridge, exploiting the re-entrancy vulnerability.

6. Repeat Unauthorized Withdrawals: The callback and re-entrant call to withdraw allow the attacker to withdraw more tokens than they should be allowed, exploiting the contract's flawed state management.

Restructure the withdraw function to update the contract's internal state (tokenStorage[address(token)] -= amount) before performing the external call to token.safeTransferFrom. This change ensures that the contract's state is consistent even if a re-entrancy attack is attempted.

Remediation Plan: The issue is marked as “Not Applicable” due to the related code-base is deprecated.

The withdraw function in the contract is designed to withdraw LP tokens from a farm, remove liquidity, and then transfer the resultant tokens to the Entangle MasterChef. A critical issue arises from setting both amountAMin and amountBMin parameters to 1 in the router.removeLiquidity call. This approach poses a significant risk of slippage, making the transaction susceptible to front-running attacks and potentially resulting in a significantly worse exchange rate than expected.

By setting the minimum received tokens (amountAMin and amountBMin) to 1, the contract effectively removes any meaningful slippage protection. This can lead to unfavorable rates for the withdrawn liquidity, especially in volatile or low-liquidity pools, where the actual amounts received can be drastically lower than the market rate. Such a vulnerability could be exploited by malicious actors performing front-running attacks, further worsening the slippage experienced by the contract.

The vulnerability arises from setting the minimum received amounts (amountAMin and amountBMin) to 1 in a liquidity removal operation. This practically removes any slippage protection and can be exploited through a front-running attack. Here's how an attacker could exploit this vulnerability:

An attacker monitors the mempool for pending transactions that call the withdraw function of the vulnerable contract. The goal is to identify transactions intending to remove liquidity from a pool.

Upon detecting a pending withdraw transaction, the attacker submits a transaction that buys a significant amount of one of the pool's tokens (either token0 or token1). This transaction is crafted with a higher gas price to ensure it is mined before the targeted withdraw transaction. The purchase increases the price of the bought token in the pool.

The original withdraw transaction is executed after the attacker's purchase transaction. Due to the slippage vulnerability (with amountAMin and amountBMin set to 1), the contract accepts whatever the current rate is, regardless of how unfavorable it has become due to the attacker's front-running transaction.

Implement dynamic slippage control based on the current market conditions or set more conservative, realistic slippage bounds. This could involve querying the expected amounts using a method like getAmountsOut before executing the liquidity removal and setting the amountAMin and amountBMin parameters based on a permissible slippage percentage from those expected amounts.

Remediation Plan: The issue is marked as “Not Applicable” due to the code will be used for liquidation at the future. In the current design of protocol, the user is not able to remove liquidity.

Modify the existing setPrice function to calculate the TWAP. This might involve fetching multiple price points within a defined time window and computing their average.

Remediation Plan: The Entangle team solved the issue by calculating price with synth amount.

Currently, the contracts does not adequately handle atypical ERC20 tokens on the staking. According to the ERC20 specification, tokens should return "false" when a transfer fails, but it does not guarantee that the function will revert. This discrepancy could potentially lead to silent failures, making it difficult to detect issues when they occur.

For instance, from the below, It can be seen that [LDO](https://etherscan.io/token/0x5a98fcbea516cf06857215779fd812ca3bef1b32#code) does not revert If the user balance is not enough for the transfer.

```
File: EntangleDexV2.sol

144: lpToken.approve(address(masterSynthChef), lpAmount);

```

```
File: SynthChefs/StargateSynthChef.sol

76: pools[poolId].LPToken.approve(address(lpStaking), type(uint256).max);

```


```
File: SynthChefs/WombatSynthChef.sol

320: womToken.transfer(ICompounderAddressResolver(masterChefEnt).compounder(), womRewards);

350: currentToken.transfer(ICompounderAddressResolver(masterChefEnt).compounder(), currentAmount);

```


```
File: interfaces/SynthChefs/Velocore/lib/Token.sol

242: tok.transferFrom(from, to, amount);

```

Consider using safeApprove & safeTransfer.

Remediation Plan: The Entangle team solved the issue by using safeApprove & safeTransfer.

In the EntangleDexV2 smart contract, a critical oversight has been identified in the sellForLP function. This function is intended to facilitate the selling of synthetic assets (synths) for LP (Liquidity Provider) tokens. However, the function lacks the necessary validation to check whether cross-chain operations are allowed for the specific synthetic asset (sid).
The contract already includes a modifier isCrossChainAllowed which is designed to perform this validation, but it is conspicuously absent in the sellForLP function. This oversight could potentially allow cross-chain synth transactions even when they are not permitted.

    function sellForLP(
        uint128 sid,
        uint256 synthAmount,
        address recipient
    ) public isPausedSID(sid) onlyLpStakingSid(sid) nonReentrant {
        if(synthAmount == 0) revert EntangleDex__E5();
        synthFactory.burn(sid, synthAmount, msg.sender, 0);
        uint price = synthFactory.getPrice(sid);
        uint lpAmount = (synthAmount * price) / 10 ** 18;
        if (block.chainid == SidLibrary.chainId(sid)) {
            masterSynthChef.withdrawLP(sid, lpAmount, recipient);
        } else {
            emit LPStackingSynthSell(sid, lpAmount, synthAmount, recipient);
            balanceManager.proposeSellForLp(sid, synthAmount, recipient);
        }
    }

Implement the isCrossChainAllowed modifier within the sellForLP function to ensure that only synths that are explicitly allowed for cross-chain operations can be processed through this function.

Remediation Plan: The Entangle team solved the issue by deprecating the feature.

Some tokens do not correctly implement the EIP20 standard, and their approve function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. Tokens that do not correctly implement the latest EIP20 spec, like USDT on Ethereum, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value.

Some tokens also require that the allowance be set to 0 before issuing a new approve call. Calling the approve function when the allowance is not zero reverts the transaction with these types of tokens.

https://github.com/Entangle-Protocol/entangle-lsd-protocol/blob/48e025e4ddf330e8ac710e82114db704a76d2857/contracts/EntangleDexV2.sol#L144

   function buyForLP(uint128 sid, uint lpAmount) public onlyLpStakingSid(sid) onlyHomeChain(sid) isPausedSID(sid) nonReentrant {
       if (lpAmount == 0) revert EntangleDex__E5();
       IERC20 lpToken = IERC20(masterSynthChef.getLpToken(sid));
       lpToken.safeTransferFrom(msg.sender, address(this), lpAmount);
       lpToken.approve(address(masterSynthChef), lpAmount);
       masterSynthChef.depositLP(sid, lpAmount);
       uint price = synthFactory.getPrice(sid);
       uint amount = (lpAmount * 10 ** 18) / price;
       synthFactory.mint(sid, amount, msg.sender, 0);
   }

It is recommended to use OpenZeppelin’s SafeERC20 and the forceApprove() function or safeApprove() with 0 first to also handle non-standard-compliant tokens.

Remediation Plan: The Entangle team solved the issue by approving with 0.

In the smart contract function proposeSetPrice, there is a critical oversight regarding the validation of data freshness, particularly concerning the prices derived from external sources or calculations based on potentially outdated state information. The function calculates a new price based on the lpTokenFarmBalance and synthTotalSupplyAllChains without performing any checks to ensure that these values are current and not stale. Price staleness can occur due to delays in data updates, network congestion, or failures in external dependencies, leading to the use of outdated information for crucial financial calculations.

    function proposeSetPrice(uint128 sid) external onlyRole(MASTER) {
        console.log("OracleBalanceSpotter: proposeSetPrice");
        (, StateParamsLibrary.EChefStateParams memory chefState) = stateSpotter.getChefState(sid);

        uint256 lpTokenFarmBalance = chefState.lpTokenFarmBalance;
        console.log("lpTokenFarmBalance = %d", lpTokenFarmBalance);
        uint256 synthTotalSupplyAllChains = stateSpotter.getSynthTotalSupplyAllChains(sid);

        console.log("synthTotalSupplyAllChains = %d", synthTotalSupplyAllChains);
        uint opId = newOperation("proposeSetPrice");

        uint256 price = (lpTokenFarmBalance * 10 ** 18) / synthTotalSupplyAllChains;
        console.log("price = %d", price);

        if (lpTokenFarmBalance == 0 || synthTotalSupplyAllChains == 0) {
            console.log("Set failsafe price");
            price = 10 ** 18;
            return;
        }

        bytes4 setPriceSelector = bytes4(keccak256("setPrice(bytes)"));
        for (uint i = 0; i < stateSpotter.getChainsLength(); i++) {
            uint64 chainId = stateSpotter.chains(i);

            // TODO: !!! apply new price in SynthState on all chains
            stateSpotter.setPrice(chainId, sid, price);
            proposer.propose(
                protocolId,
                chainId,
                chainInfo[chainId].balanceManager,
                setPriceSelector,
                abi.encode(sid, price, opId)
            );
        }
    }

Introduce mechanisms to verify the freshness of the data used in price calculations. This could involve timestamp checks, versioning of data points, or validation against a known recent block height to ensure that the data used is up-to-date.

Remediation Plan: The issue is marked as “Not Applicable” due to the related code-base is deprecated.

The Entangle LSD Protocol's package.json references an outdated version of OpenZeppelin's @openzeppelin/contracts-upgradeable package (^4.8.2). As of the latest release (v4.9.5 on Dec 8, 2023), several improvements and critical security patches may not be present in the currently utilized version. Using outdated dependencies, especially those related to security and contract upgrades, poses a significant risk to the protocol's security, maintainability, and overall functionality.

    "devDependencies": {
        "@nomicfoundation/hardhat-chai-matchers": "^1.0.3",
        "@nomicfoundation/hardhat-ethers": "^3.0.4",
        "@nomicfoundation/hardhat-toolbox": "^1.0.2",
        "@nomiclabs/hardhat-ethers": "^2.0.6",
        "@nomiclabs/hardhat-etherscan": "^3.1.0",
        "@nomiclabs/hardhat-ganache": "^2.0.1",
        "@openzeppelin/contracts-upgradeable": "^4.8.2",
}

Update the @openzeppelin/contracts-upgradeable dependency to the latest version (v4.9.5). Ensure that all contract imports are adjusted accordingly to accommodate any changes in the library's API.

Remediation Plan : The Entangle team solved the issue by updating dependency.

The SidLibrary library is designed to perform operations on a uint128 SID (Security Identifier) by extracting specific components such as chain ID, protocol ID, and pool ID from the SID. However, the library defines constants MAXUINT64 and MAXUINT32 with bit lengths that are incompatible with the operations performed on a uint128 SID variable. The constants are used to mask and shift bits within the SID to extract parts of the identifier, but the MAXUINT64 constant, in particular, is used under the assumption that it represents a uint128 maximum value for 64 bits, which is not the case due to its definition as a uint128 with only 64 significant bits set.

//SPDX-License-Identifier: BSL 1.1
pragma solidity ^0.8.15;

library SidLibrary {
    uint128 constant MAXUINT64 = 0xFFFFFFFFFFFFFFFF;
    uint128 constant MAXUINT32 = 0xFFFFFFFF;

    function chainId(uint128 SID) internal pure returns (uint64) {
        return uint64((SID & (MAXUINT64 << 64)) >> 64);
    }

    function protocolId(uint128 SID) internal pure returns (uint32) {
        return uint32((SID & (MAXUINT32 << 32)) >> 32);
    }

    function poolId(uint128 SID) internal pure returns (uint32) {
        return uint32(SID & MAXUINT32);
    }

    function isLpStackingSynth(uint128 SID) internal pure returns (bool) {
        return (protocolId(SID) & (1 << 31)) != 0 ? true : false;
    }

    function unpack(uint128 SID) internal pure returns (uint64, uint32, uint32) {
        return (chainId(SID), protocolId(SID), poolId(SID));
    }
}

Update the library to use correct bit lengths and operations that align with the uint128 SID size. This includes redefining constants with uint128 in mind and ensuring bit operations correctly handle the 128-bit size.

Remediation Plan : The Entangle team has partially solved the issue by changing the variable type to uint64.

The contracts are upgradable, inheriting from the Initializable contract. However, the current implementations are missing the _disableInitializers() function call in the constructors. Thus, an attacker can initialize the implementation. Usually, the initialized implementation has no direct impact on the proxy itself; however, it can be exploited in a phishing attack. In rare cases, the implementation might be mutable and may have an impact on the proxy.

It is recommended to call [_disableInitializers](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/utils/Initializable.sol#L145) within the contract's constructor to prevent the implementation from being initialized.

Remediation Plan : The Entangle team has partially solved the issue by adding [_disableInitializers](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/utils/Initializable.sol#L145). However, SynthChefs are still missing function call.

The contracts, which are designed to be upgradeable using the UUPS pattern, seem to be missing proper initialization functions. The initial setup for a UUPS upgradeable contract is crucial to ensure that the contract can be safely upgraded in the future without losing state or introducing vulnerabilities.

To address this concern, ensure that all base contracts are properly initialized in the initialize function.

Remediation Plan: The Entangle team solved the issue.

It was identified that the several contracts assume that the safeTransferFrom() and safeTransfer() calls transfer the full amount of tokens. This may not be true if the tokens being transferred are fee-on-transfer tokens, causing the received amount to be lesser than the accounted amount. For example, DGX (Digix Gold Token) and CGT (CACHE Gold) tokens apply transfer fees, and the USDT (Tether) token also has a currently disabled fee feature.

It was also identified that the contract assumes that its token balance does not change over time without any token transfers, which may not be true if the tokens being transferred were deflationary/rebasing tokens. For example, the supply of AMPL (Ampleforth) tokens automatically increases or decreases every 24 hours to maintain the AMPL target price.

In these cases, the contracts may not have the full token amounts, and the associated functions may revert.

It is recommended to get the exact received amount of the tokens being transferred by calculating the difference of the token balance before and after the transfer and using it to update all the variables correctly.

It is also recommended that all tokens are thoroughly checked and tested before they are used to avoid tokens that are incompatible with the contracts.

Remediation Plan: The issue is marked as “Not Applicable” due to the related code-base is deprecated.

The proposeSetPrice function within the contract calculates a price based on the lpTokenFarmBalance and synthTotalSupplyAllChains. However, the division operation (lpTokenFarmBalance * 10 ** 18) / synthTotalSupplyAllChains is performed without ensuring that synthTotalSupplyAllChains is not zero. While there is a conditional check after the division operation to set a failsafe price if either lpTokenFarmBalance or synthTotalSupplyAllChains is zero, this check is misplaced and should occur before the division to prevent a potential division by zero error.

    function proposeSetPrice(uint128 sid) external onlyRole(MASTER) {
        console.log("OracleBalanceSpotter: proposeSetPrice");
        (, StateParamsLibrary.EChefStateParams memory chefState) = stateSpotter.getChefState(sid);

        uint256 lpTokenFarmBalance = chefState.lpTokenFarmBalance;
        console.log("lpTokenFarmBalance = %d", lpTokenFarmBalance);
        uint256 synthTotalSupplyAllChains = stateSpotter.getSynthTotalSupplyAllChains(sid);

        console.log("synthTotalSupplyAllChains = %d", synthTotalSupplyAllChains);
        uint opId = newOperation("proposeSetPrice");

        uint256 price = (lpTokenFarmBalance * 10 ** 18) / synthTotalSupplyAllChains;
        console.log("price = %d", price);

        if (lpTokenFarmBalance == 0 || synthTotalSupplyAllChains == 0) {
            console.log("Set failsafe price");
            price = 10 ** 18;
            return;
        }

        bytes4 setPriceSelector = bytes4(keccak256("setPrice(bytes)"));
        for (uint i = 0; i < stateSpotter.getChainsLength(); i++) {
            uint64 chainId = stateSpotter.chains(i);

            // TODO: !!! apply new price in SynthState on all chains
            stateSpotter.setPrice(chainId, sid, price);
            proposer.propose(
                protocolId,
                chainId,
                chainInfo[chainId].balanceManager,
                setPriceSelector,
                abi.encode(sid, price, opId)
            );
        }
    }

To mitigate the risk of division by zero and ensure the robustness of the price calculation, the conditional check for zero values should be moved before the division operation. Additionally, consider setting a default or failsafe price more proactively to handle scenarios where lpTokenFarmBalance or synthTotalSupplyAllChains might be zero. Implementing these changes enhances the function's reliability and prevents potential disruptions.

Remediation Plan: The issue is marked as “Not Applicable” due to the related code-base is deprecated.

The DexWrapper contract provides functionality to swap tokens. A crucial parameter in this process is slippage, which defines the maximum price movement a user is willing to tolerate for their swap. Currently, the contract includes a setSlippage function that allows the contract owner to update the slippage value. However, the contract does not prevent setting the slippage to zero. Allowing slippage to be set to zero can lead to swaps failing due to normal price fluctuations or expose users to potential front-running attacks, where attackers can manipulate the price to their advantage knowing the user's swap will accept any amount of slippage.

    function setSlippage(uint256 _newSlippage) external onlyOwner {
        slippage = _newSlippage;
    }

To mitigate these risks, the DexWrapper contract should enforce a minimum slippage value. This can be achieved by adding a check in the setSlippage function to ensure that the slippage is not set to zero.

Remediation Plan: The issue is marked as “Not Applicable” due to the related code-base is deprecated.

The smart contract contains several setter functions (setPausedSid, setSynthFactory, setMasterSynthChef) that allow for critical updates to the contract's state, such as pausing a specific SID, updating the SynthFactory address, and setting a new MasterSynthChef. However, these functions do not emit events upon successful state changes. The absence of event emissions in these setter functions can significantly impact the transparency and auditability of the contract, making it challenging to track changes and updates in a decentralized and trustless manner.

    function setPausedSid(uint128 sid, bool enabled) public onlyRole(ADMIN_ROLE) {
        sidInfo[sid].isPausedSid = enabled;
    }

    function setSynthFactory(SynthFactory _synthFactory) public onlyRole(ADMIN_ROLE) {
        synthFactory = _synthFactory;
    }

    function setMasterSynthChef(MasterSynthChef _masterChef) public onlyRole(ADMIN_ROLE) {
        masterSynthChef = _masterChef;
    }

Define and emit specific events within each setter function to log the changes.

Remediation Plan: The Entangle team acknowledged the issue.

Several open TODO comments are identified in the contracts, indicating that their development is not finished.

./NglToken/BridgeRouter.sol:50:    /// @dev Calculates the bridge fee (TODO).
./NglToken/BridgeRouter.sol:52:        // TODO:
./test/SynthFactoryMock.sol:32:        // TODO: Translation
./oracle/OracleBalanceSpotter.sol:207:            // TODO: !!! apply new price in SynthState on all chains
./oracle/MasterSpotter.sol:48:        // TODO handle?
./SynthChefs/WombatSynthChef.sol:116:        // TODO: there will be some additional rewards after the last call
./MasterSynthChef.sol:35:    // TODO: probably not needed anymore...
./interfaces/SynthChefs/DODO/IDODOV2Pair.sol:36:    ) external returns (uint256 baseAmount, uint256 quoteAmount); // just a fucking meme TODO delete comment

It is recommended to review the open TODO comments to make sure before release all required development steps are finished.

Remediation Plan: The Entangle team acknowledged the issue.

The contracts are missing/incomplete natspec which affects readability and UX.

Consider adding Natspec to the functions/contracts.

Remediation Plan: The Entangle team acknowledged the issue.

The current ownership transfer process for all the contracts inheriting from Ownable or OwnableUpgradeable involves the current owner calling the [transferOwnership()](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.8/contracts/access/Ownable.sol#L69-L72) function:

function transferOwnership(address newOwner) public virtual onlyOwner {

    require(newOwner != address(0), "Ownable: new owner is the zero address");

    _setOwner(newOwner);

}

If the nominated EOA account is not a valid account, it is entirely possible that the owner may accidentally transfer ownership to an uncontrolled account, losing the access to all functions with the onlyOwner modifier.

It is recommended to implement a two-step process transfer ownership process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of the ownership to fully succeed. This ensures the nominated EOA account is a valid and active account. This can be easily achieved by using OpenZeppelin's [Ownable2Step contract](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.8/contracts/access/Ownable2Step.sol) instead of Ownable

Remediation Plan: The Entangle team acknowledged the issue.

The SynthFactory smart contract, as part of its design, incorporates role-based access control, leveraging the OpenZeppelin AccessControlUpgradeable for managing different roles such as ADMIN, MINTER, and PRICE_MANAGER. However, a potential issue arises with assigning and managing these roles, specifically the ADMIN role. Currently, the initialize function of the contract assigns the ADMIN and DEFAULT_ADMIN_ROLE roles to msg.sender (the address that deploys the contract). This straightforward setup could lead to centralized control and potential security risks, especially if the admin's private key is compromised. Moreover, the current implementation does not facilitate using multisig wallets for the ADMIN role, a common and more secure practice for administering key functions in mission-critical smart contracts.

https://github.com/Entangle-Protocol/entangle-lsd-protocol/blob/48e025e4ddf330e8ac710e82114db704a76d2857/contracts/SynthFactory.sol#L15

    function initialize(address _balanceManager) public initializer {
        __Ownable_init();
        __UUPSUpgradeable_init();
        _grantRole(ADMIN, msg.sender);
        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
        balanceManager = BalanceManager(_balanceManager);
    }

It is recommended to define the address variable as a function parameter.

Remediation Plan: The Entangle team acknowledged the issue.

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.

Instances (1):
```solidity
File: interfaces/SynthChefs/Thena/IMasterChef.sol

3: pragma solidity >=0.7.0 <0.9.0;

```

Consider locking the pragma version in the smart contracts. It is not recommended to use a floating pragma in production.

For example: pragma solidity 0.8.20;

Remediation Plan: The Entangle team solved the issue by deprecating the codebase.

When a loop iterates many times, it causes the amount of gas required to execute the function to increase significantly. In Solidity, excessive looping can cause a function to use more than the maximum allowed gas, which causes the function to fail.

``` File: Compounder.sol 21: for (uint i = 0; i < callConfigs.length; ++i) { 34: for (uint i = 0; i < underlyings.length; ++i) { ``` ``` File: NglToken/BaseEntangleToken.sol 87: for (uint256 i = 0; i < chainIds.length; ++i) { ``` ``` File: SynthChefs/CurveCompoundConvexSynthChef.sol 245: for (uint256 i = 0; i < pool.uTokens.length; i++) { 282: for (uint256 i; i < pool.uTokens.length; i++) { ``` ``` File: SynthChefs/FusionxSynthChef.sol 184: for (uint256 i = 0; i < rewardTokens.length; i++) { ``` ``` File: SynthChefs/FusionxSynthChefMainnet.sol 197: for (uint256 i = 0; i < rewards.length; ++i) { 205: for (uint256 i = 0; i < rewardTokens.length; i++) { ``` ``` File: SynthChefs/SpookySwapSynthChef.sol 212: for (uint i = 0; i < pool.poolTokens.length; i++) { ``` ``` File: SynthChefs/WombatSynthChef.sol 339: for (uint256 i = 0; i < additionalTokens.length; ++i) { ``` ``` File: dex-wrappers/DexWrapper.sol 82: for (uint i = 0; i < swapPath.length; ++i) { 119: for (uint256 i = 0; i < specifiedPaths.length; i++) { 184: for (uint256 i = 0; i < specifiedPaths.length; i++) { ``` ``` File: dex-wrappers/DodoDexWrapper.sol 130: for (uint i = 2; i < path.length; i++) { // skip first 2 addresses, because it is tokenFrom and tokenTo ``` ``` File: oracle/OracleStateSpotter.sol 197: chainIds[_chainId] = IdInfo(true, chains.length - 1); 227: sidIds[_sid] = IdInfo(true, sids.length - 1); 284: for (uint i = 0; i < chains.length; i++) { ```

To reduce gas consumption, it is recommended to find ways to optimize the loop or potentially break the loop into smaller batches. The following pattern can also be used:

uint256 cachedLen = array.length;

for(uint i; i < cachedLen;){

unchecked {

++i;

}

}

Remediation Plan: The Entangle team acknowledged the issue.

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table.

```

File: BalanceManager.sol

20: bytes32 public constant ADMIN = keccak256("ADMIN");

21: bytes32 public constant SPOTTER = keccak256("SPOTTER");

22: bytes32 public constant SELF = keccak256("SELF");

```

Consider defining variable as private for gas optimization.

Remediation Plan: The Entangle team acknowledged the issue.

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they’re hit by avoiding having to [allocate and store the revert string](https://blog.soliditylang.org/2021/04/21/custom-errors/#errors-in-depth). Not defining the strings also saves deployment gas.

Consider replacing all revert strings with custom errors.

Remediation Plan: The Entangle team acknowledged the issue.

Initialization to 0 or false is not necessary, as these are the default values in Solidity.

Remove the initialization values of 0 or false.

Remediation Plan: The Entangle team acknowledged the issue.

The codebase currently contains several instances of console.log statements. While these logging statements can be invaluable during development and debugging phases, leaving them in the production code can lead to unnecessary clutter.

Consider deleting console.log from the contracts.

Remediation Plan: The Entangle team acknowledged the issue.

The ATokenWrapper contract provides a withdraw function that facilitates the withdrawal of all deposited funds from the AAVE protocol. This function directly interacts with the AAVE pool to withdraw assets to the contract itself. However, the function lacks a feature: an access control mechanism, such as an onlyCompounder modifier,

// SPDX-License-Identifier: BSL 1.1
pragma solidity ^0.8.15;

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

interface IAavePool {
    function withdraw(address asset, uint256 amount, address to) external returns (uint256);
}

interface IAToken {
    function UNDERLYING_ASSET_ADDRESS() external view returns (address);
    function POOL() external view returns (address);
}

contract ATokenWrapper {
    using SafeERC20 for IERC20;

    function withdraw(address token) external {
        // withdraws all deposited funds
        IAavePool(IAToken(token).POOL()).withdraw(IAToken(token).UNDERLYING_ASSET_ADDRESS(), type(uint).max, address(this));
    }

}

Introduce an onlyCompounder modifier (or similarly named modifier) that restricts the execution of the withdraw function to addresses authorized to manage the contract's assets.

Remediation Plan: The Entangle team acknowledged the issue.

The import statement for hardhat/console.sol found in the Solidity contract is intended for development purposes, facilitating debugging during the development and testing phases by allowing developers to print variable values and contract states.

Delete the import statement for hardhat/console.sol from the contract file. This change ensures that debugging tools are not inadvertently included in the production deployment process.

Remediation Plan: The Entangle team acknowledged the issue.

The ATokenWrapper contract includes an import statement for @openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol, in addition to importing @openzeppelin/contracts/token/ERC20/IERC20.sol. Upon review of the contract's functionality, it is observed that the SafeERC20 library's functionality is not utilized within the contract's logic. The contract does not employ the safe methods provided by SafeERC20 for ERC20 token interactions, rendering this import unnecessary. This discrepancy introduces potential confusion regarding the contract's dependencies and suggests an opportunity to optimize the contract's code by removing unused imports.

Since the SafeERC20 library's methods are not utilized within the ATokenWrapper contract, it is recommended to remove this import to clean up the code and clarify the contract's actual dependencies.

Remediation Plan: The Entangle team acknowledged the issue.

The token contracts currently include the increaseAllowance and decreaseAllowance functions. While these functions provide utility, they may introduce additional risks and are not aligned with the core EIP-20 specifications.

Here are some concerns:

- The increaseAllowance and decreaseAllowance functions are not defined within the original EIP-20 specifications. Including them in the primary ERC20 contract deviates from the standard.

- There have been instances where unsuspecting users were tricked into signing malicious increaseAllowance payloads, leading to significant financial losses. One such incident occurred recently where a user lost $24 million due to a deceptive payload (See transaction : [0xcbe7b32e62c7](https://etherscan.io/tx/0xcbe7b32e62c7d931a28f747bba3a0afa7da95169fcf380ac2f7d54f3a2f77913)).

Consider removing increaseAllowance and decreaseAllowance from the primary token contracts. Instead, include them in an extension or auxiliary contract. This way, developers have the discretion to include these functions if they deem them necessary for their specific use case.

Remediation Plan: The Entangle team acknowledged the issue.

The contract contains a function, HandleLPStackingSynthSellAndWithdraw, which was designed to interact with the Synapse Bridge for executing liquidity pool (LP) stacking synth sell and withdrawal operations across chains. However, it has been identified that the SynapseBridge contract or its intended functionality has been deprecated or removed by the client, rendering this function obsolete. Continuing to include this function in the contract without the corresponding Synapse Bridge functionality can lead to confusion, potential security risks, and maintainability issues, as it references external dependencies and operations that no longer exist within the protocol.

    function HandleLPStackingSynthSellAndWithdraw(
        EntangleDexV2Events.LPStackingSynthSellAndWithdraw memory snw
    ) public onlyRole(MASTER) {
        //console.log("BalanceSpotter: HandleLPStackingSynthSellAndWithdraw");
        uint opId = newOperation("HandleLPStackingSynthSellAndWithdraw");

        uint64 chainId = SidLibrary.chainId(snw.sid);

        //console.log("BalanceSpotter: srcChainId %s, sid %s, lpAmount %s", chainId, snw.sid, snw.lpAmount);
        //console.log("BalanceSpotter: destChainId %s, recipient %s", snw.chainId, snw.recipient);

        SynapseBridgeParams memory bridgeParams = SynapseBridgeParams({
            to: snw.recipient,
            chainId: snw.chainID,
            NUSDToken: chainInfo[chainId].nusd,
            tokenIndexFrom: chainId == 250 ? 2 : 1, // token index 2 only on ftm
            tokenIndexTo: 0, // Synapse nusd
            dx: 0, // will set in balanceManager
            minDy: 0,
            deadline: type(uint64).max,
            swapTokenIndexFrom: 0, // Synapse nusd
            swapTokenIndexTo: snw.chainID == 250 ? 2 : 1,
            swapMinDy: 0,
            swapDeadline: type(uint64).max
        });

        BridgeParams memory bParamss = BridgeParams(0, abi.encode(bridgeParams));

        bytes4 withdrawLPAndBridgeSelector = bytes4(keccak256("withdrawLPFromChefAndBridge(bytes)"));
        proposer.propose(
            protocolId,
            chainId,
            chainInfo[chainId].balanceManager,
            withdrawLPAndBridgeSelector,
            abi.encode(snw.sid, snw.lpAmount, snw.synthAmount, bParamss, opId)
        );
    }

Delete the HandleLPStackingSynthSellAndWithdraw function from the contract to eliminate references to the non-existent Synapse Bridge functionality. This cleanup will help focus on the active features and reduce maintenance overhead.

Remediation Plan: The Entangle team acknowledged the issue.

The HandleBridgeExecuted function within the MasterSpotter contract is intended to handle events related to the execution of bridge operations. However, upon inspection, it is observed that the function merely decodes the event parameters without performing any subsequent actions or updates to the contract state. This lack of functionality suggests that the intended purpose of processing bridge execution events is not being fulfilled.

   function HandleBridgeExecuted(bytes calldata b) public onlyRole(SPOTTER) {
       (, , , , bytes memory eventParams) = abi.decode(b, (bytes32, uint256, uint256, bytes32, bytes));
       BalanceManagerEvents.BridgeExecuted memory bridgeExecuted = BalanceManagerEvents.DecodeBridgeExecuted(eventParams);
       return;
   }

Determine the original intent behind the HandleBridgeExecuted function. If it was meant to update the contract's state or perform specific actions based on bridge execution events, define the specific requirements and desired outcomes.

Remediation Plan: The Entangle team acknowledged the issue.

The MasterSpotter contract contains two functions, HandleWithdrawLpAndExtract and HandleWithdrawLP, which appear to serve similar purposes related to handling the withdrawal of liquidity provider (LP) tokens. Upon closer inspection, it is evident that both functions are designed to update the state of LP balances within the system based on withdrawal events. However, the existence of two separate functions for handling what essentially amounts to the same action introduces redundancy and potential confusion within the contract's design.

The presence of two functions with overlapping functionality increases the complexity of the contract, making it harder for developers and auditors to understand and maintain the codebase.

Review the contract's requirements and operational logic to determine whether both HandleWithdrawLpAndExtract and HandleWithdrawLP are genuinely needed. If their functionalities overlap significantly, consider retaining only one of the functions.

Remediation Plan: The Entangle team acknowledged the issue.

In the OracleStateSpotter contract, two functions named debug_ClearSynthState and debug_ClearChefState are included with the purpose of clearing state information for synth and chef states, respectively. These functions are marked as debug, but it is not explicitly stated whether they are intended for use in production environments or solely for testing/debugging purposes. The presence of such functions in a contract deployed to a live network raises concerns regarding their potential misuse or unintended consequences.

Clearly document the purpose of these debug functions. If they are intended for production use, explain their use cases and the safeguards in place to prevent misuse.

Remediation Plan: The Entangle team acknowledged the issue.

The StateParamsLibrary provides functionalities to decode various state parameters from bytes based on their version. However, there's an inconsistency observed in the version check across different decoding functions:

• Both decodeESynthStateParams and decodeEChefStateParams functions strictly require the version to be 1 (i.e., _version == 1), rejecting any other version.

• On the other hand, decodeEDexStateParams allows any version less than or equal to 1 (i.e., _version <= 1), accepting version 0 and 1.

This inconsistency in version checking can lead to potential issues in future updates or when interacting with data encoded with different versions, especially as the system evolves and new versions are introduced.

Align the version checking mechanism across all decoding functions.

Remediation Plan: The Entangle team acknowledged the issue.

Across various contracts in the codebase, including DexWrapper and others, there is an inconsistent application of OwnableUpgradeable and AccessControlUpgradeable from OpenZeppelin's upgradeable contracts suite. Both modules serve to manage access controls and permissions, yet their concurrent deployment within the same contracts can introduce redundancy and potential for confusion regarding the hierarchy of control and permissions. This duality in access control mechanisms could complicate the governance and operational security of the contracts, making it harder to reason about the authority required for executing sensitive functions.

The coexistence of OwnableUpgradeable and AccessControlUpgradeable complicates the contract's access control scheme, potentially leading to governance issues. It raises questions about the clear delineation of roles and permissions, especially in scenarios requiring precise control over contract operations. This setup might also increase the risk of errors in permission assignments and checks, potentially affecting the contract's security posture.

Assessing whether OwnableUpgradeable functionalities are strictly necessary or if they can be fully replaced by AccessControlUpgradeable's more granular permissions system.

Remediation Plan: The Entangle team acknowledged the issue.

In the SynthFactory contract, there's a specific implementation for retrieving the current chain ID using inline assembly. The function __chainId() directly accesses the chainid opcode to obtain the chain ID. While this approach is valid and works correctly, it introduces unnecessary complexity and direct assembly use where a higher-level, safer alternative exists. Starting from Solidity 0.8.0, the block.chainid global variable is available and provides a straightforward, built-in method to access the chain ID, mitigating potential risks associated with inline assembly and aligning with best practices for code clarity and security.

Replace the __chainId() function with direct usage of block.chainid to obtain the current chain ID. This change simplifies the contract, enhances code clarity, and leverages Solidity's built-in security features.

Remediation Plan: The Entangle team acknowledged the issue.

Several contracts within the system lack necessary validations to prevent the assignment of the zero address (0x0) in their setter functions, specifically in setBalanceManager and setFeeCollector methods. This oversight could lead to issues, where critical functionalities could be disabled or misdirected to an address that is not usable, resulting in loss of control over the contract's intended behaviors. The absence of such checks increases the risk of accidental or malicious misconfiguration, potentially leading to loss of funds or access control.

It is recommended to implement checks within the setter functions to ensure that the address being set is not the zero address.

Remediation Plan: The Entangle team acknowledged the issue.