Reserve contest - unforgiven's results

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L160-L188 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L496-L530 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L212-L237

Vulnerability details

Impact

code calculates amount of stake token and rsr token based on stakeRate and if stakeRate was near 1e18 then division error is small but attacker can cause stakeRate to be 1 and that can cause users to loss up to 1e18 token during stake and unstake.

Proof of Concept

this is init() code:

    function init(
        IMain main_,
        string calldata name_,
        string calldata symbol_,
        uint48 unstakingDelay_,
        uint48 rewardPeriod_,
        uint192 rewardRatio_
    ) external initializer {
        require(bytes(name_).length > 0, "name empty");
        require(bytes(symbol_).length > 0, "symbol empty");
        __Component_init(main_);
        __EIP712_init(name_, "1");
        name = name_;
        symbol = symbol_;

        assetRegistry = main_.assetRegistry();
        backingManager = main_.backingManager();
        basketHandler = main_.basketHandler();
        rsr = IERC20(address(main_.rsr()));

        payoutLastPaid = uint48(block.timestamp);
        rsrRewardsAtLastPayout = main_.rsr().balanceOf(address(this));
        setUnstakingDelay(unstakingDelay_);
        setRewardPeriod(rewardPeriod_);
        setRewardRatio(rewardRatio_);

        beginEra();
        beginDraftEra();
    }

As you can see it sets the value of the rsrRewardsAtLastPayout as contract balance when contract is deployed. This is _payoutReward() code:

    function _payoutRewards() internal {
        if (block.timestamp < payoutLastPaid + rewardPeriod) return;
        uint48 numPeriods = (uint48(block.timestamp) - payoutLastPaid) / rewardPeriod;

        uint192 initRate = exchangeRate();
        uint256 payout;

        // Do an actual payout if and only if stakers exist!
        if (totalStakes > 0) {
            // Paying out the ratio r, N times, equals paying out the ratio (1 - (1-r)^N) 1 time.
            // Apply payout to RSR backing
            // payoutRatio: D18 = FIX_ONE: D18 - FixLib.powu(): D18
            // Both uses of uint192(-) are fine, as it's equivalent to FixLib.sub().
            uint192 payoutRatio = FIX_ONE - FixLib.powu(FIX_ONE - rewardRatio, numPeriods);

            // payout: {qRSR} = D18{1} * {qRSR} / D18
            payout = (payoutRatio * rsrRewardsAtLastPayout) / FIX_ONE;
            stakeRSR += payout;
        }

        payoutLastPaid += numPeriods * rewardPeriod;
        rsrRewardsAtLastPayout = rsrRewards();

        // stakeRate else case: D18{qStRSR/qRSR} = {qStRSR} * D18 / {qRSR}
        // downcast is safe: it's at most 1e38 * 1e18 = 1e56
        // untestable:
        //      the second half of the OR comparison is untestable because of the invariant:
        //      if totalStakes == 0, then stakeRSR == 0
        stakeRate = (stakeRSR == 0 || totalStakes == 0)
            ? FIX_ONE
            : uint192((totalStakes * FIX_ONE_256 + (stakeRSR - 1)) / stakeRSR);

        emit RewardsPaid(payout);
        emit ExchangeRateSet(initRate, exchangeRate());
    }

As you can see it sets the value of the stakeRate to (totalStakes * FIX_ONE_256 + (stakeRSR - 1)) / stakeRSR. So to exploit this attacker needs to perform this steps:

1. send 200 * 1e18 RSR tokens (18 is the precision) to the StRSR address before its deployment by watching mempool and front running. the deployment address is calculable before deployment.
2. function init() would get executed and would set 200 * 1e18 as rsrRewardsAtLastPayout.
3. then attacker would call stake() and stake 1 RSR token (1 wei) in the contract and the value of stakeRSR and totalStakes would be 1.
4. then attacker wait for rewardPeriod seconds and then call payoutReward() and code would pay rewards based on rewardRatio and rsrRewardsAtLastPayout and as rewardRatio is higher than 1% (default and normal mode) code would increase stakeRate more than 2 * 1e18 amount. and then code would set stakeRate as totalStakes * FIX_ONE_256 + (stakeRSR - 1)) / stakeRSR = 1.
5. then calls to stake() would cause users to lose up to 1e18 RSR tokens as code calculates stake amount as newTotalStakes = (stakeRate * newStakeRSR) / FIX_ONE and rounding error happens up to FIX_ONE. because the calculated stake amount is worth less than deposited rsr amount up to 1e18.
6. attacker can still users funds by unstaking 1 token and receiving 1e18 RSR tokens. because of the rounding error in unstake()

