Dex Finance security record

When a user exits the ETF, it is possible for the user to obtain more base tokens than expected if the base token is not the first token in the `_tokens` array. In the function `exit`, it will swap all tokens to the base token and transfer the base token to the user: ```solidity function exit( uint256 amountIn, uint256 minAmountOut ) external nonReentrant returns (uint256 amountOut) { if (needForRebalance()) rebalance(); uint256 syntheticSupply = totalSupply(); uint256 exitFeeAmount = isFeeWhitelistWallet(msg.sender) ? 0 : amountIn * exitFee / DIVIDER; _burn(msg.sender, amountIn); if (exitFeeAmount > 0) { _mint(treasury, exitFeeAmount); amountIn -= exitFeeAmount; } for (uint256 i = 0; i < _tokens.length(); i++) { address token_ = _tokens.at(i); uint256 tokenAmountOut = amountIn * IERC20(token_).balanceOf(address(this)) / syntheticSupply; if (token_ == base) amountOut += tokenAmountOut; else { IERC20(token_).approve(router, tokenAmountOut); uint256 baseAmountOut = _swapTokenToBase(token_, tokenAmountOut, address(this)); amountOut += baseAmountOut; } } require(amountOut >= minAmountOut, "DexFiETF: AmountOut underflow"); IERC20(base).safeTransfer(msg.sender, amountOut); emit Exited(msg.sender, amountIn, amountOut, exitFeeAmount); } ``` The issue is that it depends on the order of the token list, the first token in the token list should be the base token, otherwise, the calculation for the `amountOut` will be bigger. Here is how it works: 1. Consider the eft has 100 USDT and 100 DAI and total shares is 200. The base token is DAI and the token list is [USDT,DAI] 2. Alice has 100 shares and she wants to exit all shares. 3. In the first loop, the token is USDT, the `tokenAmountOut` is `100*100/200 = 50`, it will be swapped to the 50 DAI, now the etf has 150 DAI and currently the `amountOut` is 50 4. In the second loop, the token is DAI, now the `tokenAmountOut` is `100*(100+50)/200 = 75`, so the `amountOut` will be `50+75=125` 5. Alice should have got 100 DAI but got 125 DAI instead ### Proof Of Concept We write a test case in `update-etf-config.test.ts`. We first exit all shares and update the token list and then we simulate the above scenario: ```javascript it('Change the order of the token list and exit, should success',async()=>{ let shares = await etf.balanceOf(acc.alice); await etf.exit(shares,1,{from:acc.alice}); console.log("=====change the order of the token list"); await etf.updateETFConfiguration({ base: exchange.usdc.address, maxDeviationPercent: 1e3, tokenConfigurations: [ { token: exchange.wNat.address, weight: 5e3, intermediaries: [], }, { token: exchange.usdc.address, weight: 5e3, intermediaries: [], } ] }); console.log("now the base token is: %s",exchange.usdc.address); let tokenList = await etf.tokensList(0,2); console.log("now the token list is: %s,%s",tokenList[0],tokenList[1]); console.log("we can see the first token in token list is not the base token"); console.log("alice join 100 usdc====>"); await etf.api.mintAndJoin(100e18, { from: acc.alice }); console.log("Bob join 100 usdc====>"); await etf.api.mintAndJoin(100e18, { from: acc.bob }); let wNatBalance = await exchange.wNat.balanceOf(etf.address); console.log("etf has %d wNat",wNatBalance); let usdcBalance = await exchange.usdc.balanceOf(etf.address); console.log("etf has %d usdc",usdcBalance); let total = await etf.totalSupply(); console.log("eft totalSupply is %d",total); let shares2 = await etf.balanceOf(acc.alice); let aliceUsdc = await exchange.usdc.balanceOf(acc.alice); console.log("alice has %d usdc now",aliceUsdc); await etf.exit(shares2,1,{from:acc.alice}); let aliceUsdc2 = await exchange.usdc.balanceOf(acc.alice); let aliceHasUsdc = Number(aliceUsdc2.toString()) - Number(aliceUsdc.toString()); console.log("alice has %d usdc after exit",aliceUsdc2); console.log("alice should have got 100e18 usdc, but got %d usdc",aliceHasUsdc); let diff = aliceHasUsdc - 100e18; console.log("alice got an extra %d usdc",diff); }); ``` ### Test result ```javascript Base replaced When total supply > 0 ✔ should fail =====change the order of the token list now the base token is: 0x50E23eCF93C418C547979bECB45C369c80050961 now the token list is: 0x46FBc8235001656d164CcC07Adf9306D0C908d74,

