Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Usage of lastEligibleStatus can cause user to miss out on rewards on manualStopEmissionsFor invocation #121

Open
c4-bot-6 opened this issue Aug 15, 2024 · 2 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-30 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@c4-bot-6
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/reward/ChefIncentivesController.sol#L605-L608

Vulnerability details

Impact

Invoking manualStopEmissionsFor can cause the user to miss out on rewards from vaults even after the user becomes eligible

Proof of Concept

If lastEligibleStatus and isCurrentlyEligible returns true, only the balance of the specific vault is updated. The idea is that whenever both these are true, all the other balances would have already been updated to the current value and hence need not be re-queried again

    function handleActionAfter(address _user, uint256 _balance, uint256 _totalSupply) external {
        if (!validRTokens[msg.sender] && msg.sender != address(mfd)) revert NotRTokenOrMfd();


        if (_user == address(mfd) || eligibilityExempt[_user]) {
            return;
        }
        if (eligibilityMode == EligibilityModes.FULL) {
            bool lastEligibleStatus = eligibleDataProvider.lastEligibleStatus(_user);
            bool isCurrentlyEligible = eligibleDataProvider.refresh(_user);
            if (isCurrentlyEligible) {
                if (lastEligibleStatus) {
                    _handleActionAfterForToken(msg.sender, _user, _balance, _totalSupply);
                } else {
                    _updateRegisteredBalance(_user);
                }
            } else {

But this assumption is broken when manualStopEmissionsFor is called which will set the balance corresponding to each vault as 0

    function manualStopEmissionsFor(address _user, address[] memory _tokens) public isWhitelisted {
        if (_user == address(0)) revert AddressZero();
        uint256 length = _tokens.length;
        for (uint256 i; i < length; ) {
            address token = _tokens[i];
            
            ....

                uint256 newTotalSupply = pool.totalSupply - amount;
                user.amount = 0;
                user.rewardDebt = 0;
                pool.totalSupply = newTotalSupply;


                emit BalanceUpdated(token, _user, 0, newTotalSupply);
            }

Hence in this case, if an user's vault position update makes the user eligible for rewards, only that specific vault associated debt will be earning rewards and all the other vault balances won't be updated

Tools Used

Manual review

Recommended Mitigation Steps

The lastEligibleStatus check can be removed or it can be handled alongside the manualStopEmissionsFor implementation

Assessed type

Other

@c4-bot-6 c4-bot-6 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Aug 15, 2024
c4-bot-4 added a commit that referenced this issue Aug 15, 2024
@howlbot-integration howlbot-integration bot added primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Aug 20, 2024
@amarcu amarcu added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Sep 20, 2024
@c4-judge
Copy link
Contributor

koolexcrypto marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report labels Sep 25, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 2, 2024

koolexcrypto marked the issue as selected for report

@C4-Staff C4-Staff added the M-30 label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-30 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants