use `safeApprove` instead of `safeDecreaseAllowance` when removing allowance inside DAO

Low/Info issue submitted by saidam017

Summary

After DAO push tokens to lockers, it will try to remove all the approval to 0 but wrongly use safeDecreaseAllowance instead of safeApprove.

Vulnerability Detail

It can be observed that when removing allowance to 0, it use safeDecreaseAllowance instead of safeApprove.

https://github.com/sherlock-audit/2024-03-zivoe/blob/main/zivoe-core-foundry/src/ZivoeDAO.sol#L239-L247

    function push(address locker, address asset, uint256 amount, bytes calldata data) external onlyOwner nonReentrant {
        require(IZivoeGlobals_DAO(GBL).isLocker(locker), "ZivoeDAO::push() !IZivoeGlobals_DAO(GBL).isLocker(locker)");
        require(ILocker_DAO(locker).canPush(), "ZivoeDAO::push() !ILocker_DAO(locker).canPush()");
        emit Pushed(locker, asset, amount, data);
        IERC20(asset).safeIncreaseAllowance(locker, amount);
        ILocker_DAO(locker).pushToLocker(asset, amount, data);
        // ZivoeDAO MUST ensure "locker" has 0 allowance before this function concludes.
>>>     if (IERC20(asset).allowance(address(this), locker) > 0) { IERC20(asset).safeDecreaseAllowance(locker, 0); }
    }

https://github.com/sherlock-audit/2024-03-zivoe/blob/main/zivoe-core-foundry/src/ZivoeDAO.sol#L282-L301

    function pushMulti(
        address locker, address[] calldata assets, uint256[] calldata amounts, bytes[] calldata data
    ) external onlyOwner nonReentrant {
        require(
            IZivoeGlobals_DAO(GBL).isLocker(locker), 
            "ZivoeDAO::pushMulti() !IZivoeGlobals_DAO(GBL).isLocker(locker)"
        );
        require(assets.length == amounts.length, "ZivoeDAO::pushMulti() assets.length != amounts.length");
        require(amounts.length == data.length, "ZivoeDAO::pushMulti() amounts.length != data.length");
        require(ILocker_DAO(locker).canPushMulti(), "ZivoeDAO::pushMulti() !ILocker_DAO(locker).canPushMulti()");
        for (uint256 i = 0; i < assets.length; i++) {
            IERC20(assets[i]).safeIncreaseAllowance(locker, amounts[i]);
            emit Pushed(locker, assets[i], amounts[i], data[i]);
        }
        ILocker_DAO(locker).pushToLockerMulti(assets, amounts, data);
        for (uint256 i = 0; i < assets.length; i++) {
            // ZivoeDAO MUST ensure "locker" has 0 allowance for each ERC20 token before this function concludes.
>>>         if (IERC20(assets[i]).allowance(address(this), locker) > 0) { IERC20(assets[i]).safeDecreaseAllowance(locker, 0); }
        }
    }

Impact

While currently, all lockers will consume their entire allowance and will not cause issues, it may introduce problems if there is a locker that doesn't all allowance in the future.

Code Snippet

https://github.com/sherlock-audit/2024-03-zivoe/blob/main/zivoe-core-foundry/src/ZivoeDAO.sol#L239-L247 https://github.com/sherlock-audit/2024-03-zivoe/blob/main/zivoe-core-foundry/src/ZivoeDAO.sol#L282-L301

Tool used

Manual Review

Recommendation

use safeApprove instead of safeDecreaseAllowance when removing allowance to 0.

sherlock-audit / 2024-03-zivoe-judging