Reserve contest - __141345__'s results

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L828-L832 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L496-L530

Vulnerability details

Impact

payoutRatio assume constant rewardRatio across all the reward periods, and derives the formula of $1 - (1-payoutRatio)^N$. However, if the setter function setRewardRatio() is called to change the rewardRatio, the constant rewardRatio assumption does not hold anymore, the modelling process of constant ratio need adjustment. As a result, the reward amount calculation is not accurate comparing with the expected amount. In some cases the users could receive more rewards than deserved, and sometimes the users could get less rewards.

The issue with the current formula is, when the rewardRatio changed, it is immediately applied to all the reward periods, including the periods in the past. Effectively applying future parameter value to past reward periods. So the reward calculation logic needs refactor.

Proof of Concept

According to the payout formula comments:

// [strsr-payout-formula]: // The process we're modelling is: // N = number of whole rewardPeriods since last >payoutRewards() call // rewards_0 = rsrRewards() // payout{i+1} = rewards_i * payoutRatio // rewards_{i+1} = rewards_i - payout_{i+1} // payout = sum{payout_i for i in [1...N]} // thus: // rewards_N = rewards_0 - payout // rewards_{i+1} = rewards_i - rewards_i * payoutRatio = rewards_i * (1-payoutRatio) // rewards_N = rewards_0 * (1-payoutRatio) ^ N // payout = rewards_N - rewards_0 = rewards_0 * (1 - (1-payoutRatio)^N)

Here rewardRatio is payoutRatio in the formula. Effectively is paying out the rewardRatio for N times.

The assumption is, the reward comes at period 0 in a lump sum. For each period, the payout is the rewardRatio of the reward amount available for this period. And the reward amount available need to subtract the payout for current period. Whatever left in the reward amount will be used for the next period. And then the $1 - (1-payoutRatio)^N$ is derived.

The implementation is:

File: contracts/p1/StRSR.sol
509:             uint192 payoutRatio = FIX_ONE - FixLib.powu(FIX_ONE - rewardRatio, numPeriods);

512:             payout = (payoutRatio * rsrRewardsAtLastPayout) / FIX_ONE;

According to the formula, the rewardRatio needs to stay constant for this formula to hold.

For example, assuming the numPeriods is 2, rsrRewards() is 10,000. And let the original rewardRatio being 0.2, and new rewardRatio being 0.3. If the setRewardRatio() call happens at the 2nd period. According to the payout formula, with the updated rewardRatio, the final payout ratio is $1 - (1 - 0.3)^2 = 0.51$, payout amount is 5,100.

But if looking at each reward period and going through the modelling process, reward_0 is 10,000, payout_0 is 2,000 (using 0.2 as rewardRatio), reward_1 is 8,000, payout_1 is 2,400 (using 0.3 as rewardRatio), the total payout amount should be 4,400, instead of 5,100.

From the logic of the reward process, before the setRewardRatio(), old rewardRatio should be applied, and the new value only take effects after the setter function call. Here, the period before the setter function is ignored, and equivalent to using future parameter value to the past periods.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Call _payoutRewards() in function setRewardRatio(), before new rewardRatio takes effect:

    function setRewardRatio(uint192 val) public governance {
        require(val <= MAX_REWARD_RATIO, "invalid rewardRatio");
        emit RewardRatioSet(rewardRatio, val);
+       _payoutRewards();
        rewardRatio = val;
    }

In this way, when the setter function is called, past period is settled with the old ratio value. And the current value change will be used for future periods.

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/Distributor.sol#L87-L137 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L496-L530

Vulnerability details

Impact

Currently the payout formula assumes lump sum reward at the beginning, and derive the formula by repeating the payout N times. But in practice, the rewards do not arrive in lump sum, but comes as multiple individual amounts. As a result this formula will give different results if the _payoutRewards() function is called in different ways. Hence, the reward amount calculation is not accurate and inconsistent.

Proof of Concept

According to the payout formula comments:

// [strsr-payout-formula]: // The process we're modelling is: // N = number of whole rewardPeriods since last >payoutRewards() call // rewards_0 = rsrRewards() // payout{i+1} = rewards_i * payoutRatio // rewards_{i+1} = rewards_i - payout_{i+1} // payout = sum{payout_i for i in [1...N]} // thus: // rewards_N = rewards_0 - payout // rewards_{i+1} = rewards_i - rewards_i * payoutRatio = rewards_i * (1-payoutRatio) // rewards_N = rewards_0 * (1-payoutRatio) ^ N // payout = rewards_N - rewards_0 = rewards_0 * (1 - (1-payoutRatio)^N)

