Yeti Finance contest - WatchPug's results

Handle

WatchPug

Vulnerability details

It is usually good to add a require-statement that checks the return value or to use something like safeTransfer; unless one is sure the given token reverts in case of a failure.

Instances include:

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TeamAllocation.sol#L63-L78

    function sendAllocatedYETI() external {
        require(yetiSet);
        require(!allocationClaimed);
        for (uint i = 0; i < 7; i++) {
            address member = team[i];
            uint amount = allocations[i];
            require(YETI.transfer(member, amount));
        }
        allocationClaimed = true;
    }


    function sendUnallocatedYETI(address _to, uint _amount) external onlyTeam {
        require(allocationClaimed);
        YETI.transfer(_to, _amount);
    }

L77 应该类似 L69 check return value for token.transfer call

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/StabilityPool.sol#L947-L947

IERC20(assets[i]).transfer(_to, amounts[i]);

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YetiFinanceTreasury.sol#L25-L25

_token.transfer(_to, _amount);

Recommendation

Consider adding a require-statement or using safeTransfer.

Handle

WatchPug

Vulnerability details

The WJLP.sol#setAddresses() function that initializes important contract state can be called by anyone.

Impact

The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract.

In the best case for the victim, they notice it and have to redeploy their contract costing gas.

Recommendation

Consider using the constructor to initialize non-proxied contracts.

Or, make sure to call it immediately after deployment and verify the transaction succeeded.

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L102-L117

function setAddresses(
    address _activePool,
    address _TML,
    address _TMR,
    address _defaultPool,
    address _stabilityPool,
    address _YetiFinanceTreasury) external {
    require(!addressesSet);
    activePool = _activePool;
    TML = _TML;
    TMR = _TMR;
    defaultPool = _defaultPool;
    stabilityPool = _stabilityPool;
    YetiFinanceTreasury = _YetiFinanceTreasury;
    addressesSet = true;
}

Handle

WatchPug

Vulnerability details

It's a best practice to use the latest compiler version.

The specified minimum compiler version is not up to date. Older compilers might be susceptible to some bugs. We recommend changing the solidity version pragma to the latest version to enforce the use of an up to date compiler.

List of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YetiFinanceTreasury.sol#L28-L30

function updateTeamWallet(address _newTeamWallet) external onlyTeam {
    teamWallet = _newTeamWallet;
}

YetiFinanceTreasury.teamWallet is a critical role, if the current teamWallet mistakenly called updateTeamWallet() with a wrong address, it can result in all the onlyTeam() methods being unaccessible, and it cannot be undo.

Recomandation

Consider changing the updateTeamWallet() function to first nominate an address as the pending teamWallet and adding an acceptTeamWallet() function which is called by the pending teamWallet to confirm the transfer.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/AssetWrappers/WJLP/ERC20_8.sol#L128-L144

// ------------------------------------------------------------------------
// Mint new tokens to a given _to address
// ------------------------------------------------------------------------
function _mint(address _to, uint _num_tokens) internal returns (bool success) {
    balances[_to] = balances[_to] + _num_tokens;
    emit Transfer(address(0), _to, _num_tokens);
    return true;
}

// ------------------------------------------------------------------------
// Burn tokens owned by _holder
// ------------------------------------------------------------------------
function _burn(address _holder, uint _num_tokens) internal returns (bool success) {
    balances[_holder] = balances[_holder].sub(_num_tokens);
    emit Transfer(_holder, address(0), _num_tokens);
    return true;
}

totalSupply is one of the essential view methods of an ERC20 contract. When tokens get mint and burn, it is supposed to update the totalSupply.

The current implementation does provide a totalSupply() view function, but the storage variable _totalSupply will never be updated.

Handle

WatchPug

Vulnerability details

There are ERC20 tokens that charge fee for every transfer() or transferFrom().

In the current implementation, BorrowerOperations.sol#_transferCollateralsIntoActivePool() assumes that the received amount is the same as the transfer amount, and uses it as collateral.

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/BorrowerOperations.sol#L731-L748

    function _transferCollateralsIntoActivePool(
        address _from,
        address[] memory _colls,
        uint256[] memory _amounts
    ) internal returns (bool) {
        uint256 len = _amounts.length;
        for (uint256 i = 0; i < len; i++) {
            address collAddress = _colls[i];
            uint256 amount = _amounts[i];
            IERC20 coll = IERC20(collAddress);

            bool transferredToActivePool = coll.transferFrom(_from, address(activePool), amount);
            if (!transferredToActivePool) {
                return false;
            }
        }
        return true;
    }

Recommended

• Compare the before and after balanceOf to get the actual transferred amount;
• Or, not to whitelist any tokens with fee on transfer.

Handle

WatchPug

Vulnerability details

In the current design/implementation of WJLP and collateral redeem system, adding JLP as collateral via WJLP can obtain rights to the future rewards.

However, when the collaterals are redeemed (from other borrowers), the rights to future rewards are not revoked.

This can be exploited as an economic attack vector.

PoC

Given:

• There are $1M worth of JLP tokens wrapped as WJLP and used as collateral;
• JLP is the only whitelisted collateral.

The attacker can do the following steps:

1. Wrap $1M worth of JLP to WJLP, the WJLP.userInfo[attacker] is the amount of $1M worth of JLP;
2. Borrow 1M YUSD using the WJLP received as collateral;
3. Call redeemCollateral() to redeem $1M worth of JLP with 1M YUSD;

WJLP.userInfo[attacker] is still the amount of $1M worth of JLP, and can continuously call WJLP.getPendingRewards() to collect the rewards.

The attack can be amplified with falshloan.

Handle

WatchPug

Vulnerability details

The following source units are imported but not referenced in the contract:

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YETI/sYETIToken.sol#L10-L10

import "./BoringCrypto/BoringBatchable.sol";

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManagerLiquidations.sol#L7-L7

import "hardhat/console.sol";

Recommendation

Check all imports and remove all unused/unreferenced and unnecessary imports.

Handle

WatchPug

Vulnerability details

Using ++i is more gas efficient than i++, especially in for loops.

For example:

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/ActivePool.sol#L167-L167

for (uint i = 0; i < _tokens.length; i++) {

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/ActivePool.sol#L184-L184

for (uint i = 0; i < _tokens.length; i++) {

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/BorrowerOperations.sol#L737-L737

for (uint256 i = 0; i < len; i++) {

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/DefaultPool.sol#L133-L133

for (uint256 i = 0; i < _tokens.length; i++) {

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/StabilityPool.sol#L588-L588

for (uint256 i = 0; i < _amountsAdded.length; i++) {

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManager.sol#L525-L525

for (uint i = 0; i < _tokens.length; i++) {

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManagerLiquidations.sol#L475-L475

for (uint256 i = 0; i < vars.collToLiquidate.tokens.length; i++) {

Handle

WatchPug

Vulnerability details

Unused named returns increase contract size and gas usage at deployment.

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L257-L260

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L246-L246

function _userUpdate(address _user, uint256 _amount, bool _isDeposit) private returns (uint pendingJoeSent) {

pendingJoeSent is unused.

Handle

WatchPug

Vulnerability details

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/ActivePool.sol#L229-L232

        require(
            msg.sender == borrowerOperationsAddress ||
            msg.sender == defaultPoolAddress,
            "ActivePool: Caller is neither BO nor Default Pool");

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/ActivePool.sol#L244-L249

    function _requireCallerIsBOorTroveM() internal view {
        require(
            msg.sender == borrowerOperationsAddress ||
            msg.sender == troveManagerAddress,
            "ActivePool: Caller is neither BorrowerOperations nor TroveManager");
    }

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/BorrowerOperations.sol#L1018-L1021

        require(
            _debtRepayment <= _currentDebt.sub(YUSD_GAS_COMPENSATION),
            "BorrowerOps: Amount repaid must not be larger than the Trove's debt"
        );

Handle

WatchPug

Vulnerability details

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

• Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
• Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
• Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
• Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/ActivePool.sol#L3-L3

pragma solidity 0.6.11;

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/BorrowerOperations.sol#L3-L3

pragma solidity 0.6.11;

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YUSDToken.sol#L3-L3

pragma solidity 0.6.11;

Handle

WatchPug

Vulnerability details

Setting uint variables to 0 is redundant as they default to 0.

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/AssetWrappers/WJLP/ERC20_8.sol#L12-L12

uint public _totalSupply;

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L82-L100

    constructor(string memory ERC20_symbol,
        string memory ERC20_name,
        uint8 ERC20_decimals,
        IERC20 _JLP,
        IERC20 _JOE,
        IMasterChefJoeV2 MasterChefJoe,
        uint256 poolPid) {

        _symbol = ERC20_symbol;
        _name = ERC20_name;
        _decimals = ERC20_decimals;
        _totalSupply = 0;

        JLP = _JLP;
        JOE = _JOE;

        _MasterChefJoe = MasterChefJoe;
        _poolPid = poolPid;
    }

Recommendation

Consider removing _totalSupply = 0;, make the code simpler and save some gas.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManager.sol#L29-L29

uint constant public SECONDS_IN_ONE_MINUTE = 60;

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YETI/CommunityIssuance.sol#L21-L21

uint constant public SECONDS_IN_ONE_MINUTE = 60;

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YETI/LockupContract.sol#L25-L25

uint constant public SECONDS_IN_ONE_YEAR = 31536000;

For the constants that should not be public, changing them to private / internal can save some gas.

Handle

WatchPug

Vulnerability details

console.log() should not be in production code, removing them can also save gas.

https://hardhat.org/hardhat-network/reference/#:~:text=do%20spend%20a%20minimal%20amount%20of%20gas

Instances include:

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L152-L160

console.log("Active Pool JLP Balance");
console.log(JLP.balanceOf(activePool));
console.log("Active Pool WJLP Balance");
console.log(balanceOf(activePool));

console.log("stability Pool JLP Balance");
console.log(JLP.balanceOf(stabilityPool));
console.log("stability Pool WJLP Balance");
console.log(balanceOf(stabilityPool));

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManagerRedemptions.sol#L228-L233

// send fee from user to YETI stakers
contractsCache.yusdToken.transferFrom(
    _redeemer,
    address(contractsCache.sYETI),
    totals.YUSDfee
);

Since totals.YUSDfee can be 0. Checking if (totals.YUSDfee > 0) before the transfer can potentially save an external call and the unnecessary gas cost of a 0 token transfer.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YETI/ShortLockupContract.sol#L29-L29

IYETIToken public yetiToken;

yetiToken will never change, use immutable variable instead of storage variable can save gas.

Handle

WatchPug

Vulnerability details

Contracts use assert() instead of require() in multiple places.

Per to Solidity’s documentation:

"Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.”

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/BorrowerOperations.sol#L290-L290

assert(vars.compositeDebt > 0);

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManager.sol#L571-L571

assert(closedStatus != Status.nonExistent && closedStatus != Status.active);

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YUSDToken.sol#L229-L230

assert(sender != address(0));
assert(recipient != address(0));

Recommendation

Change to require().

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YETI/sYETIToken.sol#L231-L231

uint256[] memory amounts = IRouter(routerAddress).swapExactTokensForTokens(YUSDToSell, YETIOutMin, path, address(this), block.timestamp + 5 minutes);

deadline is designed for the caller to specify a deadline time for the transaction to be packed. However, since the deadline is calculated based on block.timestamp + 5 minutes, it won't be effective (it always passes).

Therefore, changing it to block.timestamp will make the code simpler and save some gas.

Recommendation

Change to:

uint256[] memory amounts = IRouter(routerAddress).swapExactTokensForTokens(YUSDToSell, YETIOutMin, path, address(this), block.timestamp);

Handle

WatchPug

Vulnerability details

SafeMath is no longer needed starting with Solidity 0.8. The compiler now has built in overflow checking.

Removing SafeMath can save some gas.

Handle

WatchPug

Vulnerability details

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/AssetWrappers/WJLP/ERC20_8.sol#L68-L70

        require(_num_tokens <= balances[msg.sender], "You are trying to transfer more tokens than you have");

        balances[msg.sender] = balances[msg.sender] - _num_tokens;

balances[msg.sender] - _num_tokens will never underflow.

Handle

WatchPug

Vulnerability details

For public functions, the input parameters are copied to memory automatically which costs gas. If a function is only called externally, making its visibility as external will save gas because external functions' parameters are not copied into memory and are instead read from calldata directly.

For example:

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManagerLiquidations.sol#L148-L234

function batchLiquidateTroves(address[] memory _troveArray, address _liquidator) public {
    // ...
}

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/DefaultPool.sol#L84-L86

function getAllCollateral() public view override returns (address[] memory, uint256[] memory) {
    return (poolColl.tokens, poolColl.amounts);
}

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManagerRedemptions.sol#L440-L440

uint256 redeemedYUSDFraction = _YUSDDrawn.mul(10**18).div(_totalYUSDSupply);

10 ** 18 can be changed to 1e18 to avoid unnecessary arithmetic operation and save gas.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/HintHelpers.sol#L31-L52

function setAddresses(
        address _sortedTrovesAddress,
        address _troveManagerAddress,
        address _whitelistAddress
    )
        external
        onlyOwner
    {
        checkContract(_sortedTrovesAddress);
        checkContract(_troveManagerAddress);
        checkContract(_whitelistAddress);

        sortedTroves = ISortedTroves(_sortedTrovesAddress);
        troveManager = ITroveManager(_troveManagerAddress);
        whitelist = IWhitelist(_whitelistAddress);

        emit SortedTrovesAddressChanged(_sortedTrovesAddress);
        emit TroveManagerAddressChanged(_troveManagerAddress);
        emit WhitelistAddressChanged(_troveManagerAddress);

        _renounceOwnership();
    }

Across the codebase, including in HintHelpers.sol, setAddresses() is being used as a initializer function, it's a onlyOwner function and it will _renounceOwnership() at the end of the function.

There are no other onlyOwner functions, the addresses set in the function are immutable after being set.

Therefore, setAddresses() can be replaced with constructor and all the addresses can be declared as immutable variables instead of storage variables.

And Ownable can be removed to further reduce gas costs.

This issue also applies to:

• ActivePool.sol
• BorrowerOperations.sol
• CollSurplusPool.sol
• DefaultPool.sol
• PriceFeed.sol
• StabilityPool.sol
• TroveManager.sol
• TroveManagerLiquidations.sol
• TroveManagerRedemptions.sol
• Whitelist.sol

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YUSDToken.sol#L297-L301

    function _requireCallerIsTMLorSP() internal view {
        require(
            msg.sender == stabilityPoolAddress || msg.sender == troveManagerLiquidationsAddress,
            "YUSD: Caller is neither TroveManagerLiquidator nor StabilityPool");
    }

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YUSDToken.sol#L128-L131

    function returnFromPool(address _poolAddress, address _receiver, uint256 _amount) external override {
        _requireCallerIsTMLorSP();
        _transfer(_poolAddress, _receiver, _amount);
    }

_requireCallerIsTMLorSP() is unnecessary as it's being used only once. Therefore it can be inlined in returnFromPool() to make the code simpler and save gas.

Recommendation

Change to:

    function returnFromPool(address _poolAddress, address _receiver, uint256 _amount) external override {
        require(
            msg.sender == stabilityPoolAddress || msg.sender == troveManagerLiquidationsAddress,
            "YUSD: Caller is neither TroveManagerLiquidator nor StabilityPool");
        _transfer(_poolAddress, _receiver, _amount);
    }

Other examples include:

• TroveManagerRedemptions.sol#_getRedemptionFee() can be inlined in TroveManagerRedemptions.sol#redeemCollateral()

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManagerLiquidations.sol#L839-L847

function _updateWAssetsRewardOwner(newColls memory _colls, address _borrower, address _newOwner) internal {
        for (uint i = 0; i < _colls.tokens.length; i++) {
            address token = _colls.tokens[i];
            uint amount = _colls.amounts[i];
            if (whitelist.isWrapped(token)) {
                IWAsset(token).updateReward(_borrower, _newOwner, amount);
            }
        }
    }

Since amount is only needed in if (whitelist.isWrapped(token)) {...}, so uint amount = _colls.amounts[i]; should be moved to inside if (whitelist.isWrapped(token)) {...}.

Furthermore, considering that amount is only used once, it can be replaced with _colls.amounts[i].

Recommendation

Change to:

function _updateWAssetsRewardOwner(newColls memory _colls, address _borrower, address _newOwner) internal {
    for (uint i = 0; i < _colls.tokens.length; i++) {
        address token = _colls.tokens[i];
        if (whitelist.isWrapped(token)) {
            IWAsset(token).updateReward(_borrower, _newOwner, _colls.amounts[i]);
        }
    }
}

Handle

WatchPug

Vulnerability details

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• ActivePool.sol#sendCollaterals()

  https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/ActivePool.sol#L167

• BorrowerOperations.sol#_getTotalVariableDepositFee()

  https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/BorrowerOperations.sol#L699

• DefaultPool.sol#sendCollsToActivePool()

  https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/DefaultPool.sol#L133

• TroveManager.sol#_updateTroveRewardSnapshots()

  https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManager.sol#L348

• StabilityPool.sol#_computeRewardsPerUnitStaked()

  https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/StabilityPool.sol#L562

Handle

WatchPug

Vulnerability details

For the arithmetic operations that will never over/underflow, using SafeMath will cost more gas.

For example:

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/BorrowerOperations.sol#L791-L795

if (_debtChange > _variableYUSDFee) { // if debt decrease, and greater than variable fee, decrease 
    newDebt = _troveManager.decreaseTroveDebt(_borrower, _debtChange.sub(_variableYUSDFee));
} else { // otherwise increase by opposite subtraction
    newDebt = _troveManager.increaseTroveDebt(_borrower, _variableYUSDFee.sub(_debtChange));
}

_debtChange - _variableYUSDFee at L792 and _variableYUSDFee - _debtChange at L794 will never underflow.

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YUSDToken.sol#L240-L241

_totalSupply = _totalSupply.add(amount);
_balances[account] = _balances[account].add(amount);

_balances[account] + amount will not overflow if _totalSupply.add(amount) dose not overflow.

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/YUSDToken.sol#L248-L249

_balances[account] = _balances[account].sub(amount, "ERC20: burn amount exceeds balance");
_totalSupply = _totalSupply.sub(amount);

_totalSupply - amount will not underflow if _balances[account].sub(amount) dose not underflow.

Handle

WatchPug

Vulnerability details

For the storage variables that will be accessed multiple times, cache them in the stack can save ~100 gas from each extra read (SLOAD after Berlin).

For example:

• troveManager in TroveManagerRedemptions#_updateBaseRateFromRedemption()

  https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManagerRedemptions.sol#L432-L447

  function _updateBaseRateFromRedemption(uint256 _YUSDDrawn, uint256 _totalYUSDSupply)
  internal
  returns (uint256)
  {
      uint256 decayedBaseRate = troveManager.calcDecayedBaseRate();
  
      /* Convert the drawn Collateral back to YUSD at face value rate (1 YUSD:1 USD), in order to get
       * the fraction of total supply that was redeemed at face value. */
      uint256 redeemedYUSDFraction = _YUSDDrawn.mul(10**18).div(_totalYUSDSupply);
  
      uint256 newBaseRate = decayedBaseRate.add(redeemedYUSDFraction.div(BETA));
      newBaseRate = LiquityMath._min(newBaseRate, DECIMAL_PRECISION); // cap baseRate at a maximum of 100%
  
      troveManager.updateBaseRate(newBaseRate);
      return newBaseRate;
  }