Tendency - Strict Allowance Check Could Brick a Major Functionality in `OCL_ZVE`

[sherlock-admin4]

Tendency

medium

Strict Allowance Check Could Brick a Major Functionality in OCL_ZVE

Summary

In OCL_ZVE contract, when pulling liquidity from the DAO into a $ZVE/pairAsset pool, the assumption that both assets allowance given to the router will be used up, will cause a DoS for the pushToLockerMulti function when the pool reserves are unbalanced.

Vulnerability Detail

OCL_ZVE::pushToLockerMulti function is meant to be called on by the DAO to add liquidity into a $ZVE/pairAsset pool. After pulling the pair asset and ZVE into the contract, an allowance that is up to the balance of these assets in the contract is given to the router.

        uint balPairAsset = IERC20(pairAsset).balanceOf(address(this));
        uint balZVE = IERC20(ZVE).balanceOf(address(this));
        IERC20(pairAsset).safeIncreaseAllowance(router, balPairAsset);
        IERC20(ZVE).safeIncreaseAllowance(router, balZVE);

Where the router here could be a uniswap or sushi router.

The problem here is, after adding liquidity to the pool, the contract does this check:

       assert(IERC20(pairAsset).allowance(address(this), router) == 0);
        assert(IERC20(ZVE).allowance(address(this), router) == 0);

Which is to ensure that the router no longer has any allowance.

But when we look at uniRouter::addLiquidity function. When the reserves aren't zero, the input amount wouldn't be the amount pulled in, but the amount computed here:

    function quote(uint amountA, uint reserveA, uint reserveB) internal pure returns (uint amountB) {
        require(amountA > 0, 'UniswapV2Library: INSUFFICIENT_AMOUNT');
        require(reserveA > 0 && reserveB > 0, 'UniswapV2Library: INSUFFICIENT_LIQUIDITY');
        amountB = amountA.mul(reserveB) / reserveA;
    }

If the reserves aren't the same, one of the assets transferred from OCL_ZVE contract into the pool, will be less than the given allowance, thus making zero asset allowance check panic.

Note that, even if the reserves are the same, an attacker can frontrun the Dao transaction, by first making a direct asset transfer to the pool, and then add liquidity into the pool afterwards. This is to make sure the pool is updated to store the new pool tokens balance as reserves during IUniswapV2Pair::mint call.

Impact

OCL_ZVE::pushToLockerMulti function can be DoSed, hence uncallable by the DAO contract.

Code Snippet

https://github.com/sherlock-audit/2024-03-zivoe/blob/d4111645b19a1ad3ccc899bea073b6f19be04ccd/zivoe-core-foundry/src/lockers/OCL/OCL_ZVE.sol#L172-L215

Tool used

Manual Review

Recommendation

Should check if the router still has an allowance for either the pair asset or ZVE, if yes, then decrease its allowance to zero. i.e.:

        if (IERC20(pairAsset).allowance(address(this), router) > 0) {
            IERC20(pairAsset).safeDecreaseAllowance(router, 0);
        }
        if (IERC20(ZVE).allowance(address(this), router) > 0) {
            IERC20(ZVE).safeDecreaseAllowance(router, 0);
        }

Should also consider sending the leftover balance back to the DAO, else more yield than expected will be sent when forwarding yield, as the balance of the pair asset is used:

uint balPairAsset = IERC20(pairAsset).balanceOf(address(this));

https://github.com/sherlock-audit/2024-03-zivoe/blob/d4111645b19a1ad3ccc899bea073b6f19be04ccd/zivoe-core-foundry/src/lockers/OCL/OCL_ZVE.sol#L311-L330

Duplicate of #18

[sherlock-admin4]

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

panprog commented:

medium, dup of #18. While amounts provided are from the trusted admin, who is supposed to provide exact amounts to add liquidity in correct ratio of token amounts, the fact that live balance of the contract is used makes it possible for the balance to change from the time admin provides the amounts, making the transaction revert. Additionally, current contract balances might be in so high disproportion, that admin will simply not have enough funds to create correct ratio to add liquidity.