It is possible for users to withdraw more than expected if a farm's staking token is the wrapped native token. When users withdraw from the vault, tokens are unstaked from each farm and swapped to wrapped native tokens. ```solidity=124 function _withdrawAndSwapToWNative(uint256 syntheticAmount) internal returns (uint256 wnativeAmount) { uint256 syntheticTotalSupply = _totalSupply(); if (syntheticTotalSupply > 0) { for (uint256 i = 0; i < farms.length; i++) { VaultFarm storage farm_ = farms[i]; Farm memory farmInfo_ = factory.farms(farm_.pool, farm_.pid); uint256 amount = farm_.connector.userInfo(farm_.pid, address(this)).amount; amount = amount * syntheticAmount / syntheticTotalSupply; if (amount > 0) { farm_.connector.withdraw(farm_.pid, amount); wnativeAmount += _swapStakingOrRewardTokenToWNative( farmInfo_.dex, farmInfo_.stakingToken, IERC20Upgradeable(farmInfo_.stakingToken).balanceOf(address(this)), farmInfo_.stakingTokenInfo ); ``` When swapping the staking token to wrapped native, the amount swapped is `stakingToken.balanceOf(address(this))`. In the situation where the staking token is also the wrapped native, `stakingToken.balanceOf(address(this))` will include the amount unstaked as well as all previous unstaked amounts that were swapped. This will inflate the `wnativeAmount` value, possibly allowing the user to withdraw more than expected, such as in the event of a large harvest.

## Vulnerabilities When Using `remove` in A loop In `EnumerableSet`, the `remove` function will swap the element to delete with the last one in the array and then remove the last element. This function will decrease the length of the array by 1. When our loop depends on the length of the array, a call to `remove` in the loop will result in only half of the elements being removed, for example: ```solidity for (uint256 i = 0; i < _receivers.length(); i++) _receivers.remove(_receivers.at(i)); ``` If the `_receivers` is `[addressA,addressB]`, after the loop, the `_receivers` will be `[addressB]` instead of `[]`. ### Proof Of Concept For better testing, we first add a new contract: ```solidity pragma solidity 0.8.17; import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; contract MockReceiverSet{ using EnumerableSet for EnumerableSet.AddressSet; EnumerableSet.AddressSet private _receivers; function addReveiver(address receiver)external{ _receivers.add(receiver); } function removeAll()external{ for (uint256 i = 0; i < _receivers.length(); i++) _receivers.remove(_receivers.at(i)); } function receiversList() external view returns (address[] memory output) { uint256 receiversLength = _receivers.length(); output = new address[](receiversLength); for (uint256 i = 0; i < receiversLength; i++) output[i] = _receivers.at(i); } } ``` and then we will add two addresses to the set, print the set, remove all the addresses, and print again: ```javascript describe("test MockReceiverSet",()=>{ it("should success",async()=>{ let MockReceiverSet = artifacts.require("MockReceiverSet"); let instance = await MockReceiverSet.new(); await instance.addReveiver("0x55d398326f99059fF775485246999027B3197955"); await instance.addReveiver("0x6d2e03b7EfFEae98BD302A9F836D0d6Ab0002766"); let list = await instance.receiversList(); console.log(list); await instance.removeAll(); let list2 = await instance.receiversList(); console.log(list2); }) }) ``` ### Test result ```javascript test MockReceiverSet [ '0x55d398326f99059fF775485246999027B3197955', '0x6d2e03b7EfFEae98BD302A9F836D0d6Ab0002766' ] [ '0x6d2e03b7EfFEae98BD302A9F836D0d6Ab0002766' ] ``` Here we can see that after calling `removeAll`, there is an address that still stays in the set. ## Potential Vulnerabilities in the codebase ### DexFiTreasury._updateFeeReceiversInfo() In function `_updateFeeReceiversInfo`, there is a call to `remove` in the loop: ```solidity function _updateFeeReceiversInfo(FeeReceiverInfo[] memory _info) private { for (uint256 i = 0; i < _receivers.length(); i++) _receivers.remove(_receivers.at(i)); uint256 sumOfPercent = 0; for (uint256 i = 0; i < _info.length; i++) { require(_info[i].receiver != address(0), "DexFiTreasury: Receiver's address is zero"); _receivers.add(_info[i].receiver); _percents[_info[i].receiver] = _info[i].percent; sumOfPercent += _info[i].percent; } require(sumOfPercent == DIVIDER, "DexFiTreasury: Receiver's persent's sum neq DIVIDER"); require(_receivers.length() == _info.length, "DexFiTreasury: Info is incorrect"); emit FeeReceiversInfoUpdated(_info); } ``` This `for` loop will remove the first half of receivers instead of all of them. It's possible to fail to run this function, for example, if the first receivers are [userA, userB], then we update with [userC, userD], and after the first for loop, the receivers will be [userB], after the second for loop, the receivers will be [userB, userC, userD]. At this time, the length of `_receivers` is 3 and the length of `_info` is 2 so that the require statement `require(_receivers.length() == _info.length, "DexFiTreasury: Info is incorrect");` will revert. It is also possible to have receivers whose total percentages exceed 100%. For example, if the first receivers are [userA, userB] with associated percents [50, 50] and then receivers are updated with receivers [userA, userA] and percentages [25, 75], then the new receivers array will be [userB, userA] with percentages [50, 75]. ### ETFConfigurationManagement._updateETFConfiguration() In the function `_updateETFConfiguration`, there is a call to `remove` in the loop: ```solidity for (uint256 i = 0; i < _tokens.length(); i++) { address currentToken = _tokens.at(i); uint256 currentBalance = IERC20(currentToken).balanceOf(address(this)); bool isTokenInNewSet = false; for (uint256 j = 0; j < configurationsLength; j++) { if (currentToken == configuration_.tokenConfigurations[j].token) { isTokenInNewSet = true; break; } } if (!isTokenInNewSet) { IERC20(currentToken).approve(router, currentBalance); _swapTok