so attacker can manipulate the stakeRate in contract deployment time with sandwich attack which can cause other users to lose funds because of the big rounding error.

Tools Used

VIM

Recommended Mitigation Steps

prevent early manipulation of the PPS

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L563-L573 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L801-L814 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L219 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/Furnace.sol#L70-L84

Vulnerability details

Impact

Function melt() melt a quantity of RToken from the caller's account, increasing the basket rate. basket rate should be between 1e9 and 1e27 and function requireValidBUExchangeRate() checks that if it's not in interval the the code would revert. the call to requireValidBUExchangeRate() happens in the function mint(), melt() and setBasketsNeeded() which are used in issue() and handoutExcessAssets() and compromiseBasketsNeeded() which are used in multiple functionality of the systems. early malicious user can call issue(1e18) and melt(1e18 - 1) and then set the ratio between baskets needed and total supply to 1e27 and then any new action that increase the ratio would fail. because during the issue() code calls melt() so the issue() would fail for sure and other functionalities can increase the ratio because of the ratio too because of the rounding error which result in revert. so by exploiting this attacker can make RToken to be in broken state and most of the functionalities of the system would stop working.

Proof of Concept

This is melt() code:

    function melt(uint256 amtRToken) external notPausedOrFrozen {
        _burn(_msgSender(), amtRToken);
        emit Melted(amtRToken);
        requireValidBUExchangeRate();
    }

As you can see it allows anyone to burn their RToken balance. This is requireValidBUExchangeRate() code:

    function requireValidBUExchangeRate() private view {
        uint256 supply = totalSupply();
        if (supply == 0) return;

        // Note: These are D18s, even though they are uint256s. This is because
        // we cannot assume we stay inside our valid range here, as that is what
        // we are checking in the first place
        uint256 low = (FIX_ONE_256 * basketsNeeded) / supply; // D18{BU/rTok}
        uint256 high = (FIX_ONE_256 * basketsNeeded + (supply - 1)) / supply; // D18{BU/rTok}

        // 1e9 = FIX_ONE / 1e9; 1e27 = FIX_ONE * 1e9
        require(uint192(low) >= 1e9 && uint192(high) <= 1e27, "BU rate out of range");
    }

As you can see it checks and makes sure that the BU to RToken exchange rate to be in [1e-9, 1e9]. so Attacker can perform this steps:

1. add 1e18 RToken as first issuer by calling issue()
2. call melt() and burn 1e18 - 1 of his RTokens.
3. not basketsNeeded would be 1e18 and totalSupply() of RTokens would be 1 and the BU to RToken exchange rate would be its maximum value 1e27 and requireValidBUExchangeRate() won't allow increasing the ratio.
4. now calls to melt() would revert and because issue() calls to furnace.melt() which calls RToken.melt() so all calls to issue() would revert. other functionality which result in calling mint(), melt() and setBasketsNeeded() if they increase the ratio would fail too. as there is rounding error when converting RToken amount to basket amount so burning and minting new RTokens and increase the ratio too because of those rounding errors and those logics would revert. (handoutExcessAssets() would revert because it mint revenue RToken and update basketsNeeded and it calculates new basket amount based on RToken amounts and rounds down so it would increase the BU to RToken ratio which cause code to revert in mint()) (redeem() would increase the ratio simillar to handoutExcessAssets() because of rounding down)
5. the attacker doesn't need to be first issuer just he needs to be one of the early issuers and by performing the attack and also if the ratio gets to higher value of the maximum allowed the protocol won't work properly as it documented the supported rage for variables to work properly.

so attacker can make protocol logics to be broken and then RToken won't be useless and attacker can perform this attack to any newly deployed RToken.

Tools Used