Here rewardRatio is payoutRatio in the formula. Effectively is paying out the rewardRatio for N times.

The assumption is, the reward comes at period 0 in a lump sum. For each period, the payout is the rewardRatio of the reward amount available for this period. And the reward amount available need to subtract the payout for current period. Whatever left in the reward amount will be used for the next period. And then the $1 - (1-payoutRatio)^N$ is derived.

The implementation is:

File: contracts/p1/StRSR.sol
509:             uint192 payoutRatio = FIX_ONE - FixLib.powu(FIX_ONE - rewardRatio, numPeriods);

512:             payout = (payoutRatio * rsrRewardsAtLastPayout) / FIX_ONE;

For example, assuming the numPeriods is 2, rewards of 1,000 will arrive for each period, rewardRatio being 0.2. So rsrRewards() is 1,000 at the beginning. If call function _payoutRewards() at the end of these 2 periods, the final payout ratio will be $1 - (1 - 0.2)^2 = 0.36$, total payout would be 2,000 * 0.36 = 720.

However, if _payoutRewards() is called each time the reward arrives, the payout_1 would be 1,000 * 0.2 = 200, and 1,000 - 200 = 800 will remain in the reward available. Next time rsrRewards() will be 1,800, and payout_2 would be 1,800 * 0.2 = 360. Total payout is 200 + 360 = 560.

The difference of 720 and 560 come from the assumption that all rewards arrive at the beginning of all the periods. So the late arrived rewards will be paid out more, in the example above, the 2nd 1,000 reward only went through 1 reward period (200 payout for 1 st period), but the formula treats it as paid through the whole 2 periods (200 + 160 for 2nd period).

The consequence is, in practice, users could received different amounts of rewards if the _payoutRewards() function are called at different periods. The reward system can not give consistent outcome.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Each time new reward is distributed in Distributor.sol:distribute(), add call to StRSR._payoutRewards() to payout the reward. So previous available rewards will be cleared, and the later payout will give consistent results.

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BackingManager.sol#L238-L242

Vulnerability details

Impact

When handoutExcessAssets(), if one of the token transfer fails, the whole excess assets handout would revert, causing DoS. _manageTokens() -> handoutExcessAssets() function will be inoperable until the token transfer resumes.

However, some external factors could lead to transfer failure:

• some token has pause mode, such as USDC, USDT, WBTC, etc. When this mode is turned on, all transfer will revert.
• some token has blacklist, such as USDC, if the rsrTrader is added to the blacklist, the transfer will fail.

Proof of Concept

The entire list of erc20s is iterated to transfer excess token.

