Yeti Finance contest - hyh's results

Handle

hyh

Vulnerability details

Impact

The condition isn't checked now as the whole balance is used instead of the Yeti tokens bought back from the market. As it's not checked, the amount added to effectiveYetiTokenBalance during rebase can exceed the actual amount of the Yeti tokens owned by the contract. As the before check amount is calculated as the contract net worth, it can be fixed by immediate buy back, but it will not be the case.

The deficit of Yeti tokens can materialize in net worth terms as well if Yeti tokens price will raise compared to the last used one. In this case users will be cumulatively accounted with the amount of tokens that cannot be actually withdrawn from the contract, as its net holdings will be less then total users’ claims. In other words, the contract will be in default if enough users claim after that.

Proof of Concept

Now the whole balance amount is used instead of the amount bought back from market.

Rebasing amount is added to effectiveYetiTokenBalance, so it should be limited by extra Yeti tokens, not the whole balance: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/YETI/sYETIToken.sol#L247

Recommended Mitigation Steps

It looks like only extra tokens should be used for the check, i.e. yetiToken.balance - effectiveYetiTokenBalance.

Now:

function rebase() external {
		...
		uint256 yetiTokenBalance = yetiToken.balanceOf(address(this));
		uint256 valueOfContract = _getValueOfContract(yetiTokenBalance);
		uint256 additionalYetiTokenBalance = ...
		if (yetiTokenBalance < additionalYetiTokenBalance) {
				additionalYetiTokenBalance = yetiTokenBalance;
		}
		effectiveYetiTokenBalance = effectiveYetiTokenBalance.add(additionalYetiTokenBalance);
...
function _getValueOfContract(uint _yetiTokenBalance) internal view returns (uint256) {
		uint256 adjustedYetiTokenBalance = _yetiTokenBalance.sub(effectiveYetiTokenBalance);
		uint256 yusdTokenBalance = yusdToken.balanceOf(address(this));
		return div(lastBuybackPrice.mul(adjustedYetiTokenBalance), (1e18)).add(yusdTokenBalance);
}

As the _getValueOfContract function isn't used elsewhere, the logic can be simplified. To be:

function rebase() external {
		...
		uint256 adjustedYetiTokenBalance = (yetiToken.balanceOf(address(this))).sub(effectiveYetiTokenBalance);
		uint256 valueOfContract = _getValueOfContract(adjustedYetiTokenBalance);
		uint256 additionalYetiTokenBalance = ...
		if (additionalYetiTokenBalance > adjustedYetiTokenBalance) {
				additionalYetiTokenBalance = adjustedYetiTokenBalance;
		}
		effectiveYetiTokenBalance = effectiveYetiTokenBalance.add(additionalYetiTokenBalance);
...
function _getValueOfContract(uint _adjustedYetiTokenBalance) internal view returns (uint256) {
		uint256 yusdTokenBalance = yusdToken.balanceOf(address(this));
		return div(lastBuybackPrice.mul(_adjustedYetiTokenBalance), (1e18)).add(yusdTokenBalance);
}

Handle

hyh

Vulnerability details

Impact

Stale 'carried over' price can be used for liquidations. This can cause various types of malfunctions and manipulated liquidations.

For example, if a portfolio consists of two inversely correlated assets, which move in opposite directions most of the times, and portfolio owner uses that knowledge to borrow against such a composition. Now first asset price is stale, second one is updated and it is in decline, so the link between assets looks to be broken and the system treats the portfolio as under collateralized, while in reality the inverse correlation holds, first asset market price rose to fully compensate the decline of the second asset price, and portfolio value isn't declined and no liquidation should take place.

Proof of Concept

Prices used in liquidations are obtained from PriceFeed contract, call sequence is as follows: Trove and Pool functions -> Whitelist.getValueVC -> Whitelist.getPrice -> PriceFeed.fetchPrice_v -> PriceFeed._getCurrentChainlinkResponse

PriceFeed determines that the price is usable via _badChainlinkResponse function. _badChainlinkResponse checks that timestamp is valid (not 0, not in the future), but do not check that it is recent: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/PriceFeed.sol#L578

Moreover, _getCurrentChainlinkResponse function which provides core price structure used, ignores answeredInRound returned by ChainLink in the first place: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/PriceFeed.sol#L756

Chainlink feed has two properties that can help determine if the answer is actual (price is fresh): updatedAt time https://docs.chain.link/docs/faq/#what-is-the-difference-between-the-data-feed-properties-updatedat-and-answeredinround and answeredInRound: https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

This way ChainLink roundId isn't checked in PriceFeed contract used for price discovery and carried over price is treated as fresh in all VC related computations.

Recommended Mitigation Steps

Liquidation mechanics is a core system functionality which is based on current market price observations. It is crucial to ensure that these observations are sound.

Best practice is to check that answeredInRound is equal to roundId and this way the price obtained is fresh and can be used for decision making. It's advised to save answeredInRound and check it against roundId returned, and use the price in PriceFeed response only if rounds match, treating it as stale otherwise.

It is advised to allow state changing operations only when the price is recent, i.e. it is better to reject an operation than to process it using stale parameter.

Handle

hyh

Vulnerability details

Impact

Transactions will not be reverted on failed transfer call, setting system state as if it was successful. This will lead to wrong state accounting down the road with a wide spectrum of possible consequences.

Proof of Concept

_safeJoeTransfer do not check for JOE.transfer call success: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L268

_safeJoeTransfer is called by _sendJoeReward, which is used in reward claiming.

JOE token use transfer from OpenZeppelin ERC20: https://github.com/traderjoe-xyz/joe-core/blob/main/contracts/JoeToken.sol#L9

Which does return success code: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L113

Trader Joe also uses checked transfer when dealing with JOE tokens: https://github.com/traderjoe-xyz/joe-core/blob/main/contracts/MasterChefJoeV3.sol#L102

Also, unwrapFor do not check for JLP.transfer call success: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L166

Recommended Mitigation Steps

Add a require() check for the success of JOE transfer in _safeJoeTransfer function and create and use a similar function with the same check for JLP token transfers

Handle

hyh

Vulnerability details

Impact

User's rewards can be partial lost as system will write down all current rewards as transferred, while actual transfer amount can be smaller. WJLP will be still accounted for the remaining part of rewards by Trader Joe, but there will be no link to the user to whom these rewards should be accounted for in WJLP.

Proof of Concept

WJLP's _sendJoeReward calls _safeJoeTransfer to send unclaimedJOEReward and unconditionally set unclaimedJOEReward to 0: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L229

While _safeJoeTransfer will send less if there is not enough balance at the moment: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L268

Trader Joe system ('an external system we are dealing with' in general if this to be an example contract) can malfunction and send less than what's due or avoid sending rewards altogether. As _MasterChefJoe.withdraw is called without withdraw call result check of any kind, this will go unnoticed and user's balance will be decreased by more than was actually transferred to a user. In other words, part of users' rewards will be lost.

Recommended Mitigation Steps

Now:

function _sendJoeReward(address _rewardOwner, address _to) internal {
		userInfo[_rewardOwner].unclaimedJOEReward = 0;
		_safeJoeTransfer(_to, joeToSend);
...
function _safeJoeTransfer(address _to, uint256 _amount) internal {
		uint256 joeBal = JOE.balanceOf(address(this));
		if (_amount > joeBal) {
				JOE.transfer(_to, joeBal);
		} else {
				JOE.transfer(_to, _amount);
		}
}

To be:

function _sendJoeReward(address _rewardOwner, address _to) internal {
		uint256 joeSent = _safeJoeTransfer(_to, joeToSend);
		userInfo[_rewardOwner].unclaimedJOEReward = joeToSend - joeSent;
...
function _safeJoeTransfer(address _to, uint256 _amount) internal returns (uint256) {
		uint256 toSend = JOE.balanceOf(address(this));
		if (toSend > _amount) {
				toSend = _amount;
		}
		bool success = JOE.transfer(_to, toSend);
		require(success, "JOE transfer failed");
		return toSend;
}

JLP safe transfer function can approach token balance availability and transfer result in the similar manner.