Reserve Protocol - Invitational - rvierdiiev's results

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/Distributor.sol#L61-L65

Vulnerability details

Impact

In case Distributor.setDistribution use, revenue from rToken RevenueTrader and rsr token RevenueTrader should be distributed. Otherwise wrong distribution will be used.

Proof of Concept

BackingManager.forwardRevenue function sends revenue amount to the rsrTrader and rTokenTrader contracts, according to the distribution inside Distributor contract. For example it can 50%/50%. In case if we have 2 destinations in Distributor: strsr and furnace, that means that half of revenue will be received by strsr stakers as rewards.

This distribution can be changed at any time. The job of RevenueTrader is to sell provided token for a tokenToBuy and then distribute it using Distributor.distribute function. There are 2 ways of auction that are used: dutch and gnosis. Dutch auction will call RevenueTrader.settleTrade, which will initiate distribution. But Gnosis trade will not do that and user should call distributeTokenToBuy manually, after auction is settled.

The problem that i want to discuss is next. Suppose, that governance at the beginning set distribution as 50/50 between 2 destinations: strsr and furnace. And then later forwardRevenue sent some tokens to the rsrTrader and rTokenTrader. Then, when trade was active to exchange some token to rsr token, Distributor.setDistribution was set in order to make strsr share to 0, so now everything goes to Furnace only. As result, when trade will be finished in the rsrTrader and Distributor.distribute will be called, then those tokens will not be sent to the strsr contract, because their share is 0 now. They will be stucked inside rsrTrader.

Another problem here is that strsr holders should receive all revenue from the time, where they distribution were created. What i mean is if in time 0, rsr share was 50% and in time 10 rsr share is 10%, then BackingManager.forwardRevenue should be called for all tokens that has surplus, because if that will be done after changing to 10%, then strsr stakers will receive less revenue.

Tools Used

VsCode

Recommended Mitigation Steps

You need to think how to guarantee fair distribution to the strsr stakers, when distribution params are changed.

Assessed type

Error

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/Furnace.sol#L84-L91

Vulnerability details

Impact

FurnaceP1.setRatio will not update lastPayout when called in frozen state, which means that after component will be unfrozen, melting will be incorrect.

Proof of Concept

melt function should burn some amount of tokens from lastPayoutBal. It depends of lastPayout and ratio variables. The more time has passed, the more tokens will be burnt.

When setRatio function is called, then melt function is tried to be executed, because new ratio is provided and it should not be used for previous time ranges. In case if everything is ok, then lastPayout and lastPayoutBal will be updated, so it's safe to update ratio now. But it's possible that melt function will revert in case if notFrozen modifier is not passed. As result lastPayout and lastPayoutBal will not be updated, but ratio will be. Because of that, when Furnace will be unfrozen, then melting rate can be much more, then it should be, because lastPayout wasn't updated.

Tools Used

VsCode

Recommended Mitigation Steps

In case of catch case, you can update lastPayout and lastPayoutBal.

Assessed type

Error

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L222-L232

Vulnerability details

Impact

Loss of staking yield for stakers when another user stakes in pause/frozen state.

Proof of Concept

Issue #148 from previous audit is present again. As i can see it was mitigated. But maybe after that new code changes were made, so this issue is present again.

Tools Used

VsCode

Recommended Mitigation Steps

In case if you can't call payoutRewards when frozen, then do not allow to call stake as well.

Assessed type

Error

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L341-L380

Vulnerability details

Impact

StRSR.cancelUnstake doesn't call _payoutRewards before minting new shares. As result this rewards will be distributed for new staker as well.

Proof of Concept

If user wants to cancel his withdraw, then he can call StRSR.cancelUnstake and mint new shares from the RSR amount he wanted to withdraw. This minting will use current stakeRate to find out stRSR amount to mint.

The problem is that _payoutRewards function is not called before minting. It should be because, this function increases stakeRSR param with rewards, which then changes stakeRate.

As result user that decided to cancel withdraw will receive part of rewards which he should not receive.

Tools Used

VsCode

Recommended Mitigation Steps

Call _payoutRewards, before minting new stRSR to user.

Assessed type

Error

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/AssetRegistry.sol#L86-L115

Vulnerability details

Impact

Attacker can disable basket, when swapping or unregistering not basket asset.

Proof of Concept

