gogo - Wrong type casting leads to unsigned integer underflow exception when current price is < last price

[sherlock-admin]

gogo

high

Wrong type casting leads to unsigned integer underflow exception when current price is < last price

Summary

When the current price of a locked token is lower than the last price, the Vault.storePriceAndRewards will revert because of the wrong integer casting.

Vulnerability Detail

The following line appears in Vault.storePriceAndRewards:

int256 priceDiff = int256(currentPrice - lastPrices[_protocolId]);

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Vault.sol#L233

If lastPrices[_protocolId] is higher than the currentPrice, the solidity compiler will revert due the underflow of subtracting unsigned integers because it will first try to calculate the result of currentPrice - lastPrices[_protocolId] and then try to cast it to int256.

Impact

The rebalance will fail when the current token price is less than the last one stored.

Code Snippet

int256 priceDiff = int256(currentPrice - lastPrices[_protocolId]);

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Vault.sol#L233

Tool used

Manual Review

Recommendation

Casting should be performed in the following way to avoid underflow and to allow the priceDiff being negative:

int256 priceDiff = int256(currentPrice) - int256(lastPrices[_protocolId]));

[GeorgeHNTR]

Escalate for 10 USDC Although I didn't have much time for this contest and may be missing some knowledge about the whole flow, I believe this finding is incorrectly downgraded from High to Medium. As mentioned in the docs for the rebalance() function (which calls rebalanceCheckProtocols() which calls the problematic storePriceAndRewards() for each protocol), "This is the most important function of the vault." This finding shows that the vault rebalancing will not work when the current price of any of the protocols' LP tokens reaches a high price and the new price has not increased since then. During a bear market, this could mean that the vault rebalancing won't work for a long time, while the docs say it should be "triggered once per two weeks." Please review and provide feedback, @sjoerdsommen @judge.

[sherlock-admin]

Escalate for 10 USDC Although I didn't have much time for this contest and may be missing some knowledge about the whole flow, I believe this finding is incorrectly downgraded from High to Medium. As mentioned in the docs for the rebalance() function (which calls rebalanceCheckProtocols() which calls the problematic storePriceAndRewards() for each protocol), "This is the most important function of the vault." This finding shows that the vault rebalancing will not work when the current price of any of the protocols' LP tokens reaches a high price and the new price has not increased since then. During a bear market, this could mean that the vault rebalancing won't work for a long time, while the docs say it should be "triggered once per two weeks." Please review and provide feedback, @sjoerdsommen @judge.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Theezr]

Fix: https://github.com/derbyfinance/derby-yield-optimiser/pull/195

[hrishibhat]

Escalation accepted

Considering this issue a valid high because rebalance is important and currentprice can be less than the last price regularly resulting in not being able to rebalance.

[sherlock-admin]

Escalation accepted

Considering this issue a valid high because rebalance is important and currentprice can be less than the last price regularly resulting in not being able to rebalance.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[dmitriia]

Fix looks good.