Mising caller protection when issuing Rtoken

[code423n4]

Lines of code

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

Vulnerability details

Impact

Similar protection like it has been implemented in redemption is missing in issue() and issueTo(). This could lead to caller sending in significantly higher quantities of ERC20 than intended that could be preceded by a huge ratio increase in Furnace.sol to ease out a prolonged accumulation of lastPayoutBal.

Proof of Concept

Supposing full governance has been fully DAO enforced, here is the scenario:

1. A proposal successfully executes to set the melt ratio to a very low if not near to zero value (FIX_ZERO) on line 87 such that the Rtoken sent to this contract is increasingly stuck and accumulated as lastPayoutBal on line 78:

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

    function melt() external notFrozen {
        if (uint48(block.timestamp) < uint64(lastPayout) + PERIOD) return;

        // # of whole periods that have passed since lastPayout
        uint48 numPeriods = uint48((block.timestamp) - lastPayout) / PERIOD;

        // Paying out the ratio r, N times, equals paying out the ratio (1 - (1-r)^N) 1 time.
        uint192 payoutRatio = FIX_ONE.minus(FIX_ONE.minus(ratio).powu(numPeriods));

        uint256 amount = payoutRatio.mulu_toUint(lastPayoutBal);

        lastPayout += numPeriods * PERIOD;
78:        lastPayoutBal = rToken.balanceOf(address(this)) - amount;
        if (amount > 0) rToken.melt(amount);
    }

    /// Ratio setting
    /// @custom:governance
    function setRatio(uint192 ratio_) public governance {
        // solhint-disable-next-line no-empty-blocks
        if (lastPayout > 0) try this.melt() {} catch {}
87:        require(ratio_ <= MAX_RATIO, "invalid ratio");
        // The ratio can safely be set to 0 to turn off payouts, though it is not recommended
        emit RatioSet(ratio, ratio_);
        ratio = ratio_;
    }

2. After quite some time, another proposal successfully excecutes to set the melt ratio to a significantly higher value nearing to FIX_ONE so that the future melt() will typically have all Rtoken received from the distributor melted and burned without decreasing the basket needed.

3. Users not paying attention to the timelock annoucement will all be caught off guard when attempting to issue RToken by paying a whole basket of collaterals at much higher rates (as indicated on line 132 due to a smaller denominator, supply, and correspondingly relates to deposits and deposits[i] on lines 136 and 148) with premium commensurate with the amount of RToken issued:

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RToken.sol#L131-L150

        uint192 amtBaskets = supply > 0
132:            ? basketsNeeded.muluDivu(amount, supply, CEIL)
            : _safeWrap(amount);
        emit Issuance(issuer, recipient, amount, amtBaskets);

136:        (address[] memory erc20s, uint256[] memory deposits) = basketHandler.quote(
            amtBaskets,
            CEIL
        );

        // == Interactions: Create RToken + transfer tokens to BackingManager ==
        _scaleUp(recipient, amtBaskets, supply);

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

Tools Used

Manual

Recommended Mitigation Steps

Consider implementing maxAmounts logic in the issue functions pertaining to the sending of collaterals to the Backing Manager.

Introduce a sliding scale mechanism that only permits small incremental changes of melt ratio over time. This would make it more difficult for an attacker with high governance power to manipulate the ratio while also providing users with more predictability.

Consider also impleting a lower bound require check to prevent the setting of zero or near to zero ratio that circumvents the intended purpose of raising the exchange rate of RToken to Collateral.

Assessed type

Governance

[0xean]

Hard to see this risk as M considering a time lock is already in place. The required set up for this which requires the melt ratio to be modified to extremes makes me think that QA is the appropriate severity for this.

[tbrent]

We think should be downgraded to QA but is deserving of documentation that should make clear precise approvals should be used to protect against loss in this circumstance. maxAmounts would work but is more gas intensive.

[tbrent]

The caller can also redeem directly after for the same token amounts they sent in.

[c4-sponsor]

tbrent marked the issue as sponsor acknowledged

[c4-sponsor]

tbrent marked the issue as disagree with severity

[c4-judge]

0xean changed the severity to QA (Quality Assurance)