steal RSR stakers funds by reentrancy attack in issue() function's token transfers

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L186-L280
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BackingManager.sol#L105-L150
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BasketHandler.sol#L271-L275

Vulnerability details

Impact

Function issue() loops through basket tokens and transfers them to BackingManager and mint RToken for user if issuance fit in one block, and transfer required tokens to protocol from user. because protocol is permission less and supports all ERC20 tokens it's possible for tokens to be ERC777 compliant or any other protocol tokens which have hook function during transfer, so transfer of tokens can cause external call to holder address. during the basket erc20 token list transfer's external call which triggers hook call, attacker can call backingManager.manageTokens() and because basketsNeeded updated but tokens balance in BackingManager is not yet updated code would detect that RTokens are not fully collateralized wrongly and would seize RSR stakers fund to become fully collateralized. This would cause RSR stakers to lose funds in favor of the RToken holders. attacker can drain RSR stakers funds totally and then withdraw his inflated RTokens.
This is reading reentrancy attack across contracts.

Proof of Concept

This is part of issue() code where creates RTokens in the same block:

.......
.......
        // ==== If the issuance can fit in this block, and nothing is blocking it, then
        // just do a "quick issuance" of iss instead of putting the issuance in the queue:
        // effects and actions if we go this way are the combined actions to create and vest iss:
        //   basketsNeeded += iss.amtBaskets
        //   mint(recipient, iss.amtRToken)
        //   for each token index i, erc20s[i].transferFrom(issuer, backingManager, iss.deposits[i])
        if (
            // D18{blocks} <= D18{1} * {blocks}
            vestingEnd <= FIX_ONE_256 * block.number &&
            queue.left == queue.right &&
            status == CollateralStatus.SOUND
        ) {
            // Fixlib optimization:
            // D18{BU} = D18{BU} + D18{BU}; uint192(+) is the same as Fix.plus
            uint192 newBasketsNeeded = basketsNeeded + amtBaskets;
            emit BasketsNeededChanged(basketsNeeded, newBasketsNeeded);
            basketsNeeded = newBasketsNeeded;

            // Note: We don't need to update the prev queue entry because queue.left = queue.right
            emit Issuance(recipient, amtRToken, amtBaskets);

            // == Interactions then return: transfer tokens ==
            // Complete issuance
            _mint(recipient, amtRToken);

            for (uint256 i = 0; i < erc20s.length; ++i) {
                IERC20Upgradeable(erc20s[i]).safeTransferFrom(
                    issuer,
                    address(backingManager),
                    deposits[i]
                );
            }

            // All RTokens instantly issued
            return amtRToken;
        }
...........
...........

As you can see when issuance can fit in the same block code would increase basketsNeeded and then tries to transfer baskets erc20 tokens from issuer to BackingManager address. If one of those tokens were ERC777 then that token would call token holder hook function in token transfers. there may be other protocol tokens that calls registered hook functions of sender or receiver during the token transfer. as reserve protocol is permission less and tries to work with all tokens so the external call in the token transfer can hook sender registered hook functions. attacker can use this and perform reentrancy attack. This is fullyCollateralized() code in BasketHandler contract:

    function fullyCollateralized() external view returns (bool) {
        return basketsHeldBy(address(backingManager)) >= rToken.basketsNeeded();
    }

As you can see it reads basketsNeeded from RToken contract and calculated baskets held by BackingManager current token balances by comparing them decides that RTtoken is fully collateralized or not. This is _manageTokens() code which is called by manageTokens() which is callable by anyone:

    function _manageTokens(IERC20[] calldata erc20s) private {
........
.......
        if (basketHandler.fullyCollateralized()) {
            // == Interaction (then return) ==
            handoutExcessAssets(erc20s);
        } else {
            (bool doTrade, TradeRequest memory req) = RecollateralizationLibP1
                .prepareRecollateralizationTrade(this);

            if (doTrade) {
                // Seize RSR if needed
                if (req.sell.erc20() == rsr) {
                    uint256 bal = req.sell.erc20().balanceOf(address(this));
                    if (req.sellAmount > bal) stRSR.seizeRSR(req.sellAmount - bal);
                }

                tryTrade(req);
            } else {
                // Haircut time
                compromiseBasketsNeeded();
            }
        }
    }

As you can see when RTtoken is not fully collateralized code would seize RSR tokens from RSR stakers.
so to perform the attack, attacker would do these steps: (in one transaction by writing a smart contract)

1. tokens [SOME_ERC777, USDT] with quantity [1, 1] are in the basket right now.
2. attacker would register a hook for his address in SOME_ERC777 token to get called during transfers.
3. attacker contract would call issue() to create AMOUNT1 RToken (which can be fit in current block) and code would increase basketsNeeded value and then tries to transfer basket tokens from attacker address to BackingManager contract.
4. during the transfer of the SOME_ERC777 token, attacker registered hook function would get called and attacker contract would call backingManager.manageTokens() and _manageTokens() function would call fullyCollateralized() and because basketsNeeded updated but BackingManager token balances are not yet updated so code would detect that RToken is not fully collateralized and it would seize RSR stakers RSR tokens to trade.
5. then attacker hook function would return and issue() would transfer rest of the erc20 tokens from attacker address to contract address. even so RTokens were fully collateralized but code seized RSR stakers funds. after this attacker call manageTokens() again and code would distribute those extra funds between RToken holders by minting new RTokens and melting them.
6. the attack can have more impact if lastIssRate was higher. but even if it was the default value, attacker can stole RSR token up to worth of lastIssRate * totalSupply() RToken in each block from RSR holders.

Tools Used

VIM

Recommended Mitigation Steps

prevent reading reentrancy attack by central reentrancy guard or by proxy interface contract that has reentrancy guard.
or create contract state (similar to basket nonce) which changes after each interaction and check for contracts states change during the call. (start and end of the call)

[0xean]

Given that issue() respects the check-effects pattern and all state is updated prior to the loop that transfers tokens I am a bit skeptical that this attack would work without a coded POC.

That being said, since the balances cannot be updated atomically if an ERC777 token is present, there may be some risk here.

Downgrading to M and will use this to aggregate all of the many re-entrancy concerns raised.

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as primary issue

[0xean]

#297 has a bit cleaner description and POC of the issue

[tmattimore]

dupe of #347

[c4-judge]

0xean marked the issue as duplicate of #347