VIM

Recommended Mitigation Steps

don't allow everyone to melt their tokens or don't allow melting if totalSupply() become very small.

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L655-L714 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L532-L544 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L186-L280

Vulnerability details

Impact

Function refundSpan() loops through tokens and transfer them to caller and if one of tokens were ERC777 or any other 3rd party protocol token with hook, attacker can perform reentrancy attack during token transfers. Attacker can cause multiple impacts by choosing the reentrancy function:

1. in one scenario because code uses queue's ERC20 token list from storage attacker can change list during the token transfers and steal users funds, to do this attacker can cancel all his vesting issuance which is for old basket nonce and during the refundSpan()'s token transfer external calls, attacker can perform reentrancy and call issue() and set new basket nonce (because queue is empty) and erc20 list for queue and then refundSpan() would transfer another tokens to attacker in the rest of the execution as it reads ERC20 tokens from storage. and attacker would steal other users funds.
2. in one scenario because refundSpan() sets the value of the liabilities[] but during the token transfers, the balance of the tokens didn't get updated so attacker can call sweepRewards() by reentrancy, and make contract send collateral vesting tokens as rewards to BackingManager and some users deposits would be lost by this.

Proof of Concept

Function refundSpan() refunds all the deposits in the specified span. This is refundSpan() code:

    function refundSpan(
        address account,
        uint256 left,
        uint256 right
    ) private {
        if (left >= right) return; // refund an empty span

        IssueQueue storage queue = issueQueues[account];

        // compute total deposits to refund
        uint256 tokensLen = queue.tokens.length;
        uint256[] memory amt = new uint256[](tokensLen);
        uint256 amtRToken; // {qRTok}
        IssueItem storage rightItem = queue.items[right - 1];

        // compute item(right-1) - item(left-1)
        // we could dedup this logic for the zero item, but it would take more SLOADS
        if (left == 0) {
            amtRToken = rightItem.amtRToken;
            for (uint256 i = 0; i < tokensLen; ++i) {
                amt[i] = rightItem.deposits[i];

                // Decrement liabilities
                liabilities[IERC20(queue.tokens[i])] -= amt[i];
            }
        } else {
            IssueItem storage leftItem = queue.items[left - 1];
            amtRToken = rightItem.amtRToken - leftItem.amtRToken;
            for (uint256 i = 0; i < tokensLen; ++i) {
                amt[i] = rightItem.deposits[i] - leftItem.deposits[i];

                // Decrement liabilities
                liabilities[IERC20(queue.tokens[i])] -= amt[i];
            }
        }

        if (queue.left == left && right == queue.right) {
            // empty entire queue
            queue.left = 0;
            queue.right = 0;
        } else if (queue.left == left && right < queue.right) {
            queue.left = right; // remove span from beginning
        } else if (queue.left < left && right == queue.right) {
            queue.right = left; // refund span from end
        } else {
            // untestable:
            //      All calls to refundSpan() use valid values for left and right.
            //      queue.left <= left && right <= queue.right.
            //      Any call to refundSpan() passes queue.left for left,
            //      OR passes queue.right for right, OR both.
            revert("Bad refundSpan");
        } // error: can't remove [left,right) from the queue, and leave just one interval

        emit IssuancesCanceled(account, left, right, amtRToken);

        // == Interactions ==
        for (uint256 i = 0; i < queue.tokens.length; ++i) {
            IERC20Upgradeable(queue.tokens[i]).safeTransfer(account, amt[i]);
        }
    }

As you can see if user refunds all his spans then code would set queue.left = queue.right = 0 and also code would update liabilities[] based on the user's deposits. in the last part code would try to send user deposited tokens based on the queue.tokens[] values and because queue is defined as storage so code uses values from storage. if one of those tokens were ERC777 then that token would account hook function in token transfer. there may be other protocol tokens that calls registered hook functions 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 account functions. attacker can use this hook and perform reentrancy attack. This is Part of issue() function:

        // overwrite intentionally: we may have stale values in `tokens` and `basketNonce`
        queue.basketNonce = basketNonce;
        queue.tokens = erc20s;
        queue.right++;

