xiaoming90 - Protocol could be DOS by transfer error due to lack of code length check

[sherlock-admin3]

xiaoming90

Medium

Protocol could be DOS by transfer error due to lack of code length check

Summary

The protocol could be DOS due to inadequate handling of transfer errors as it does not perform code length check. As a result, many critical features such as deposit, redemption, and liquidation will be broken, leading to assets being stuck or lost of assets.

Vulnerability Detail

Per the comment at Lines 312-315 below, the transfer error must be ignored to ensure that the error does not prevent normal vault operations from working. This is crucial because many critical features such as deposit, redemption, and liquidation will revert if the error is not handled properly, which could lead to assets being stuck or loss of assets.

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/common/VaultRewarderLib.sol#L316

File: VaultRewarderLib.sol
311:         if (0 < rewardToClaim) {
312:             // Ignore transfer errors here so that any strange failures here do not
313:             // prevent normal vault operations from working. Failures may include a
314:             // lack of balances or some sort of blacklist that prevents an account
315:             // from receiving tokens.
316:             try IEIP20NonStandard(rewardToken).transfer(account, rewardToClaim) {
317:                 bool success = TokenUtils.checkReturnCode();
318:                 if (success) {
319:                     emit VaultRewardTransfer(rewardToken, account, rewardToClaim);
320:                 } else {
321:                     emit VaultRewardTransfer(rewardToken, account, 0);
322:                 }
323:             // Emits zero tokens transferred if the transfer fails.
324:             } catch {
325:                 emit VaultRewardTransfer(rewardToken, account, 0);
326:             }
327:         }

Line 326 above attempts to mitigate the transfer error by "wrapping" the transfer call within the try-catch block. If the transfer function reverts, it will not revert the entire transaction and break the critical features of the protocol.

However, this approach was found to be insufficient to mitigate all cases of transfer error. There is still an edge case where an error could occur during transfer, reverting the entire transaction.

If the rewardToken points to an address that does not contain any code (codesize == 0), the transaction will revert instead of going into the try-catch block due to how Solidity works. It is possible that some reward tokens may contain self-destruct feature for certain reasons, resulting in the codesize becoming zero at some point in time.

Impact

If the edge case occurs, many critical features such as deposit, redemption, and liquidation will be broken, leading to assets being stuck or lost of assets.

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/common/VaultRewarderLib.sol#L316

Tool used

Manual Review

Recommendation

Ensure that the transfer error does not revert the entire transaction under any circumstance. Consider implementing the following changes:

        if (0 < rewardToClaim) {
            // Ignore transfer errors here so that any strange failures here do not
            // prevent normal vault operations from working. Failures may include a
            // lack of balances or some sort of blacklist that prevents an account
            // from receiving tokens.
+           if (rewardToken.code.length > 0) {}
              try IEIP20NonStandard(rewardToken).transfer(account, rewardToClaim) {
                  bool success = TokenUtils.checkReturnCode();
                  if (success) {
                      emit VaultRewardTransfer(rewardToken, account, rewardToClaim);
                  } else {
                      emit VaultRewardTransfer(rewardToken, account, 0);
                  }
              // Emits zero tokens transferred if the transfer fails.
              } catch {
                  emit VaultRewardTransfer(rewardToken, account, 0);
              }
+           }
        }

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

Low/QA at most

[xiaoming9090]

Escalate.

This issue should be a Medium.

Although the probability of encountering such a reward token is low, it is still possible for such a scenario to occur (edge case). If it happens, the impact will be serious, causing many critical features such as deposit, redemption, and liquidation to be broken, leading to assets being stuck or lost of assets.

Per Sherlock's judging rules, issues that require certain external conditions or specific states that could lead to a loss of assets should be judged as Medium, which should be the case here:

1. Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

[sherlock-admin3]

Escalate.

This issue should be a Medium.

Although the probability of encountering such a reward token is low, it is still possible for such a scenario to occur (edge case). If it happens, the impact will be serious, causing many critical features such as deposit, redemption, and liquidation to be broken, leading to assets being stuck or lost of assets.

