Yeti Finance contest - cmichel's results

Handle

cmichel

Vulnerability details

The sYETIToken.lastBuyBackPrice is set in buyBack and hardcoded as:

function buyBack(address routerAddress, uint256 YUSDToSell, uint256 YETIOutMin, address[] memory path) external onlyOwner {
    require(YUSDToSell > 0, "Zero amount");
    require(lastBuybackTime + 69 hours < block.timestamp, "Must have 69 hours pass before another buyBack");
    yusdToken.approve(routerAddress, YUSDToSell);
    uint256[] memory amounts = IRouter(routerAddress).swapExactTokensForTokens(YUSDToSell, YETIOutMin, path, address(this), block.timestamp + 5 minutes);
    lastBuybackTime = block.timestamp;
    // amounts[0] is the amount of YUSD that was sold, and amounts[1] is the amount of YETI that was gained in return. So the price is amounts[0] / amounts[1]
    // @audit this hardcoded lastBuybackPrice is wrong when using a different path (think path length 3)
    lastBuybackPrice = div(amounts[0].mul(1e18), amounts[1]);
    emit BuyBackExecuted(YUSDToSell, amounts[0], amounts[1]);
}

It divides the first and second return amounts of the swap, however, these amounts depend on the swap path parameter that is used by the caller. If a swap path of length 3 is used, then this is obviously wrong. It also assumes that each router sorts the pairs the same way (which is true for Uniswap/Sushiswap).

Impact

The lastBuyBackPrice will be wrong when using a different path. This will lead rebases using a different yeti amount and the effectiveYetiTokenBalance being updated wrong.

Recommended Mitigation Steps

Verify the first and last element of the path are YETI/YUSD and use the first and last amount parameter.

Handle

cmichel

Vulnerability details

It's possible to repeatedly add the first collateral token in validCollateral through the Whitelist.addCollateral function. The validCollateral[0] != _collateral check will return false and skip further checks.

POC

Owner calls addCollateral(collateral=validCollateral[0]):

function addCollateral(
    address _collateral,
    uint256 _minRatio,
    address _oracle,
    uint256 _decimals,
    address _priceCurve, 
    bool _isWrapped
) external onlyOwner {
    checkContract(_collateral);
    checkContract(_oracle);
    checkContract(_priceCurve);
    // If collateral list is not 0, and if the 0th index is not equal to this collateral,
    // then if index is 0 that means it is not set yet.
    // @audit evaluates validCollateral[0] != validCollateral[0] which is obv. false => skips require check
    if (validCollateral.length != 0 && validCollateral[0] != _collateral) {
        require(collateralParams[_collateral].index == 0, "collateral already exists");
    }

    validCollateral.push(_collateral);
    // overwrites parameters
    collateralParams[_collateral] = CollateralParams(
        _minRatio,
        _oracle,
        _decimals,
        true,
        _priceCurve,
        validCollateral.length - 1, 
        _isWrapped
    );
}

Impact

The collateral parameters collateralParams are re-initialized which can break the existing accounting. The collateral token also exists multiple times in validCollateral.

Recommended Mitigation Steps

Fix the check. It should be something like:

if (validCollateral.length > 0) {
    require(collateralParams[_collateral].index == 0 && validCollateral[0] != _collateral, "collateral already exists");
}

Handle

cmichel

Vulnerability details

The ThreePieceWiseLinearPriceCurve.adjustParams function uses three functions f1, f2, f3 where y_i = f_i(x_i). It computes the y-axis intersect (b2 = f_2(0), b3 = f_3(0)) for each of these but uses unsigned integers for this, which means these values cannot become negative. This rules out a whole class of functions, usually the ones that are desirable.

Example:

Check out this two-piece linear interest curve of Aave:

Example: Imagine a curve that is flat at 10% on the first 50% utilization but shoots up to 110% at 100% utilization.

• m1 = 0, b1 = 10%, cutoff1 = 50%
• m2 = 200% => b2 = m1 * cutoff1 + b1 - m2 * cutoff1 = f1(cutoff1) - m2 * cutoff1 = 10% - 200% * 50% = 10% - 100% = -90%. (f2(100%) = 200% * 100% - 90% = 110% ✅) This function would revert in the b2 computation as it underflows due to being a negative value.

Impact

Most curves that are actually desired for a lending platform (becoming steeper at higher utilization) cannot be used.

Recommended Mitigation Steps

Evaluate the piecewise linear function in a different way that does not require computing the y-axis intersection value. For example, for cutoff2 >= x > cutoff1, use f(x) = f_1(cutoff) + f_2(x - cutoff). See Compound.

Handle

cmichel