As you can see it sets the value of the queue.tokens and queue.basketNonce to new values. This is part of sweepRewards() code in RewardableLibP1 which is used in RTokens speedRewards():

        for (uint256 i = 0; i < erc20sLen; ++i) {
            deltas[i] = erc20s[i].balanceOf(address(this)) - liabilities[erc20s[i]]; // {qTok}
        }

        // == Interactions ==
        // Sweep deltas
        for (uint256 i = 0; i < erc20sLen; ++i) {
            if (deltas[i] > 0) {
                erc20s[i].safeTransfer(address(bm), deltas[i]);

                // Verify nothing has gone wrong -- we should keep this assert, even in P1
                assert(erc20s[i].balanceOf(address(this)) >= liabilities[erc20s[i]]);
            }
        }

As you can see it calculated rewards based on balance of the contract and liabilities[] and sends the difference to BackingManager contract as rewards. To exploit this issue attacker can choose different functions to call during reentrancy and each choice has different impact and would cause different damage.

scenario #1: attacker use issue() function to reentrancy. the steps are:

1. tokens [SOME_ERC777, USDT] with quantity [1, 1] are in the basket right now and basket nonce is BasketNonce1.
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 100 RToken and code would set attacker vesting queue values. queue.basketNonce would be BasketNonce1 and queue.tokens would be [SOME_ERC777, USDT] and queue.left = 0 and queue.right = 1. also code would transfer 100 USDT and 100 SOME_ERC777 tokens from attacker address to RToken address and set the items of the queue accordingly.
4. attacker would wait until the governance changes the basket for any reason. let's assume the new basket is [USDT, WETH] with basket nonce as BasketNonce2. then attacker would wait for so users would call issue() and contract would has some balance in USDT and WETH because of vesting.
5. attacker would call vest(attackerAddress, 1) to refund his deposits. code would check and see that queue's nonce is not equal to basketHandler's nonce and would call refundSpan(attackerAddress, 0, 1).
6. function refundSpan() would set queue.left = queue.right = 0 and also update liabilities[SOME_ERC77] -= 100 and liabilities[USDT] -= 100 and then tires to transfer user deposits and when contract tries to transfer SOME_ERC777 tokens to attacker address, the ERC777 token hook function would call attacker specified contract.
7. attacker contract would call issue() to create new vesting for himself. issue() code would set new values for queue.basketNonce as BasketNonce2 and queue.tokens as [USDT, WETH] and would return and attacker contract hook would return too.
8. execution would return to refundSpan() and code would try to transfer the second tokens and read it from queue.tokens[1] from storage and because storage value for queue.tokens[] has been changes code would try to transfer 100 WETH tokens to attacker address from other users deposited WETH.
9. so attacker payed 100 SOME_ERC77 and USDT but received 100 SOME_ERC777 and WETH and stole the other users funds.

**scenario #2: attacker can use sweepRewards() function for reentrancy: (attacker can perform all of this in one transaction)

1. tokens [SOME_ERC777, USDT] with quantity [1, 1] are in the basket right now and basket nonce is BasketNonce1.
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 100K RToken and code would set attacker vesting queue values. queue.basketNonce would be BasketNonce1 and queue.tokens would be [SOME_ERC777, USDT] and queue.left = 0 and queue.right = 1. also code would transfer 100K USDT and 100K SOME_ERC777 tokens from attacker address to RToken address and set the items of the queue accordingly and increase liabilities[SOME_ERC777] and liabilities[USDT] by 100K.
4. when contract has more than 200K USDT balance which belong to users deposits, attacker would call cancel(attackerAddress, 1) to refund his deposits. code would call refundSpan(attackerAddress, 0, 1) to cancel and refund attacker deposits.
5. function refundSpan() would decrease the values of liabilities[SOME_ERC777] and liabilities[USDT] by 100K and then would try to transfer tokens and start with SOME_ERC777 token and during the transfer ERC777 token would call receiver hook function and attacker contract would get called.
6. attacker contract would call sweepRewardsSingle(USDT) and the code would call RewardableLibP1.sweepRewardsSingle(liabilities, USDT, assetRegistry, backingManager) which would transfer amt = USDT.balanceOf(address(this)) - liabilities[USDT] amount to BackingManger as rewards. but as liabilities[USDT] value is decreased by 100K but contract still hold 100K so code would calculated wrong amount for rewards would transfer 100K of users depsit funds to BackingManager as rewards which would get distributed among RSR stakers and RToken holders.
7. then attacker contract would return and refundSpan() would transfer 100K USDT to attacker address.
8. so by doing this attacker caused contract to send users deposits which are in the vesting to mint RToken to be transferred to BackingManager as rewards. this would cause some of the users to not be able to receive RToken even so they deposited their funds and the fund would be distributed between RToken holder and RSR stakers and deposit users would lose their funds.

