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

Users may permanently lose a portion of rewards when claiming rewards if the contract balance is insufficient #361

Closed
code423n4 opened this issue May 11, 2023 · 4 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-251 satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L811-L821

Vulnerability details

Impact

The current implementation of the _transferAjnaRewards function in the smart contract may result in a permanent loss of rewards for users if the contract balance of ajnaToken is insufficient to cover the total rewards earned. In such a scenario, the user will only receive the remaining ajnaToken balance as a reward, while the unpaid portion will be permanently lost.

Proof of Concept

The relevant code snippet can be found in the _transferAjnaRewards function:

uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance;

if (rewardsEarned_ != 0) {
    // transfer rewards to sender
    IERC20(ajnaToken).safeTransfer(msg.sender, rewardsEarned_);
}

The function checks if the rewardsEarned_ is greater than the contract's ajnaToken balance (ajnaBalance). If so, the rewardsEarned_ is set equal to the remaining balance, effectively causing the user to permanently lose the unpaid portion of the rewards.

Tools Used

N/A

Recommended Mitigation Steps

To address this issue, it is recommended to keep track of the unpaid rewards separately, rather than permanently losing them due to an insufficient contract balance. The _transferAjnaRewards function can be modified to store the unpaid rewards for each user, allowing the possibility of future claims when the contract balance is replenished.

An example of a modified _transferAjnaRewards function could look like this:

function _transferAjnaRewards(uint256 rewardsEarned_) internal {
    // check that rewards earned isn't greater than remaining balance
    // if remaining balance is greater, set to remaining balance
    uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
    uint256 payableRewards = rewardsEarned_;
    uint256 unpaidRewards = 0;

    if (rewardsEarned_ > ajnaBalance) {
        payableRewards = ajnaBalance;
        unpaidRewards = rewardsEarned_ - ajnaBalance;
        // Store unpaid rewards for the user (e.g., using a mapping)
        userUnpaidRewards[msg.sender] += unpaidRewards;
    }

    if (payableRewards != 0) {
        // transfer rewards to sender
        IERC20(ajnaToken).safeTransfer(msg.sender, payableRewards);
    }
}

By implementing this change, users will be able to claim their unpaid rewards at a later time when the contract balance is sufficient.

Assessed type

Token-Transfer

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels May 11, 2023
code423n4 added a commit that referenced this issue May 11, 2023
@c4-judge
Copy link
Contributor

Picodes marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label May 12, 2023
@c4-sponsor
Copy link

MikeHathaway marked the issue as sponsor confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label May 19, 2023
@c4-judge
Copy link
Contributor

Picodes marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label May 29, 2023
@c4-judge c4-judge added duplicate-251 and removed primary issue Highest quality submission among a set of duplicates labels May 29, 2023
@c4-judge
Copy link
Contributor

Picodes marked issue #251 as primary and marked this issue as a duplicate of 251

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-251 satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants