GMX RewardRouterV2 allows redeeming to a different address, leading to incorrect tokensIn

Summary

GMX's unstakeAndRedeemGlp() function allows a withdrawer to withdraw their tokens to a receiver account that isn't their own, while the corresponding Controller assumes that all tokens are withdrawn to the Sentiment account that called the function.

Vulnerability Detail

The RewardRouterV2Controller.sol contract interacts with GMX's RewardRouterV2 contract. One of the permitted functions is unstakeAndRedeemGlp(), which takes the following arguments:

unstakeAndRedeemGlp(address _tokenOut, uint256 _glpAmount, uint256 _minOut, address _receiver)

The receiver argument specifies where the withdrawn tokens should be sent, which can be any address.

The RewardRouterV2Controller's canCallRedeem() function incorrectly assumes that the specified tokens will be withdrawn to the user's Sentiment account, regardless of the address entered as the receiver.

Impact

Accounting on user accounts can be thrown off (intentionally or unintentionally), resulting in mismatches between their assets array and hasAsset mapping and the reality of their account.

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/RewardRouterV2Controller.sol#L82-L91

Tool used

Manual Review

Recommendation

When decoding the data from the call to GMX, confirm that receiver == msg.sender.

function canCallRedeem(bytes calldata data)
    internal
    view
    returns (bool, address[] memory, address[] memory)
{
    address[] memory tokensIn = new address[](1);
-    (tokensIn[0],,,) = abi.decode(data, (address, uint256, uint256, uint256));
+    (tokensIn[0],,,address receiver) = abi.decode(data, (address, uint256, uint256, address));
+    if (receiver != msg.sender) return (true, new address[](0), sGLP);

    return (true, tokensIn, sGLP);
}

Note: The decoding is currently slightly off, as it decodes the final value as a uint256: (tokensIn[0],,,) = abi.decode(data, (address, uint256, uint256, uint256));

However, the function has the following signature: unstakeAndRedeemGlp(address _tokenOut,uint256 _glpAmount,uint256 _minOut,address _receiver)

This will need to be adjusted if you follow the recommendation above to use the receiver value.

Duplicate of #5

r0ohafza commented 1 year ago

Agree with the issue mentioned. Disagree with the fix provided since the receiver can be any other account and still lead to an accounting error, I think the recommended fix mentioned by @zobront on issue https://github.com/sherlock-audit/2023-01-sentiment-judging/issues/10 will resolve this as well.

hrishibhat commented 1 year ago

Considering this issue as a duplicate of #5 as the underlying issue is the same occurring in multiple places

sherlock-audit / 2023-01-sentiment-judging

obront - GMX RewardRouterV2 allows redeeming to a different address, leading to incorrect tokensIn #4