Tools Used

VIM

Recommended Mitigation Steps

one thing can be checking basketNonce of the queue in refundSpan() end line to not be changed. add reentrancy guard for important function like cancel() or issue().

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)

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L439-L514 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BackingManager.sol#L105-L150

Vulnerability details

Impact

Function redeem() redeems RToken for basket collateral and it updated basketsNeeded and transfers users basket ERC20 from BackingManager to user address. it loops through tokens and transfer them to caller and if one of tokens were ERC777 or any other 3rd party protocol token with hook, attacker can perform reentrancy attack during token transfers. Attacker can cause multiple impacts by choosing the reentrancy function:

1. attacker can call redeem() again and bypass "bounding each withdrawal by the prorata share when protocol is under-collateralized" because tokens balance of BackingManager is not updated yet.
2. attacker can call BackingManager.manageTokens() and because basketsNeeded gets decreased and basket tokens balances of BasketManager are not updated, code would detect those tokens as excess funds and would distribute them between RSR stakers and RToken holders and some of RToken deposits would get transferred to RSR holders as rewards.

Proof of Concept

This is redeem() code:

    function redeem(uint256 amount) external notFrozen {
...............
...............
        (address[] memory erc20s, uint256[] memory amounts) = basketHandler.quote(baskets, FLOOR);

        uint256 erc20length = erc20s.length;

        uint192 prorate = uint192((FIX_ONE_256 * amount) / supply);

        // Bound each withdrawal by the prorata share, in case we're currently under-collateralized
        for (uint256 i = 0; i < erc20length; ++i) {
            uint256 bal = IERC20Upgradeable(erc20s[i]).balanceOf(address(backingManager));

            uint256 prorata = (prorate > 0)
                ? (prorate * bal) / FIX_ONE // {qTok} = D18{1} * {qTok} / D18
                : mulDiv256(bal, amount, supply); // {qTok} = {qTok} * {qRTok} / {qRTok}

            if (prorata < amounts[i]) amounts[i] = prorata;
        }

        basketsNeeded = basketsNeeded_ - baskets;
        emit BasketsNeededChanged(basketsNeeded_, basketsNeeded);

        // == Interactions ==
        _burn(redeemer, amount);

        bool allZero = true;
        for (uint256 i = 0; i < erc20length; ++i) {
            if (amounts[i] == 0) continue;
            if (allZero) allZero = false;

            IERC20Upgradeable(erc20s[i]).safeTransferFrom(
                address(backingManager),
                redeemer,
                amounts[i]
            );
        }

        if (allZero) revert("Empty redemption");
    }

As you can see code calculates withdrawal amount of each basket erc20 tokens by calling basketHandler.quote() and then bounds each withdrawal by the prorata share of token balance, in case protocol is under-collateralized. and then code updates basketsNeeded and in the end transfers the tokens. if one of those tokens were ERC777 then that token would call receiver hook function in token transfer. there may be other 3rd party protocol tokens that calls registered hook functions 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 call hook functions. attacker can use this hook and perform reentrancy attack. This is fullyCollateralized() code in BasketHandler:

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

As you can see it calculates baskets that can be held by backingManager tokens balance and needed baskets by RToken contract and by comparing them determines that if RToken is fully collateralized or not. if RToken is fully collateralized then BackingManager.manageTokens() would call handoutExcessAssets() and would distributes extra funds between RToken holders and RSR stakers. the root cause of the issue is that during tokens transfers in redeem() not all the basket tokens balance of the BackingManager updates once and if one has hook function which calls attacker contract then attacker can use this updated token balance of the contract and perform his reentrancy attack. attacker can call different functions for reentrancy. these are two scenarios: ** scenario #1: attacker call redeem() again and bypass prorata share bound check when protocol is under-collaterialized:

