search

sherlock-audit / 2023-01-sentiment-judging

2 stars 0 forks source link

obront - GMX Reward Router's claimForAccount() can be abused to incorrectly add WETH to tokensIn #10

Open github-actions[bot] opened 1 year ago

github-actions[bot] commented 1 year ago

obront

medium

GMX Reward Router's claimForAccount() can be abused to incorrectly add WETH to tokensIn

Summary

When claimFees() is called, the Controller automatically adds WETH to the user's account. However, in the case where no fees have accrued yet, there will not be WETH withdrawn. In this case, the user will have WETH added as an asset in their account, while they won't actually have any WETH holdings.

Vulnerability Detail

When a user calls the GMX Reward Router's claimFees() function, the RewardRouterController confirms the validity of this call in the canCallClaimFees() function:

function canCallClaimFees()
    internal
    view
    returns (bool, address[] memory, address[] memory)
{
    return (true, WETH, new address[](0));
}

This function assumes that any user calling claimFees() will always receive WETH. However, this is only the case if their stake has been accruing.

Imagine the following two actions are taken in the same block:

The result will be that claimFees() returns no WETH, but WETH is added to the account's asset list.

The same is true if a user performs the following three actions:

Impact

A user can force their account into a state where it has WETH on the asset list, but doesn't actually hold any WETH.

This specific Impact was judged as Medium for multiple issues in the previous contest:

Code Snippet

https://github.com/sherlock-audit/2023-01-sentiment/blob/main/controller-55/src/gmx/RewardRouterController.sol#L54-L60

Tool used

Manual Review

Recommendation

The best way to solve this is actually not at the Controller level. It's to solve the issue of fake assets being added once and not have to worry about it on the Controller level in the future.

This can be accomplished in AccountManager.sol#_updateTokensIn(). It should be updated to only add the token to the assets list if it has a positive balance, as follows:

function _updateTokensIn(address account, address[] memory tokensIn)
    internal
{
    uint tokensInLen = tokensIn.length;
    for(uint i; i < tokensInLen; ++i) {
-        if (IAccount(account).hasAsset(tokensIn[i]) == false)
+        if (IAccount(account).hasAsset(tokensIn[i]) == false && IERC20(token).balanceOf(account) > 0)
            IAccount(account).addAsset(tokensIn[i]);
    }
}

However, _updateTokensIn() is currently called before the function is executed in exec(), so that would need to be changed as well:

function exec(address account, address target, uint amt, bytes calldata data) external onlyOwner(account) {
    bool isAllowed;
    address[] memory tokensIn;
    address[] memory tokensOut;
    (isAllowed, tokensIn, tokensOut) = controller.canCall(target, (amt > 0), data);
    if (!isAllowed) revert Errors.FunctionCallRestricted();
-    _updateTokensIn(account, tokensIn);
    (bool success,) = IAccount(account).exec(target, amt, data);
    if (!success)
        revert Errors.AccountInteractionFailure(account, target, amt, data);
+    _updateTokensIn(account, tokensIn);
    _updateTokensOut(account, tokensOut);
    if (!riskEngine.isAccountHealthy(account))
        revert Errors.RiskThresholdBreached();
}

While this fix does require changing a core contract, it would negate the need to worry about edge cases causing incorrect accounting of tokens on any future integrations, which I think is a worthwhile trade off.

This accuracy is especially important as Sentiment becomes better known and integrated into the Arbitrum ecosystem. While I know that having additional assets doesn't cause internal problems at present, it is hard to predict what issues inaccurate data will cause in the future. Seeing that Plutus is checking Sentiment contracts for their whitelist drove this point home — we need to ensure the data stays accurate, even in edge cases, or else there will be trickle down problems we can't currently predict.

bahurum commented 1 year ago

Escalate for 50 USDC. I believe that this issue is Low severity. I filed a list of issues with same impact as a Low severity submission (https://github.com/sherlock-audit/2023-01-sentiment-judging/issues/26). This issue will never lead to loss or lockup of funds. So by Sherlock judging criteria this is Low severity. Otherwise, please provide a scenario where this issue leads to a loss or lockup of funds. The watson mentions some similiar issues judged as Medium in previous contests. I didn't see those before or I would have escalated them as well for the same reason.

sherlock-admin commented 1 year ago

Escalate for 50 USDC. I believe that this issue is Low severity. I filed a list of issues with same impact as a Low severity submission (https://github.com/sherlock-audit/2023-01-sentiment-judging/issues/26). This issue will never lead to loss or lockup of funds. So by Sherlock judging criteria this is Low severity. Otherwise, please provide a scenario where this issue leads to a loss or lockup of funds. The watson mentions some similiar issues judged as Medium in previous contests. I didn't see those before or I would have escalated them as well for the same reason.

You've created a valid escalation for 50 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

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

zobront commented 1 year ago

Baharum is correct that within the protocol, as it stands, there are no risks of an account having an incorrect Assets list.

But Sentiment is becoming increasingly incorporated into the Arbitrum DeFi ecosystem. Sentiment purports that their account Asset lists are accurate and they go to extreme lengths with the Controller setup (and take on additional security risk) to track tokens in and out to keep it accurate. If it didn’t matter, they could just use the list of all allowed tokens when taking operations on an account.

As I stated in my Recommendations:

[W]e need to ensure the data stays accurate, even in edge cases, or else there will be trickle down problems we can't currently predict.

I believe this is a valid Medium, and is the kind of thing that Sentiment needs to understand when releasing new Controllers with the explicit goal of keeping Asset lists accurate as users interact with a given protocol.

bahurum commented 1 year ago

Thank you for your reply.

While I agree that data accuracy is desired, I'd like to insist that IMO the impact of this issue is not within Sherlock's definition of Medium severity issue, since there is no path to loss of funds, unless the contrary is shown.

For integration, if an asset with zero balance is in an account's asset list, I don't see that as a problem since an external protocol can check if the balance is positive if a zero balance ever causes issues.

If Sherlock thinks differently then I encourage them to reconsider my Low severity issue #26 along with your 2 issues #10 and #5 as a Medium.

zobront commented 1 year ago

I agree that your Low should be Medium.

hrishibhat commented 1 year ago

Escalation accepted.

Considering issue #26 as a valid medium in this case

sherlock-admin commented 1 year ago

Escalation accepted.

Considering issue #26 as a valid medium in this case

This issue's escalations have been accepted!

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