Yeti Finance contest - certora's results

Handle

certora

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/StabilityPool.sol#L947 use of transfer instead of safeTransfer

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

Recommended Mitigation Steps

change to safeTransfer

Handle

certora

Vulnerability details

Impact

You have many loops in your code. In all those loop you use postfix opertaions, i++ by replacing to prefix operations,++i you can save a little bit of gas every iteration. This amount of gas will add up very quickly since there are a lot of loops and each loop iterate many times. This is an easy fix and can save a lot of gas.

++i is cheaper than i++ because it doesnt save the previous value of i to return it later

Tools Used

Manual code review

Recommended Mitigation Steps

replace every i++ with ++i where it doesnt affect the logic (of course also i-- to --i)

Handle

certora

Vulnerability details

Impact

In StabilityPool line 747 you multiply a sum of two parameters but since the second parameter is calculated through division it can cause lack of precision since the order of the operations is div()->mul() instead of mul()->div()

Tools Used

Manual code review

Recommended Mitigation Steps

dont divide secondPortion by SCALE_FACTOR, instead multiply the firstPortion by SCALE_FACTOR and after you multiply their sum by initialDeposit, divide the result by SCALE_FACTOR. like this:

original:

uint256 firstPortion = epochToScaleToSum[asset][snapshots.epoch][snapshots.scale].sub(
            S_Snapshot
        );        
        uint256 secondPortion = epochToScaleToSum[asset][snapshots.epoch][snapshots.scale.add(1)]
            .div(SCALE_FACTOR);

        uint256 assetGain = initialDeposit.mul(firstPortion.add(secondPortion)).div(P_Snapshot).div(
            DECIMAL_PRECISION
        );

changed:

uint256 firstPortion = epochToScaleToSum[asset][snapshots.epoch][snapshots.scale].sub(
            S_Snapshot
        ).mul(SCALE_FACTOR); 
        uint256 secondPortion = epochToScaleToSum[asset][snapshots.epoch][snapshots.scale.add(1)];

        uint256 assetGain = initialDeposit.mul(firstPortion.add(secondPortion)).div(SCALE_FACTOR).div(P_Snapshot).div(
            DECIMAL_PRECISION
        );

Handle

certora

Vulnerability details

Impact

In multiple functions you use named return values but still use the return statment. For example in _getTotalVariableDepositFee in BorrowerOperations in getPendingYUSDDebtReward in TroberManager and in many more...

This is very misleading (beside a waste of gas) and cause a confusing documantation

Tools Used

Manual code review

Recommended Mitigation Steps

remove the return statement or the named return value

Handle

certora

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/TroveManager.sol#L672 https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/YUSDToken.sol#L255

the following lines use assert to validate user input, it's wrong and should be replaced with require. here is an article that explains why you should always use require to validate user input: https://medium.com/blockchannel/the-use-of-revert-assert-and-require-in-solidity-and-the-new-revert-opcode-in-the-evm-1a3a7990e06e

Impact

users who accidentally provide invalid input will not be refunded by the remaining gas, and it can be a very high amount.

Recommended Mitigation Steps

replace assert with require in the lines I wrote.

Handle

certora

Vulnerability details

Impact

In CommuintyIssuance.sol line 84 you use assert instead of require. assert is a waste of gas since it will take all the gas that was sent to the transaction instead of stopping at the amount of gas spent until the failure.

Tools Used

Manual code review

Recommended Mitigation Steps

replace assert with require

Handle

certora

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/YETI/sYETIToken.sol#L231 wrong use of deadline field:

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

the deadline is the timestamp after which the transaction will revert. the goal of this field is that the caller can set a deadline for the transaction so the transaction will not succeed in any arbitrary time in the future, and after this deadline, they can resubmit the transaction: https://help.uniswap.org/en/articles/5455497-my-transaction-has-been-pending-for-a-long-time-what-can-i-do#:~:text=Wait%20%E2%80%94%20the%20Uniswap%20interface%20has,you%20can%20resubmit%20your%20transaction. here it is set to block.timestamp + 5 minutes. the problem here is that the transaction will always occur at block.timestamp so setting the deadline to be block.timestamp + 5 minutes won't have any effect. therefore the transaction can still run in an arbitrary time in the future. block.timestamp is confused with the time the transaction was submitted.

Impact

practically, buyBack transaction doesn't have a deadline

Proof of Concept

The owner calls buyBack, they have read the code so they expect that it won't take more than 5 minutes since they submitted the transaction. However, it can take an arbitrary time for the transaction to succeed. If it takes too long, the owner can try to cancel it in order to resubmit another, but it's not guaranteed to work: https://help.uniswap.org/en/articles/5455497-my-transaction-has-been-pending-for-a-long-time-what-can-i-do#:~:text=Wait%20%E2%80%94%20the%20Uniswap%20interface%20has,you%20can%20resubmit%20your%20transaction If it won't work, then the owner won't be able to resubmit it, and it can take a very long time until the transaction is done.

Tools Used

manual review

Recommended Mitigation Steps

add a parameter in buyBack for the swap deadline.

Handle

certora

Vulnerability details

Impact

In multiple for loops you access the .length if an array in every loop iteration. for example in BorrowerOperations line 873, 920, 929,930 and more... This is a waste of gas and could easily be reduced by saving the length in a local variable

Proof of Concept

Checked on Remix

Tools Used

Manual code review

Recommended Mitigation Steps

Save the length in a local variable right before entering the loop and iterate over it instead

Handle

certora

Vulnerability details

Impact

Most of the contracts use solidity version 0.6.11 some use 0.6.12 like sYETIToken.sol and BoringBatchable.sol and more.. some even use 0.8.7 like ERC20_8.sol This may cause a problem

Tools Used

Manual code review

Recommended Mitigation Steps

change all contract to the same solidity version, if you lower some to before 0.8.0 make sure you change to safeMath too if needed