AssetRegistry.swapRegistered and AssetRegistry.unregister are both functions that can be called by governance only. It's possible that after proposal is passed, then it can be executed by anyone. Also executor can provide as much gas with the call, as he wishes.

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/AssetRegistry.sol#L86-L96

    function swapRegistered(IAsset asset) external governance returns (bool swapped) {
        require(_erc20s.contains(address(asset.erc20())), "no ERC20 collision");


        try basketHandler.quantity{ gas: _reserveGas() }(asset.erc20()) returns (uint192 quantity) {
            if (quantity > 0) basketHandler.disableBasket(); // not an interaction
        } catch {
            basketHandler.disableBasket();
        }


        swapped = _registerIgnoringCollisions(asset);
    }

Here, reserve team is checking that after basketHandler.quantity call, function still has 900000 gas, which is needed to finish basketHandler.disableBasket call. As you can see, in case if quantity of asset in the basket is 0, then there is no need to disable basket.

However, malicious user still can do that. What he need to do is to call swapRegistered with 900000 + x gas amount. Where x will be the amount that is not enough to execute basketHandler.quantity function, which will cause out of gas error and catch block will then call basketHandler.disableBasket. As result basket will be disabled if attacker wants it.

Tools Used

VsCode

Recommended Mitigation Steps

You need to ensure that you send enough gas to basketHandler.quantity function.

Assessed type

Error

#1. BasketHandler.warmupPeriod can be changed, when basket is in warm up period

Impact

BasketHandler.warmupPeriod can be changed, when basket is in warm up period, which will allow another contracts to call basket handler.

Proof of Concept

BasketHandler.isReady function is used by another components to check if it's possible to communicate with basket handler. https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BasketHandler.sol#L261-L265

    function isReady() external view returns (bool) {
        return
            status() == CollateralStatus.SOUND &&
            (block.timestamp >= lastStatusTimestamp + warmupPeriod);
    }

As you can see warmupPeriod period is needed to wait after basket status changed. The problem that owner can change warmupPeriod period any time. And in case if warmup has already started before it will rewrite it.

Recommended Mitigation Steps

You need to store time, where warmup will be finished instead of using warmupPeriod check.

#2. RecollateralizationLibP1.nextTradePair should prioritize selling IFFY collateral over SOUND.

Impact

BackingManager currently trying to sell SOUND collateral instead of IFFY. Because of that RToken leave token that can be defaulted soon.

Proof of Concept

RecollateralizationLibP1.nextTradePair function should find trade pair in order to make recollateralization. It uses isBetterSurplus function in order to compare selling different assets. https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/mixins/RecollateralizationLib.sol#L418-L434

    function isBetterSurplus(
        MaxSurplusDeficit memory curr,
        CollateralStatus other,
        uint192 surplusAmt
    ) private pure returns (bool) {
        // NOTE: If the CollateralStatus enum changes then this has to change!
        if (curr.surplusStatus == CollateralStatus.DISABLED) {
            return other == CollateralStatus.DISABLED && surplusAmt.gt(curr.surplus);
        } else if (curr.surplusStatus == CollateralStatus.SOUND) {
            return
                other == CollateralStatus.DISABLED ||
                (other == CollateralStatus.SOUND && surplusAmt.gt(curr.surplus));
        } else {
            // curr is IFFY
            return other != CollateralStatus.IFFY || surplusAmt.gt(curr.surplus);
        }
    }

As you can see in case if current status is SOUND, then any amount of DISABLED will be counted as better option to sell. But in case if other is IFFY, then curr will not be updated.

The idea here is to better sell DISABLED token over SOUND. But IFFY token can become DEFAULTED as well soon. That's why i believe that it should be prioritized to sell over SOUND asset. So i guess that SOUND tokens should be sold in last order.

Recommended Mitigation Steps

In case if current is SOUND and other is IFFY, then sell IFFY.

#3. StRSR.pushDraft will make new draft be withdrawable later, then it should be in case if current unstakingDelay is smaller then previous one.

Impact

User should be able to withdraw his draft after unstakingDelay period, but he should wait more.

Proof of Concept

When user creates new draft, then availableAt time is calculated for him. This is a time when he should be able to withdraw his draft.

However, because of this check it's possible that user will wait more time to withdraw that draft.

This can happen when unstakingDelay governance param was changed to smaller one.

While, i understand that should be done, because of algorithm that is used to work with drafts, this submission is about unfair waiting time.

Recommended Mitigation Steps

Think, that it can't be changed, as it will break work with drafts.