Sherlock contest - hyh's results

Handle

hyh

Vulnerability details

Impact

Sucessfull arbRestake performs _redeemShares for arbRewardShares amount to extract the arbitrager reward. This effectively reduces shares accounted for an NFT, but leaves untouched the addressShares of an nftOwner.

As a result the tokenBalanceOfAddress function will report an old balance that existed before arbitrager reward was slashed away. This will persist if the owner will transfer the NFT to someone else as its new reduced shares value will be subtracted from addressShares in _beforeTokenTransfer, leaving the arbitrage removed shares permanently in addressShares of the NFT owner, essentially making all further reporting of his balance incorrectly inflated by the cumulative arbitrage reward shares from all arbRestakes happened to the owner's NFTs.

Proof of Concept

arbRestake redeems arbRewardShares, which are a part of total shares of an NFT:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L673

This will effectively reduce the stakeShares:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L491

But there is no mechanics in place to reduce addressShares of the owner apart from mint/burn/transfer, so addressShares will still correspond to NFT shares before arbitrage. This discrepancy will be accumulated further with arbitrage restakes.

Recommended Mitigation Steps

Add a flag to _redeemShares indicating that it was called for a partial shares decrease, say isPartialRedeem, and do addressShares[nftOwner] -= _stakeShares when isPartialRedeem == true.

Another option is to do bigger refactoring, making stakeShares and addressShares always change simultaneously.

Handle

hyh

Vulnerability details

Impact

Part of the funds held with the strategy can be frozen if the current strategy has tight liquidity when updateYieldStrategy is run as this function makes an attempt to withdraw all the funds and then unconditionally removes the strategy.

The Sherlock to YieldStrategy link will be broken as a result: Sherlock points to the new Strategy, while old Strategy still allows only this Sherlock contract to withdraw.

This way back and forth switches will be required in the future to return the funds: withdraw all from new strategy and switch to old, withdraw all from old and point to new one again, reinvest there.

Proof of Concept

In peer-to-peer lending protocols it is not always possible for the token supplier to withdraw all what's due. This happens on high utilization of the market (when it has a kind of liquidity crunch).

This way yieldStrategy.withdrawAll is not guaranteed to obtain all the funds held with the strategy:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L286

The worst case scenario here seems to be the remainder funds to be left frozen within the strategy.

For example, AaveV2Strategy withdraw and withdrawAll have onlySherlockCore modifier:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/AaveV2Strategy.sol#L78-100

While Sherlock core is immutable for the Strategy by default:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/Manager.sol#L26-41

Recommended Mitigation Steps

Consider implementing a new method that fails whenever a strategy cannot withdraw all what's due now, and rename current implementation to, for example, forceUpdateYieldStrategy, to have a degree of flexibility around liquidity issues.

Also, to avoid back and forth switching, a strategy argument can be introduced to yieldStrategyWithdrawAll to allow withdrawals from any (not only current) yieldStrategy:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L322

Now:

function yieldStrategyWithdrawAll() external override onlyOwner {

To be (if _yieldStrategy is zero then utilize current):

function yieldStrategyWithdrawAll(IStrategyManager _yieldStrategy) external override onlyOwner {