1. tokens [SOME_ERC777, USDT] with quantity [1, 1] are in the basket right now and basket nonce is BasketNonce1.
2. BackingManager has 200K SOME_ERC777 balance and 100K USDT balance. basketsNeeded in RToken is 150K and RToken supply is 150K and attacker address Attacker1 has 30k RToken. battery charge allows for attacker to withdraw 30K tokens in one block.
3. attacker would register a hook for his address in SOME_ERC777 token to get called during transfers.
4. attacker would call redeem() to redeem 15K RToken and code would updated basketsNeeded to 135K and code would bounds withdrawal by prorata shares of balance of the BackingManager because protocol is under-collateralized and code would calculated withdrawal amouns as 15K SOME_ERC777 tokens and 10K USDT tokens (instead of 15K USDT tokens) for withdraws.
5. then contract would transfer 15K SOME_ERC777 tokens first to attacker address and attacker contract would get called during the hook function and now basketsNeeded is 135K and total RTokens is 135K and BackingManager balance is 185K SOME_ERC777 and 100K USDT (USDT is not yet transferred). then attacker contract can call redeem() again for the remaining 15K RTokens.
6. because protocol is under-collateralized code would calculated withdrawal amouns as 15K SOME_ERC777 and 11.1K USDT (USDT_balance * rtokenAmount / totalSupply = 100K * 15K / 135K) and it would burn 15K RToken form caller and the new value of totalSupply of RTokens would be 120K and basketsNeeded would be 120K too. then code would transfers 15K SOME_ERC777 and 11.1K USDT for attacker address.
7. attacker's hook function would return and redeem() would transfer 10K USDT to attacker in the rest of the execution. attacker would receive 30K SOME_ERC777 and 21.1K USDT tokens for 15K redeemed RToken but attacker should have get (100 * 30K / 150K = 20K) 20K USDT tokens because of the bound each withdrawal by the prorata share, in case we're currently under-collateralized.
8. so attacker would be able to bypass the bounding check and withdraw more funds and stole other users funds. the attack is more effective if withdrawal battery charge is higher but in general case attacker can perform two withdraw each with about charge/2 amount of RToken in each block and stole other users funds when protocol is under collaterlized.

** scenario #2: attacker can call BackingManager.manageTokens() for reentrancy call:

1. tokens [SOME_ERC777, USDT] with quantity [1, 1] are in the basket right now and basket nonce is BasketNonce1.
2. BackingManager has 200K SOME_ERC777 balance and 150K USDT balance. basketsNeeded in RToken is 150K and RToken supply is 150K and attacker address Attacker1 has 30k RToken. battery charge allows for attacker to withdraw 30K tokens in one block.
3. attacker would register a hook for his address in SOME_ERC777 token to get called during transfers.
4. attacker would call redeem() to redeem 30K RToken and code would updated basketsNeeded to 120K and burn 30K RToken and code would calculated withdrawal amounts as 30K SOME_ERC777 tokens and 30K USDT tokens for withdraws.
5. then contract would transfer 30K SOME_ERC777 tokens first to attacker address and attacker contract would get called during the hook function and now basketsNeeded is 120K and total RTokens is 120K and BackingManager balance is 170K SOME_ERC777 and 150K USDT (USDT is not yet transferred). then attacker contract can call BackingManager.manageTokens().
6. function manageTokens() would calculated baskets can held by BackingManager and it would be higher than 150K and basketsNeeded would be 130K and code would consider 60K SOME_ERC777 and 30K USDT tokens as revenue and try to distribute it between RSR stakers and RToken holders. code would mint 30K RTokens and would distribute it.
7. then attacker hook function would return and redeem() would transfer 30K USDT to attacker address in rest of the execution.
8. so attacker would able to make code to calculate RToken holders backed tokens as revenue and distribute it between RSR stakers and RSR stakers would receive RTokens backed tokens as rewards. the attack is more effective is battery charge is high but in general case attacker can call redeem for battery charge amount and cause those funds to be counted and get distributed to the RSR stakers (according to the rewards distribution rate)

