code-423n4 / 2021-11-badgerzaps-findings

0 stars 0 forks source link

Improper implementation of slippage check #47

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

https://github.com/Badger-Finance/ibbtc/blob/d8b95e8d145eb196ba20033267a9ba43a17be02c/contracts/Zap.sol#L216-L238

function redeem(IERC20 token, uint amount, uint poolId, int128 idx, uint minOut)
    external
    defend
    blockLocked
    whenNotPaused
    returns(uint out)
{
    ibbtc.safeTransferFrom(msg.sender, address(this), amount);

    Pool memory pool = pools[poolId];
    if (poolId < 3) { // setts
        settPeak.redeem(poolId, amount);
        pool.sett.withdrawAll();
        pool.deposit.remove_liquidity_one_coin(pool.lpToken.balanceOf(address(this)), idx, minOut);
    } else if (poolId == 3) { // byvwbtc
        byvWbtcPeak.redeem(amount);
        IbyvWbtc(address(pool.sett)).withdraw(); // withdraws all available
    } else {
        revert("INVALID_POOL_ID");
    }
    out = token.balanceOf(address(this));
    token.safeTransfer(msg.sender, out);
}

In the current implementation of. Zap.sol#redeem(), the outAmount of IbyvWbtc.withdraw() is not controlled by minOut.

Recommendation

Consider implementing the minOut check in between L236 and L237.


    ...
    out = token.balanceOf(address(this));
    require(out >= _minOut, "Slippage Check");
    token.safeTransfer(msg.sender, out);
}
GalloDaSballo commented 2 years ago

Agree with the finding, not having slippage check at end means people can get rekt, we'll add as suggested