Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BackingManager.sol#L44 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BackingManager.sol#L107-L173 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BackingManager.sol#L85-L100

Vulnerability details

Description

The BackingManagerP1 contract is responsible for managing the backing of an RToken, including rebalancing collateral and settling trades. The contract uses a tokensOut mapping to track the amount of tokens involved in ongoing trades. However, the current implementation of the rebalance() function incorrectly updates this mapping, potentially leading to inaccurate token accounting.

In the rebalance() function, when a new trade is executed, the tokensOut mapping is updated as follows:

File: BackingManager.sol
165:             ITrade trade = tryTrade(kind, req, prices);
166:             tradeEnd[kind] = trade.endTime(); // {s}
167:             tokensOut[sellERC20] = trade.sellAmount(); // {tok}

This assignment overwrites any existing value in tokensOut for the given token, rather than adding to it. As a result, if multiple trades for the same token are executed before the first trade is settled, the tokensOut mapping will only reflect the amount from the most recent trade.

The settleTrade() function, which is called to finalize a trade, deletes the corresponding entry in the tokensOut mapping:

File: BackingManager.sol
85:     function settleTrade(IERC20 sell) public override(ITrading, TradingP1) returns (ITrade trade) {
86:         delete tokensOut[sell];
87:         trade = super.settleTrade(sell); // nonReentrant

    // ... (additional code)
}

This deletion can lead to incorrect accounting if multiple trades for the same token were executed, as it removes the entire entry, including amounts from unsettled trades.

The combination of these two issues can result in the contract losing track of the actual amount of tokens involved in ongoing trades, potentially leading to over-collateralization or under-collateralization of the RToken.

Impact

The incorrect accounting of tokens in ongoing trades can lead to significant discrepancies in the contract's understanding of its collateral. This misrepresentation of the contract's financial state can have severe consequences:

Over-collateralization: The contract may believe it has less collateral than it actually does, leading to unnecessary trades or seizure of additional collateral.
Under-collateralization: The contract may believe it has more collateral than it actually does, potentially allowing the minting of RTokens without sufficient backing.
Manipulation: An attacker could exploit this bug to artificially inflate or deflate the perceived collateral, potentially leading to unauthorized minting or burning of RTokens.

Proof of Concept

Consider the following scenario:

The BackingManagerP1 contract initiates Trade A for 100 TokenX.
- tokensOut[TokenX] = 100
Before Trade A is settled, another rebalance occurs, initiating Trade B for 50 TokenX.
- tokensOut[TokenX] = 50 (overwrites the previous value)
Trade A is settled:
- delete tokensOut[TokenX] (removes the entire entry, including the amount for Trade B)
The contract now believes there are no ongoing trades for TokenX, even though Trade B is still active.

This scenario results in the contract losing track of 50 TokenX, which could lead to incorrect collateralization calculations and potential exploitation.

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, the rebalance() function should update the tokensOut mapping additively instead of overwriting it. Here's the recommended fix:

diff --git a/contracts/p1/BackingManager.sol b/contracts/p1/BackingManager.sol
index abcdef1..1234567 100644
--- a/contracts/p1/BackingManager.sol
+++ b/contracts/p1/BackingManager.sol
@@ -123,7 +123,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager {
     // Execute Trade
     ITrade trade = tryTrade(kind, req, prices);
     tradeEnd[kind] = trade.endTime(); // {s}
-    tokensOut[sellERC20] = trade.sellAmount(); // {tok}
+    tokensOut[sellERC20] += trade.sellAmount(); // {tok}
 }

Additionally, the settleTrade() function should be updated to only reduce the tokensOut amount by the settled trade amount, rather than deleting the entire entry:

diff --git a/contracts/p1/BackingManager.sol b/contracts/p1/BackingManager.sol
index abcdef1..1234567 100644
--- a/contracts/p1/BackingManager.sol
+++ b/contracts/p1/BackingManager.sol
@@ -150,7 +150,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager {
 function settleTrade(IERC20 sell) public override(ITrading, TradingP1) returns (ITrade trade) {
-    delete tokensOut[sell];
+    tokensOut[sell] -= trade.sellAmount();
     trade = super.settleTrade(sell); // nonReentrant
     // ... (additional code)
 }

Assessed type

Other

code-423n4 / 2024-07-reserve-validation

Incorrect `tokensOut` Mapping Update Can Lead to Over-Collateralization or Under-Collateralization (`BackingManagerP1::rebalance`) #68