Tools Used

VIM

Recommended Mitigation Steps

prevent reading reentrancy attack by central reentrancy guard or by one main 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)

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L406-L418 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L655-L714 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L342-L370

Vulnerability details

Impact

When users issue new RTokens, code creates vesting deposits and calculates finish time and increase the amount of the allVestAt but if users cancel their issuance in vesting time code don't decrease the value of the allVestAt. this would cause natural DOS by design as users interact with protocol and issue and cancel. also attacker can get a small loan and repeat 1000 times of issue() and cancel() for those tokens and increase allVestAt as much high that no one be able to create RToken. the default is issuance rate is 6% in each hour and each month has 720 hour so if attacker get a flash loan for basket tokens worth of 6% RToken backed tokens and 720 times repeats issue() and cancel() then allVestAt would increase 1 month and no one be able to create RToken in that Month and attacker needs to do this each month one time to completely block protocol. this is a critical issue because it is happing normally by design as users interact with system and also attacker can intentionally block the new RToken supply and break the protocol and cause losses for RSR stakers. There is a griefing attack too as other users call issue() and see that they have to wait long time for minting new RTokens they need to call cancel() and pay two transaction gas. there is no parameter that user can specify maximum time they allow for vesting time when they call issue() and code can create vesting issue with any delay for user and user has no control over the maximum allowed time and user can't calculate the time before transaction too because other transactions can change the state of the contract before user transaction mining.

Proof of Concept

This is cancel() and refundSpan() code:

    function cancel(uint256 endId, bool earliest) external notFrozen {
        address account = _msgSender();
        IssueQueue storage queue = issueQueues[account];

        require(queue.left <= endId && endId <= queue.right, "out of range");

        // == Interactions ==
        if (earliest) {
            refundSpan(account, queue.left, endId);
        } else {
            refundSpan(account, endId, queue.right);
        }
    }

    function refundSpan(
        address account,
        uint256 left,
        uint256 right
    ) private {
        if (left >= right) return; // refund an empty span

        IssueQueue storage queue = issueQueues[account];

        // compute total deposits to refund
        uint256 tokensLen = queue.tokens.length;
        uint256[] memory amt = new uint256[](tokensLen);
        uint256 amtRToken; // {qRTok}
        IssueItem storage rightItem = queue.items[right - 1];

        // compute item(right-1) - item(left-1)
        // we could dedup this logic for the zero item, but it would take more SLOADS
        if (left == 0) {
            amtRToken = rightItem.amtRToken;
            for (uint256 i = 0; i < tokensLen; ++i) {
                amt[i] = rightItem.deposits[i];

                // Decrement liabilities
                liabilities[IERC20(queue.tokens[i])] -= amt[i];
            }
        } else {
            IssueItem storage leftItem = queue.items[left - 1];
            amtRToken = rightItem.amtRToken - leftItem.amtRToken;
            for (uint256 i = 0; i < tokensLen; ++i) {
                amt[i] = rightItem.deposits[i] - leftItem.deposits[i];

                // Decrement liabilities
                liabilities[IERC20(queue.tokens[i])] -= amt[i];
            }
        }

        if (queue.left == left && right == queue.right) {
            // empty entire queue
            queue.left = 0;
            queue.right = 0;
        } else if (queue.left == left && right < queue.right) {
            queue.left = right; // remove span from beginning
        } else if (queue.left < left && right == queue.right) {
            queue.right = left; // refund span from end
        } else {
            // untestable:
            //      All calls to refundSpan() use valid values for left and right.
            //      queue.left <= left && right <= queue.right.
            //      Any call to refundSpan() passes queue.left for left,
            //      OR passes queue.right for right, OR both.
            revert("Bad refundSpan");
        } // error: can't remove [left,right) from the queue, and leave just one interval

        emit IssuancesCanceled(account, left, right, amtRToken);

        // == Interactions ==
        for (uint256 i = 0; i < queue.tokens.length; ++i) {
            IERC20Upgradeable(queue.tokens[i]).safeTransfer(account, amt[i]);
        }
    }