Per Sherlock's judging rules, issues that require certain external conditions or specific states that could lead to a loss of assets should be judged as Medium, which should be the case here:

1. Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

You've created a valid escalation!

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.

[mystery0x]

Escalate.

This issue should be a Medium.

Although the probability of encountering such a reward token is low, it is still possible for such a scenario to occur (edge case). If it happens, the impact will be serious, causing many critical features such as deposit, redemption, and liquidation to be broken, leading to assets being stuck or lost of assets.

Per Sherlock's judging rules, issues that require certain external conditions or specific states that could lead to a loss of assets should be judged as Medium, which should be the case here:

1. Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

Will have the sponsors look into this finding... but it seems to me that making sure rewardToken.code.length != 0 is a low issue equivalent to sanity check on zero value/address.

Additionally, 0 < rewardToClaim signifies that rewardToken is already intact assuredly having rewardToken.code.length > 0:

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/common/VaultRewarderLib.sol#L302-L304

        rewardToClaim = _getRewardsToClaim(
            rewardToken, account, vaultSharesBefore, rewardsPerVaultShare
        );

[WangSecurity]

The first question is, who sets the reward tokens? Notional admins, correct?

Secondly, if the reward token's contract was self-destructed, it wouldn't be even possible to withdraw such rewards. So with or without the try/catch block it leads to a loss of funds?

Thirdly, this destructed token can be replaced, correct?

Fourthly, I see code comments saying that the code should ignore reverts, but need to remark, that breaking statements from the code comments, doesn't mean the issue automatically warrants medium severity.

[WangSecurity]

@xiaoming9090 looking deeper into this issue, I'm unsure the reward tokens can be replaced, which leads to users not being able to get the reward tokens at all, correct? But it's only possible if the reward token has the selfdestruct opcode?

[xiaoming9090]

@WangSecurity

looking deeper into this issue, I'm unsure the reward tokens can be replaced, which leads to users not being able to get the reward tokens at all, correct? But it's only possible if the reward token has the selfdestruct opcode?

As stated in the comments here or you may refer to the implementation of the updateRewardToken function, once a reward token is set, the address of the reward token can never be changed. Only the emission rate can be updated.

However, the main concern that I'm trying to highlight in this report is not the loss of reward tokens, but DOS or the breaking of the protocol's core functionality.

The sponsor went to great lengths to ensure that the token transfer would never revert under any circumstance. One has to understand why the sponsor makes such a great effort to do so and is even willing to forgo the reward tokens to ensure the continuity of the protocol. The reason is that if the token transfer reverts, for whatever reason, the entire protocol will be brick because the deposit, redeem, and liquidation (core) features rely on this claim reward function.

Thus, naturally, one would review whether the sponsor's current approach will cover all the edge cases. It was found that there is an edge case if a reward token self-destructs. The reward token's code will be empty, and the token transfer will revert, resulting in the deposit, redemption and liquidation (core) features being broken.

        if (0 < rewardToClaim) {
            // Ignore transfer errors here so that any strange failures here do not
            // prevent normal vault operations from working. Failures may include a
            // lack of balances or some sort of blacklist that prevents an account
            // from receiving tokens.
            try IEIP20NonStandard(rewardToken).transfer(account, rewardToClaim) {
                bool success = TokenUtils.checkReturnCode();
                if (success) {
                    emit VaultRewardTransfer(rewardToken, account, rewardToClaim);
                } else {
                    emit VaultRewardTransfer(rewardToken, account, 0);
                }
            // Emits zero tokens transferred if the transfer fails.
            } catch {
                emit VaultRewardTransfer(rewardToken, account, 0);
            }
        }

[WangSecurity]

Thank you for that clarification.

Agree with the escalation, planning to accept it with medium severity.

[WangSecurity]

Result: Medium Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: accepted

[WangSecurity]

@xiaoming9090 @mystery0x @brakeless-wtp are there any duplicates?

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/leveraged-vaults/pull/101

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.