Unintended Token Transfer Can Disrupt Distribution Process (`RevenueTraderP1::returnTokens`)

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RevenueTrader.sol#L76-L93

Vulnerability details

Description

The RevenueTraderP1 contract is designed to manage and distribute revenue by converting all asset balances to a single target asset (tokenToBuy) and distributing it through the Distributor. The returnTokens function is intended to return registered ERC20 tokens to the BackingManager if the distribution for tokenToBuy is zero. However, the function does not prevent tokenToBuy from being included in the tokens being returned, which can disrupt the intended distribution process.

The returnTokens function checks if the total distribution for tokenToBuy is zero using revTotals. If the total distribution is zero, it proceeds to return the tokens in the erc20s array to the BackingManager. The function ensures that only registered ERC20 tokens are returned but does not explicitly exclude tokenToBuy from being returned.

This logical flaw allows tokenToBuy to be transferred back to the BackingManager, even if it has a non-zero balance intended for future distribution. This could lead to a situation where funds meant for distribution are incorrectly sent back to the BackingManager, disrupting the intended token flow and distribution process.

Relevant code:

File: RevenueTrader.sol
76:     function returnTokens(IERC20[] memory erc20s) external notTradingPausedOrFrozen {
77:         RevenueTotals memory revTotals = distributor.totals();
78:         if (tokenToBuy == rsr) {
79:             require(revTotals.rsrTotal == 0, "rsrTotal > 0");
80:         } else if (address(tokenToBuy) == address(rToken)) {
81:             require(revTotals.rTokenTotal == 0, "rTokenTotal > 0");
82:         } else {
83:             // untestable: tokenToBuy is always the RSR or RToken
84:             revert("invalid tokenToBuy");
85:         }
86: 
87:         // Return ERC20s to the BackingManager
88:         uint256 len = erc20s.length;
89:         for (uint256 i = 0; i < len; ++i) {
90:             require(assetRegistry.isRegistered(erc20s[i]), "unregistered erc20");
91:             erc20s[i].safeTransfer(address(backingManager), erc20s[i].balanceOf(address(this)));
92:         }
93:     }

Impact

This can disrupt the intended distribution process, leading to incorrect token management. If tokenToBuy is included in the erc20s array, it will be returned to the BackingManager, even if it has a non-zero balance intended for distribution. This could lead to an unintended transfer of funds, potentially causing a loss of tokens meant for distribution and disrupting the overall token flow.

Proof of Concept

1. tokenToBuy is set to RSR.
2. The contract has a balance of RSR tokens intended for distribution.
3. revTotals.rsrTotal is 0 (perhaps due to a recent update or a specific contract state).
4. A user calls returnTokens with an array that includes the RSR token.
5. The function will pass the initial check (revTotals.rsrTotal == 0).
6. It will then proceed to transfer all RSR tokens back to the BackingManager, even though these tokens were meant for distribution.

Tools Used

Manual review

Recommended Mitigation Steps

To prevent this issue, the function should be modified to exclude tokenToBuy from the tokens that can be returned, or to add an additional check to ensure that tokenToBuy has a zero balance before allowing the transfer.

Suggested fix:

function returnTokens(IERC20[] memory erc20s) external notTradingPausedOrFrozen {
    RevenueTotals memory revTotals = distributor.totals();
    if (tokenToBuy == rsr) {
        require(revTotals.rsrTotal == 0, "rsrTotal > 0");
    } else if (address(tokenToBuy) == address(rToken)) {
        require(revTotals.rTokenTotal == 0, "rTokenTotal > 0");
    } else {
        revert("invalid tokenToBuy");
    }

    // Return ERC20s to the BackingManager
    uint256 len = erc20s.length;
    for (uint256 i = 0; i < len; ++i) {
        require(assetRegistry.isRegistered(erc20s[i]), "unregistered erc20");
+       require(erc20s[i] != tokenToBuy, "cannot return tokenToBuy"); // Added check
        erc20s[i].safeTransfer(address(backingManager), erc20s[i].balanceOf(address(this)));
    }
}

This modification ensures that tokenToBuy cannot be returned to the BackingManager, preserving the intended distribution mechanism.

Assessed type

Other