As you can see there is no logic to decrease the value of the allVestAt and as users calls issue() the function whenFinished() would get called and increase the allVestAt but as they call cancel() the value of the allVestAt won't get decreased so as users interact with system contract would delay minting new tokens and users would cancel their deposits and it would cause more delay too. attacker can perform this attack intentionally to perform a DOS attack and totally block minting new RTokens. attacker can perform this steps:

1. attacker contract would get a flash loan worth of 10% of the RToken backed tokens in each basket tokens.
2. for 700 times attacker contract would perform this: 2.1 call the issue(10% of the RToken supply) and tokens would get transferred to RToken and code would increase allVestAt by about 2 hour time and crease a queue for attacker. 2.2 call the cancel() and cancel vesting deposits and code would transfer attacker deposits to contract address but it won't decrease allVestAt
3. in the end allVestAt would be increase as 1 month. and any user wants to crease RToken would have wait 1 month. attacker can repeat the attack and increase the allVestAt as much as he wants and deny others from minting new RTokens.
4. if the issuance per block would be smaller then the attack can be more effective but with default values attack is effective too. because issue() has no argument that user can set for maxVestingTimeAllowed time allowed so if users calls issue() and sees they have to wait more than one month to get RToken they would cancel their issuance and it would increase allVestAt more. and the depositing user most call cancel() again and would pay two transaction gas. so there is a possible greifing attack too which increase the impact of the DOS.
5. by performing this attacker can break contract core feature and RSR stakers won't receive more rewards as RTokens won't get minted and attacker can perform this attack in all deployed RTokens.

Tools Used

VIM

Recommended Mitigation Steps

decrease allVestAt when users cancels their deposits.

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BackingManager.sol#L67-L77 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L439-L443

Vulnerability details

Impact

Function redeem() in RToken contract, redeems RToken for basket collateral and it's callable when protocol is paused. but function redeem() transfers basket tokens from BackingManager contract to redeemer and it requires spending allowance of the BackingManger tokens which is granted by calling grantRTokenAllowance() for each basket tokens. function grantRTokenAllowance() is not callable when protocol is paused. so if for any one of the basket tokens function grantRTokenAllowance() didn't get called (newly added tokens to basket) then when protocol is paused grantRTokenAllowance() can't be called and redeem() won't be possible.

Proof of Concept

This is grantRTokenAllowance() code:

    function grantRTokenAllowance(IERC20 erc20) external notPausedOrFrozen {
        require(assetRegistry.isRegistered(erc20), "erc20 unregistered");
        // == Interaction ==
        IERC20Upgradeable(address(erc20)).safeApprove(address(main.rToken()), 0);
        IERC20Upgradeable(address(erc20)).safeApprove(address(main.rToken()), type(uint256).max);
    }

As you can see it gives RToken spending allowance and it can't be called when protocol is Paused. This is redeem() code:

function redeem(uint256 amount) external notFrozen {
        require(amount > 0, "Cannot redeem zero");
.........
.........
.........
        // Accept and burn RToken, reverts if not enough balance to burn
        _burn(redeemer, amount);

        bool allZero = true;
        for (uint256 i = 0; i < erc20length; ++i) {
            if (amounts[i] == 0) continue;
            if (allZero) allZero = false;

            // Send withdrawal
            IERC20Upgradeable(erc20s[i]).safeTransferFrom(
                address(backingManager),
                redeemer,
                amounts[i]
            );
        }

        if (allZero) revert("Empty redemption");
    }

As you can see it can be called when protocol is paused and it transfers tokens from BackingManager to redeemer and require that RToken has spending allowance of BackingManager tokens so function grantRTokenAllowance() most be called for basket tokens before. the problem is that if there were newly added token in the basket and grantRTokenAllowance() isn't called for that token and protocol was paused then user won't be able to redeem() because grantRTokenAllowance() is not callable during Paused time and redeem() won't be possible in Paused time. so as docs says that redeem() should be possible when protocol is paused but it won't happen in all cases and redeem() would be blocked.

Tools Used

VIM

Recommended Mitigation Steps

allow grantRTokenAllowance() to be called when contract is paused or call it when basket changes from contract.