code-423n4 / 2022-05-backd-findings

0 stars 0 forks source link

QA Report #94

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA

Use unchecked lib

PoolMigrationZap.sol

Enforce pattern on loops

You can add the unchecked math lib like you do on the others contracts to safely increment the i var, saving gas and make contract consistent with the others by enforcing the same style.

On PoolMigrationZap.sol#L39-L44 you are doing a unchecked increment in the loop, but in the rest of the code you a different pattern. Add the unchecked math lib and change:

        for (uint256 i; i < oldPoolAddresses_.length; ) {
            migrate(oldPoolAddresses_[i]);
            unchecked {
                ++i;
            }
        }

To;

        for (uint256 i; i < oldPoolAddresses_.length; i = i.uncheckedInc()) {
            migrate(oldPoolAddresses_[i]);
        }

_underlyingNewPools can end with a wrong data

If underlying_ is address(0) then _underlyingNewPools[address(0)] will be fill...

Consider change lines PoolMigrationZap.sol#L26-L27 from:

            _underlyingNewPools[underlying_] = newPool_;
            if (underlying_ == address(0)) continue;

To:

            if (underlying_ == address(0)) continue;
            _underlyingNewPools[underlying_] = newPool_;

ConvexStrategyBase.sol

Change var name to avoid shadow

On line L287 you are declaring variable with the same name, i understang that is for caching, but i recommend to change the variable name, consider changing this;

        address _swapperRouter = address(_swapperRouter);
        IERC20(token_).safeApprove(_swapperRouter, 0);
        IERC20(token_).safeApprove(_swapperRouter, type(uint256).max);

To this;

        address swapperRouter_ = address(_swapperRouter);
        IERC20(token_).safeApprove(swapperRouter_, 0);
        IERC20(token_).safeApprove(swapperRouter_, type(uint256).max);

LiquidityPool.sol

Add check before calling safeApprove

In think you will need a check before doing the safeApprove call on LiquidityPool.sol#L700 Just replace;

IERC20(lpToken_).safeApprove(staker_, type(uint256).max);

With

if (IERC20(lpToken_).allowance(staker_, spender) > 0) return;
IERC20(lpToken_).safeApprove(staker_, type(uint256).max);

_keeperGauge

Missing event emission

Critical function dont emit events; InflationManager.sol#L58-L63:setMinter InflationManager.sol#L89:deactivateWeightBasedKeeperDistribution

GalloDaSballo commented 2 years ago

Enforce pattern on loops

Valid gas savings

_underlyingNewPools can end with a wrong data

Lack of address(0) check, valid

Change var name to avoid shadow

Agree with avoiding shadowing

Add check before calling safeApprove

Disagree that it's an issue

Missing event emission

Agree with informational level finding

GalloDaSballo commented 2 years ago

Personally I recommend sorting by issue rather than by file, that said this report was pretty short which means either way of grouping findings is fine