bin2chen - Immediate payment of REWARD_TOKEN may block most of nToken's operations

[sherlock-admin2]

bin2chen

medium

Immediate payment of REWARD_TOKEN may block most of nToken's operations

Summary

When nToken has a balance change, the protocol will immediately execute REWARD_TOKEN.transfer() to the user. If REWARD_TOKEN has a pause or blacklist mechanism, it will cause all of the user's nToken methods such as mint/withdraw to fail.

Vulnerability Detail

When the user's nToken balance changes, the following method will be executed: any nToken balance change ->Incentives.claimIncentives() -> SecondaryRewarder.claimRewards() -> REWARD_TOKEN.transfer(user)

    function _claimRewards(
        address account,
        uint16 currencyId,
        uint256 nTokenBalanceBefore,
        uint256 nTokenBalanceAfter,
        uint256 priorNTokenSupply
    ) external override onlyNotional {
...
        if (0 < rewardToClaim) {
@>          GenericToken.safeTransferOut(REWARD_TOKEN, account, rewardToClaim);
            emit RewardTransfer(REWARD_TOKEN, account, rewardToClaim);
        }

This poses a significant risk, blocking nToken's operations.

On Arbitrum, this will be ARB as a result of the ARB STIP grant. In the future, this may be any arbitrary ERC20 token.

1. Many tokens have a blacklist mechanism, such as USDC.
2. Many tokens' transfer() will have a pause mechanism.

As long as the above situations occur, it will cause most of nToken's methods to revert, such as mint/withdraw/transfer, etc.

These core businesses should not be affected by SecondaryRewarder.

It is recommended to use the claimable[] method, only recording the amount that can be claimed.

The user actively goes to claim() and then executes transfer().

Impact

Immediate payment of REWARD_TOKEN may block most of nToken's operations.

Code Snippet

https://github.com/sherlock-audit/2023-12-notional-update-5/blob/main/contracts-v3/contracts/external/adapters/SecondaryRewarder.sol#L231

Tool used

Manual Review

Recommendation

Use the common practice, add claimable[user] variable, _claimRewards() only records the quantity.

The user actively goes to execute claim and then transfers to the user.

Duplicate of #22

[jeffywu]

Acknowledged this is a very minor risk in the case this prevents a liquidation. However, I think the fix here is to put a try / catch around the reward block and it results in a loss of rewards for the blacklisted account rather than changing the entire UX of the process.

[sherlock-admin2]

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

takarez commented:

valid because { Invalid as the sherlock rule VII. number 9 says that when it only harms the user then its outy-of scope}