File: contracts/p1/BackingManager.sol
154:     function handoutExcessAssets(IERC20[] calldata erc20s) private {

238:         for (uint256 i = 0; i < length; ++i) {
239:             IERC20Upgradeable erc20 = IERC20Upgradeable(address(erc20s[i]));
240:             if (toRToken[i] > 0) erc20.safeTransfer(address(rTokenTrader), toRToken[i]);
241:             if (toRSR[i] > 0) erc20.safeTransfer(address(rsrTrader), toRSR[i]);
242:         }

But the external dependency on token transfer success could DoS and making the _manageTokens() -> handoutExcessAssets() functionality fail to operate.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Use try/catch to skip the erc20 with problem, then the manageToken() function will be more robust to external token special conditions.

Lines of code

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

Vulnerability details

Impact

There is no requirement of non pause/frozen mode to stake(). The difference for these modes is the rewards payout. During pause/frozen mode, rewards will not be paid out before the stake is recorded. As a result, new stakes in pause/frozen mode will dilute the current stakers shares in StRSR, and lose rewards in the future.

Proof of Concept

Even in pause/frozen mode, anyone can still call stake(). But in pause/frozen mode, accumulated rewards will not be paid out.

File: contracts/p1/StRSR.sol
212:     function stake(uint256 rsrAmount) external {
213:         require(rsrAmount > 0, "Cannot stake zero");
214: 
215:         if (!main.pausedOrFrozen()) _payoutRewards();

If _payoutRewards() is called, stakeRSR and stakeRate will be updated. stakeRSR will increase while stakeRate will decrease.

496:     function _payoutRewards() internal {

513:             stakeRSR += payout;

524:         stakeRate = (stakeRSR == 0 || totalStakes == 0)
525:             ? FIX_ONE
526:             : uint192((totalStakes * FIX_ONE_256 + (stakeRSR - 1)) / stakeRSR);

Then in stake(), stakeRSR and stakeRate will be used to calculate the stakeAmount to mint.

212:     function stake(uint256 rsrAmount) external {

222:         uint256 newStakeRSR = stakeRSR + rsrAmount;
223:         // newTotalStakes: {qStRSR} = D18{qStRSR/qRSR} * {qRSR} / D18
224:         uint256 newTotalStakes = (stakeRate * newStakeRSR) / FIX_ONE;
225:         uint256 stakeAmount = newTotalStakes - totalStakes;
226: 
227:         // Update staked
228:         address account = _msgSender();
229:         stakeRSR += rsrAmount;
230:         _mint(account, stakeAmount);

If in pause/frozen mode, new rsr is staked, existing stakers StRSR will be diluted due to the missing rewards.

Assuming, stakeRSR and totalStakes are both 100 (stakeRate is 1), if _payoutRewards(), stakeRSR will increase to 125, stakeRate will become 0.8. Now some stake will try to stake rsr of 100. For non pause/frozen mode, the new stakeAmout to mint is 80. Means this new staker will get 80 / (80 + 100) = 4 / 9 = 44% of future rewards. However, in pause/frozen mode, no rewards will be paid out, and stakeRSR will remain 100, stakeRate keeps 1. The new staker will get 100 StRSR to mint, and 100 / (100 + 100) = 1 / 2 = 50% of future rewards.

In the above example, the difference of 44% and 50% of future rewards shares comes from the missed accumulated rewards.

This staking and rewards mechanism will make the existing stakers lose rewards in the future. And malicious users can try to stake large amount on purpose if the system goes into pause/frozen mode.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Disallow stake() in pausedOrFrozen mode, just like payoutRewards() and unstake().

built in time units

File: contracts/p1/StRSR.sol
37:     uint48 public constant MAX_UNSTAKING_DELAY = 31536000; // {s} 1 year
38:     uint48 public constant MAX_REWARD_PERIOD = 31536000; // {s} 1 year

There are built in time units of 1 years in solidity

return value not used

File: contracts/p1/AssetRegistry.sol
80:         swapped = _registerIgnoringCollisions(asset);

156:         registered = _registerIgnoringCollisions(asset);
157:     }

Suggestion: check the return value

Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

File: contracts/p1/RToken.sol
452:         try main.furnace().melt() {} catch {}

Variable re-arrangement by storage packing

Reference: Layout of State Variables in Storage.

IDeployer.sol

In struct DeploymentParams, each uint48 can be placed with a uint192, as a result, the 2 variables will be packed into 1 slot storage. Otherwise, the uint192 could occupy 1 slot by itself. According to the currently layout, from line 24 to line 46, 7 uint192 and 6 uint48, there are total 8 slots, but after rearrangement, they can be packed into 7 slots.

File: contracts\interfaces\IDeployer.sol
19: struct DeploymentParams {
20:     // === Revenue sharing ===
21:     RevenueShare dist; // revenue sharing splits between RToken and RSR
22:     //
23:     // === Trade sizing ===
24:     uint192 minTradeVolume; // {UoA}
25:     uint192 rTokenMaxTradeVolume; // {UoA}
26:     //
27:     // === Freezing ===
28:     uint48 shortFreeze; // {s} how long an initial freeze lasts
29:     uint48 longFreeze; // {s} how long each freeze extension lasts
30:     //
31:     // === Rewards (Furnace + StRSR) ===
32:     uint192 rewardRatio; // the fraction of available revenues that stRSR holders get each PayPeriod
33:     uint48 rewardPeriod; // {s} the atomic unit of rewards, determines # of exponential rounds
34:     //
35:     // === StRSR ===
36:     uint48 unstakingDelay; // {s} the "thawing time" of staked RSR before withdrawal
37:     //
38:     // === BackingManager ===
39:     uint48 tradingDelay; // {s} how long to wait until starting auctions after switching basket
40:     uint48 auctionLength; // {s} the length of an auction
41:     uint192 backingBuffer; // {1} how much extra backing collateral to keep
42:     uint192 maxTradeSlippage; // {1} max slippage acceptable in a trade
43:     //
44:     // === RToken ===
45:     uint192 issuanceRate; // {1/block} number of RToken to issue per block / (RToken value)
46:     uint192 scalingRedemptionRate; // {1/hour} max fraction of supply that can be redeemed hourly
47:     uint256 redemptionRateFloor; // {qRTok/hour} the lowest possible hourly redemption limit
48: }

It can be rearranged to

struct DeploymentParams {
    // === Revenue sharing ===
    RevenueShare dist; // revenue sharing splits between RToken and RSR
    //
    // === Trade sizing ===
    uint192 minTradeVolume; // {UoA}
+   uint48 rewardPeriod; // {s} the atomic unit of rewards, determines # of exponential rounds
    uint192 rTokenMaxTradeVolume; // {UoA}
    //
    // === Freezing ===
    uint48 shortFreeze; // {s} how long an initial freeze lasts
    uint48 longFreeze; // {s} how long each freeze extension lasts
    //
    // === Rewards (Furnace + StRSR) ===
    uint192 rewardRatio; // the fraction of available revenues that stRSR holders get each PayPeriod
-    uint48 rewardPeriod; // {s} the atomic unit of rewards, determines # of exponential rounds
    //
    // === StRSR ===
-    uint48 unstakingDelay; // {s} the "thawing time" of staked RSR before withdrawal
    //
    // === BackingManager ===
-    uint48 tradingDelay; // {s} how long to wait until starting auctions after switching basket
    uint48 auctionLength; // {s} the length of an auction
    uint192 backingBuffer; // {1} how much extra backing collateral to keep
    uint192 maxTradeSlippage; // {1} max slippage acceptable in a trade
+    uint48 unstakingDelay; // {s} the "thawing time" of staked RSR before withdrawal
    //
    // === RToken ===
    uint192 issuanceRate; // {1/block} number of RToken to issue per block / (RToken value)
+    uint48 tradingDelay; // {s} how long to wait until starting auctions after switching basket
    uint192 scalingRedemptionRate; // {1/hour} max fraction of supply that can be redeemed hourly
    uint256 redemptionRateFloor; // {qRTok/hour} the lowest possible hourly redemption limit
}

FiatCollateral.sol

In struct CollateralConfig, uint48 priceTimeout can be placed next to uint192 oracleError, as a result, 1 slot storage can be saved. According to the currently layout, they both occupy 1 slot, but after re-arrangement, they can be packed into 1 slot.

File: contracts/plugins/assets/FiatCollateral.sol
11: struct CollateralConfig {
12:     uint48 priceTimeout; // {s} The number of seconds over which saved prices decay
13:     AggregatorV3Interface chainlinkFeed; // Feed units: {target/ref}
14:     uint192 oracleError; // {1} The % the oracle feed can be off by
15:     IERC20Metadata erc20; // The ERC20 of the collateral token
16:     uint192 maxTradeVolume; // {UoA} The max trade volume, in UoA
17:     uint48 oracleTimeout; // {s} The number of seconds until a oracle value becomes invalid
18:     bytes32 targetName; // The bytes32 representation of the target name
19:     uint192 defaultThreshold; // {1} A value like 0.05 that represents a deviation tolerance
20:     // set defaultThreshold to zero to create SelfReferentialCollateral
21:     uint48 delayUntilDefault; // {s} The number of seconds an oracle can mulfunction
22: }

can be changed to

struct CollateralConfig {
-    uint48 priceTimeout; // {s} The number of seconds over which saved prices decay
    AggregatorV3Interface chainlinkFeed; // Feed units: {target/ref}
+    uint48 priceTimeout; // {s} The number of seconds over which saved prices decay    
    uint192 oracleError; // {1} The % the oracle feed can be off by
    IERC20Metadata erc20; // The ERC20 of the collateral token
    uint192 maxTradeVolume; // {UoA} The max trade volume, in UoA
    uint48 oracleTimeout; // {s} The number of seconds until a oracle value becomes invalid
    bytes32 targetName; // The bytes32 representation of the target name
    uint192 defaultThreshold; // {1} A value like 0.05 that represents a deviation tolerance
    // set defaultThreshold to zero to create SelfReferentialCollateral
    uint48 delayUntilDefault; // {s} The number of seconds an oracle can mulfunction
}

assets[erc20] can be saved to local cache

Each storage read uses opcode sload which costs 100 gas (warm access), while memory read uses mload which only cost 3 gas. (reference)

File: contracts/p1/AssetRegistry.sol
165:             if (assets[erc20] == asset) return false;
166:             else emit AssetUnregistered(erc20, assets[erc20]);

Suggestion: Save assets[erc20] to local memory variable to save 1 sload operation.

asset.erc20() can be saved to local variable

Each asset.erc20() will incur function call of getter and 1 sload. Each storage read uses opcode sload which costs 100 gas (warm access), while memory read uses mload which only cost 3 gas. (reference).

File: contracts/p1/AssetRegistry.sol
87:     function unregister(IAsset asset) external governance {
88:         require(_erc20s.contains(address(asset.erc20())), "no asset to unregister");
89:         require(assets[asset.erc20()] == asset, "asset not found");
90:         uint192 quantity = basketHandler.quantity(asset.erc20());
91: 
92:         _erc20s.remove(address(asset.erc20()));
93:         assets[asset.erc20()] = IAsset(address(0));
94:         emit AssetUnregistered(asset.erc20(), asset);
95: 
96:         if (quantity > 0) basketHandler.disableBasket();
97:     }

Suggestion: Save asset.erc20() to local memory variable to save 6 sload operation.

Splitting require() statements that use &&

require() statements with multiple conditions can be split.

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper.

The demo of the gas comparison can be seen here.

Instances number of this issue: 2

File: contracts/p1/Deployer.sol
48:         require(
49:             address(rsr_) != address(0) &&
50:                 address(gnosis_) != address(0) &&
51:                 address(rsrAsset_) != address(0) &&
52:                 address(implementations_.main) != address(0) &&
53:                 address(implementations_.trade) != address(0) &&
54:                 address(implementations_.components.assetRegistry) != address(0) &&
55:                 address(implementations_.components.backingManager) != address(0) &&
56:                 address(implementations_.components.basketHandler) != address(0) &&
57:                 address(implementations_.components.broker) != address(0) &&
58:                 address(implementations_.components.distributor) != address(0) &&
59:                 address(implementations_.components.furnace) != address(0) &&
60:                 address(implementations_.components.rsrTrader) != address(0) &&
61:                 address(implementations_.components.rTokenTrader) != address(0) &&
62:                 address(implementations_.components.rToken) != address(0) &&
63:                 address(implementations_.components.stRSR) != address(0),
64:             "invalid address"
65:         );


File: contracts/p1/RevenueTrader.sol
72:         require(buyPrice > 0 && buyPrice < FIX_MAX, "buy asset price unknown");

Deployment gas

Line 83-84 is duplication of the function getPastEra().

File: contracts/p1/StRSRVotes.sol
82:     function getPastTotalSupply(uint256 blockNumber) public view returns (uint256) {
83:         require(blockNumber < block.number, "ERC20Votes: block not yet mined");
84:         uint256 pastEra = _checkpointsLookup(_eras, blockNumber);
85:         return _checkpointsLookup(_totalSupplyCheckpoints[pastEra], blockNumber);
86:     }
87: 
88:     function getPastEra(uint256 blockNumber) public view returns (uint256) {
89:         require(blockNumber < block.number, "ERC20Votes: block not yet mined");
90:         return _checkpointsLookup(_eras, blockNumber);
91:     }

Suggestion: replace line 83-84 with getPastEra().

Iterating in totals()

Each time totals() is called, the whole destinations will be looped. Each iteration involves sload, which cost 100 gas.

File: contracts/p1/Distributor.sol
141:     function totals() public view returns (RevenueTotals memory revTotals) {
142:         uint256 length = destinations.length();
143:         for (uint256 i = 0; i < length; ++i) {
144:             RevenueShare storage share = distribution[destinations.at(i)];
145:             revTotals.rTokenTotal += share.rTokenDist;
146:             revTotals.rsrTotal += share.rsrDist;
147:         }
148:     }

Suggestion: Save the total sum into some variable, and update it if the destinations is changed. Instead of duplicate looping.

Replace sload with memory variable

Each storage read uses opcode sload which costs 100 gas (warm access), while memory read uses mload which only cost 3 gas. (reference)

unstakingDelay in line 816 can be replaced by val. Saving 1 sload.

File: contracts/p1/StRSR.sol
815:         unstakingDelay = val;
816:         require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");

rewardPeriod in line 824 can be replaced by val. Saving 1 sload.

File: contracts/p1/StRSR.sol
823:         rewardPeriod = val;
824:         require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");