Vulnerability details

The ThreePieceWiseLinearPriceCurve.getFee comment states that the total + the input must be less than the cap:

If dollarCap == 0, then it is not capped. Otherwise, then the total + the total input must be less than the cap.

The code only checks if the input is less than the cap:

// @param _collateralVCInput is how much collateral is being input by the user into the system
if (dollarCap != 0) {
    require(_collateralVCInput <= dollarCap, "Collateral input exceeds cap");
}

Recommended Mitigation Steps

Clarify the desired behavior and reconcile the code with the comments.

Handle

cmichel

Vulnerability details

The ThreePieceWiseLinearPriceCurve.calculateDecayedFee function is supposed to decay the lastFeePercent over time. This is correctly done in the decay > 0 && decay < decayTime case, but for the decay > decayTime case it does not decay at all but should set it to 0 instead..

if (decay > 0 && decay < decayTime) {
    // @audit if decay is close to decayTime, this fee will be zero. but below it'll be 1. the more time passes, the higher the decay. but then decay > decayTime should return 0.
    fee = lastFeePercent.sub(lastFeePercent.mul(decay).div(decayTime));
} else {
    fee = lastFeePercent;
}

Recommended Mitigation Steps

It seems wrong to handle the decay == 0 case (decay happened in same block) the same way as the decay >= decayTime case (decay happened long time ago) as is done in the else branch. I believe it should be like this instead:

// decay == 0 case should be full lastFeePercent
if(decay < decayTime) {
    fee = lastFeePercent.sub(lastFeePercent.mul(decay).div(decayTime));
} else {
    // reset to zero if decay >= decayTime
    fee = 0;
}

Handle

cmichel

Vulnerability details

The WJLP.wrap function accepts a from parameter and a to parameter. The tokens are transferred from the from account to the to account:

function wrap(uint _amount, address _from, address _to, address _rewardOwner) external override {
    // @audit can frontrun and steal => use from=victim, to=attacker
    JLP.transferFrom(_from, address(this), _amount);
    JLP.approve(address(_MasterChefJoe), _amount);

    // stake LP tokens in Trader Joe's.
    // In process of depositing, all this contract's
    // accumulated JOE rewards are sent into this contract
    _MasterChefJoe.deposit(_poolPid, _amount);

    // update user reward tracking
    _userUpdate(_rewardOwner, _amount, true);
    _mint(_to, _amount);
}

When a user wants to wrap their JLP tokens, they first need to approve the contracts with their token and in a second transaction call the wrap function. However, an attacker can frontrun the actual wrap function and call their own wrap(from=victim, to=attacker) which will make the victim pay with their approved tokens but the WJLP are minted to the attacker.

Impact

WJLP tokens can be stolen.

Recommended Mitigation Steps

Always transfer from msg.sender instead of using a caller-provided from parameter.

Handle

cmichel

Vulnerability details

Calling WJLP.unwrap burns WJLP, withdraws the amount from the master chef and returns the same amount of JLP back to the to address. However, it does not update the internal accounting in WJLP with a _userUpdate call.

This needs to be done on the caller side according to the comment in the WJLP.unwrap function:

"Prior to this being called, the user whose assets we are burning should have their rewards updated"

This happens when being called from the StabilityPool but not when being called from the ActivePool.sendCollateralsUnwrap:

function sendCollateralsUnwrap(address _to, address[] memory _tokens, uint[] memory _amounts, bool _collectRewards) external override returns (bool) {
    _requireCallerIsBOorTroveMorTMLorSP();
    require(_tokens.length == _amounts.length);
    for (uint i = 0; i < _tokens.length; i++) {
        if (whitelist.isWrapped(_tokens[i])) {
            // @audit this burns the tokens for _to but does not reduce their amount. so there are no tokens in WJLP masterchef but can keep claiming
            IWAsset(_tokens[i]).unwrapFor(_to, _amounts[i]);
            if (_collectRewards) {
                IWAsset(_tokens[i]).claimRewardFor(_to);
            }
        } else {
            _sendCollateral(_to, _tokens[i], _amounts[i]); // reverts if send fails
        }
    }
    return true;
}

Impact

The unwrapFor call withdraws the tokens from the Masterchef and pays out the user, but their user balance is never decreased by the withdrawn amount. They can still use their previous balance to claim rewards through WJLP.claimReward which updated their unclaimed joe reward according to the old balance. Funds from the WJLP pool can be stolen.

Recommended Mitigation Steps

As the comment says, make sure the user is updated before each unwrap call. It might be easier and safer to have a second authorized unwrapFor function that accepts a rewardOwner parameter, the user that needs to be updated.