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

[github-actions]

obront

medium

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:

• obront - Curve LP staking allows adding false tokensIn to account by setting _claim_rewards to false 2022-11-sentiment-judging#20
• obront - User can trade away an asset using Balancer batchSwap without removing it from account 2022-11-sentiment-judging#7

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]

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]

Escalate for 50 USDC.
I believe that this issue is Low severity.
I filed it as a low severity myself (#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]

Escalate for 50 USDC.
I believe that this issue is Low severity.
I filed it as a low severity myself (#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]

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]

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.