In the contract `DexFiETFZapper`, the `joinMulti` function can help a user join several tokens, but it will not join the `etfBase` token. ```solidity function joinMulti( address etf, Source[] memory sources, uint256 minAmountOut ) external nonReentrant returns (uint256 amountOut) { address etfBase = IDexFiETF(etf).base(); uint256 baseAmount = 0; for (uint256 i = 0; i < sources.length; i++) { Source memory source_ = sources[i]; address[] memory path = source_.path; address to = path[path.length - 1]; require(source_.amountIn > 0, "DexFiETFZapper: Amount is zero"); require(to == etfBase, "DexFiETFZapper: Last path element neq etf base"); if (path[0] != to) baseAmount += _swap(path, source_.amountIn, address(this), true); } amountOut = _join(etf, etfBase, baseAmount, minAmountOut); IERC20(etf).safeTransfer(msg.sender, amountOut); } ``` Users can pass several sources and each token in the source will first be swapped to the `etfBase` and then be joined to the `eft`. The issue is that when the source token is the `etfBase` itself, which means `path[0] == to`, the amount of tokens is not accrued to `baseAmount` and these tokens will not be joined to the `etf`. ### Proof Of Concept To simplify this test case, we will join only one token, the `etfBase` token, and then look at the change in the share of etf held by the user before and after calling the `joinMulti` function. We add the following code into the join `describe` in `end-to-end.test.ts` ```javascript it('join with zapper', async () => { txConf = { from }; let zapper = artifacts.require("DexFiETFZapper"); let ownerSource = await etf.ownerSource(); let router = await etf.router(); let base = await etf.base(); let zapperInstance = await zapper.new(ownerSource,router,txConf); let beforeBalance = await etf.balanceOf(from); console.log("user has %d eft shares before calling joinMulti",beforeBalance); await zapperInstance.joinMulti(etf.address,[{amountIn:100000,path:[base]}],0,txConf); let afterBalance = await etf.balanceOf(from); console.log("user has %d eft shares after calling joinMulti",afterBalance); }); ``` ## Test result ```javascript user has 950000000000000000 eft shares before calling joinMulti user has 950000000000000000 eft shares after calling joinMulti ``` We can see that the shares that the user held didn't change after calling the `joinMulti` function.

