All Rage Trade functions allow sending tokens to a different address, leading to incorrect tokensIn

Summary

All three approved functions on Rage Trade (depositTokens(), withdrawTokens() and redeemTokens()) allow for a receiver argument to be included, which sends the resulting tokens to that address. The corresponding Controller assumes that all tokens are withdrawn to the Sentiment account that called the function.

Vulnerability Detail

The three functions that can be called on Rage Trade have the following signatures:

depositToken(address token, address receiver, uint256 amount)
redeemToken(address token, address receiver, uint256 amount)
withdrawToken(address token, address receiver, uint256 amount)

Each of these functions contains a receiver argument, which can be passed any address that will receive the outputted tokens.

The DNGMXVaultController incorrectly assumes in all cases that the outputted tokens will be received by the Sentiment account in question, regardless of what is entered as a 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-52/src/rage/DNGMXVaultController.sol#L60-L73

https://github.com/sherlock-audit/2023-01-sentiment/blob/main/controller-52/src/rage/DNGMXVaultController.sol#L75-L88

Tool used

Manual Review

Recommendation

When decoding the data from the call to Rage Trade, confirm that receiver == msg.sender. Here's an example with the deposit function:

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

    address[] memory tokensOut = new address[](1);
    tokensOut[0] = token;

    return (true, vault, tokensOut);
}

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 #10 will resolve this as well.

bahurum commented 1 year ago

Escalate for 50 USDC. I believe that this issue is Low severity. I filed it as a low severity myself (https://github.com/sherlock-audit/2023-01-sentiment-judging/issues/26). See the 3rd point in the Vulnerability Detail section. 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 previously judged as Medium. 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 it as a low severity myself (https://github.com/sherlock-audit/2023-01-sentiment-judging/issues/26). See the 3rd point in the Vulnerability Detail section. 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 previously judged as Medium. 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.

hrishibhat commented 1 year ago

Escalation accepted

While the above issue is not considered a low in this case, considering issue #26 as a valid medium based on further discussions in issue #10

sherlock-admin commented 1 year ago

Escalation accepted

While the above issue is not considered a low in this case, considering issue #26 as a valid medium based on further discussions in issue #10

This issue's escalations have been accepted!

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

sherlock-audit / 2023-01-sentiment-judging

obront - All Rage Trade functions allow sending tokens to a different address, leading to incorrect tokensIn #5