In the `DexFiETF` contract, the target proportion of each token is fixed. When the proportion of tokens changes, it can be rebalanced by calling the `rebalance` function. In the `rebalance` function, the over-proportioned tokens are first swapped to the base token and then the base token will be swapped to under-proportioned tokens, thus achieving rebalance. The token swapping is subject to a fee, which is paid by the `DexFiETF` contract. Therefore, each time the rebalance function is called, it results in a small fee that is paid by the `DexFiETF` contract. Since the `rebalance` function is a public function, an attacker can keep calling the `rebalance` function, thus making the `DexFiETF` contract keep paying the fees and lose more money than if `rebalance` was only called once. ### Proof Of Concept: We will first pay 100 ether to join the EFT, and then call `rebalance` 10 times. We will print the fund loss after each `rebalance` call: ```javascript describe('Join', async () => { it('should succeed', async () => { await etf.api.mintAndJoin(100e18, txConf); }); it('weights should equal', async () => { await assertBalancesInBase(etf); }); }); describe('Rebalance', () => { it('should succeed', async () => { for(let i=0;i<10;i++){ let beforeVaule = await etf.calculationBaseForAllTokens(); console.log("====%d====beforeVaule: %s",i,beforeVaule.toString()); await etf.rebalance(); let afterVaule = await etf.calculationBaseForAllTokens(); console.log("====%d====afterVaule: %s",i,afterVaule.toString()); let before = Number(beforeVaule.toString()); let after = Number(afterVaule.toString()); if(before>=after){ console.log("====%d====the loss after rebalance: %s",i,before - after); } } }); it('weights should equal', async () => { await assertBalancesInBase(etf); }); }); ``` ### Test Result: ```solidity ====0====beforeVaule: 99615393510008363518 ====0====afterVaule: 99614633238796485563 ====0====the loss after rebalance: 760271211872256 ====1====beforeVaule: 99614633238796485563 ====1====afterVaule: 99614631689337250591 ====1====the loss after rebalance: 1549459226624 ====2====beforeVaule: 99614631689337250591 ====2====afterVaule: 99614631686246680815 ====2====the loss after rebalance: 3090579456 ====3====beforeVaule: 99614631686246680815 ====3====afterVaule: 99614631686241404416 ====3====the loss after rebalance: 5275648 ====4====beforeVaule: 99614631686241404416 ====4====afterVaule: 99614631686241393532 ====4====the loss after rebalance: 16384 ====5====beforeVaule: 99614631686241393532 ====5====afterVaule: 99614631686241393488 ====5====the loss after rebalance: 0 Rebalance should succeed: Error: Returned error: VM Exception while processing transaction: revert PancakeRouter: INSUFFICIENT_OUTPUT_AMOUNT -- Reason given: PancakeRouter: INSUFFICIENT_OUTPUT_AMOUNT. at Context.<anonymous> (dist/test/ETF/end-to-end.test.js:68:27) at runMicrotasks (<anonymous>) at processTicksAndRejections (node:internal/process/task_queues:96:5) ``` With each iteration, the fund loss becomes less and less until it finally becomes zero, so the total fund loss is fixed. After the fund loss becomes zero, calling `rebalance` will fail.

The functions which are never called internally within the contract should have external visibility for gas optimization. In `DexShareRewardPoolConnector`: ```solidity=54 function deposit( ``` --- ```solidity=67 function withdraw( ``` In `PancakeMasterPoolConnector`: --- ```solidity=56 function deposit( ``` --- ```solidity=69 function withdraw( ```

The below code calculates the total balance twice in the `needForRebalance` function, which is unnecessary. ```solidity if (calculationBaseForAllTokens() == 0) return false; uint256 len = _tokens.length(); uint256 currentTotalBalanceInBase = calculationBaseForAllTokens(); ... ... ``` The above code can be simplified as below to save gas: ```solidity uint256 currentTotalBalanceInBase = calculationBaseForAllTokens(); if (currentTotalBalanceInBase == 0) return false; uint256 len = _tokens.length(); ... ... ```

In the internal `_harvestAndSwapToWNative()` function, the `reinvestAmount` assets can only be transferred under the `reinvestAmount > 0` condition. However, the `reinvestAmount` variable has already been decreased by the harvest fee and profit. If the harvest fee and profit add up to 100% of `reinvestAmount`, then the fee and profit will not be transferred. If there is a call to `_depositAll()` after this function, these `reinvestAmount` assets will be converted to staking tokens and transferred to the farm contract.

`DexFiSwapTokenToToken` and `VaultTokenSwaps` will call `swapExactTokensForTokens` to swap tokens. The issue is that this function doesn't support tokens that charge fees. For example, when swapping standard ERC20 deflationary tokens, the input amount will not be equal to the received amount due to the charged transaction fee. As a result, an inconsistency in the amount will occur and the swap may fail due to validation checks. The same issue exists in the contract `ETFConfigurationManagement` and `ETFZapperCalculationsUtilities`.

In the `DexFiVault` contract, the `updateVault()` function calls the `_harvestAndSwapToWNative()` to perform harvesting and swapping. However, the `_depositAll()` function only can be invoked under the `farms_.length > 0` condition, the `_harvestAndSwapToWNative()` function probably causes some assets to remain in the vault until the user or owner later triggers the `_depositAll()` to deal with those assets.