From 383633c735309d854311c3be5b1e1d2d23679637 Mon Sep 17 00:00:00 2001 From: Daniel Von Fange Date: Sat, 13 Feb 2021 12:15:00 -0500 Subject: [PATCH 1/3] Simplification and correct rounding on OUSD change supply. --- contracts/contracts/token/OUSD.sol | 38 ++++++++++++++++++++------- contracts/test/vault/rebase.js | 41 +++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/contracts/contracts/token/OUSD.sol b/contracts/contracts/token/OUSD.sol index 74626651ef..99f7ebf3df 100644 --- a/contracts/contracts/token/OUSD.sol +++ b/contracts/contracts/token/OUSD.sol @@ -449,8 +449,17 @@ contract OUSD is Initializable, InitializableERC20Detailed, Governable { onlyVault nonReentrant { + // Prevents division by zero require(_totalSupply > 0, "Cannot increase 0 supply"); - + // We currently require the OUSD balance to only go up. + // If in the future OUSD is changed to rebase down, then all math in + // this contract needs to be rechecked. + require(_newTotalSupply >= _totalSupply); + // In order for our multiple-then-divide operations to work, we need to + // keep enough bit headroom. + require(_newTotalSupply <= MAX_SUPPLY); + + // If we would divide by zero, emit our event and return. if (_totalSupply == _newTotalSupply) { emit TotalSupplyUpdated( _totalSupply, @@ -460,20 +469,31 @@ contract OUSD is Initializable, InitializableERC20Detailed, Governable { return; } - _totalSupply = _newTotalSupply > MAX_SUPPLY - ? MAX_SUPPLY - : _newTotalSupply; - - rebasingCreditsPerToken = rebasingCredits.divPrecisely( - _totalSupply.sub(nonRebasingSupply) - ); + // Calculates the inverse value of each credit. The add(1) at the end + // ensures that any rounding error is in favor of the pool + rebasingCreditsPerToken = rebasingCredits + .divPrecisely(_newTotalSupply.sub(nonRebasingSupply)) + .add(1); + // If rebasingCreditsPerToken is ever 0, then all the accounting + // between rebasing and non-rebasing accounts will be wrong. The above + // add(1) should prevent this from ever reaching 0, but it is a + // critical invarient and should have a require. require(rebasingCreditsPerToken > 0, "Invalid change in supply"); - _totalSupply = rebasingCredits + // actualSupply is the sum of all OUSD accounts + uint256 actualSupply = rebasingCredits .divPrecisely(rebasingCreditsPerToken) .add(nonRebasingSupply); + // Verify that the sum of all OUSD accounts is equal to or less than + // the value of backing assets. This ensures that all rounding has been + // done correctly, and all OUSD tokens are fully backed. + require(_newTotalSupply >= actualSupply); + + // Store supply. + _totalSupply = _newTotalSupply; + emit TotalSupplyUpdated( _totalSupply, rebasingCredits, diff --git a/contracts/test/vault/rebase.js b/contracts/test/vault/rebase.js index 6332fe5822..ff0d9d43e8 100644 --- a/contracts/test/vault/rebase.js +++ b/contracts/test/vault/rebase.js @@ -1,5 +1,5 @@ const { defaultFixture } = require("../_fixture"); -const { expect } = require("chai"); +const { expect, assert } = require("chai"); const { ousdUnits, @@ -252,3 +252,42 @@ describe("Vault yield accrual to OGN", async () => { }); }); }); + +describe("OUSD rebasing", async () => { + let fixture; + + async function expectSolvantVault() { + const vaultValue = await fixture.vault.totalValue(); + const ousdSupply = await fixture.ousd.totalSupply(); + const rebaseRatio = await fixture.ousd.rebasingCreditsPer; + assert(ousdSupply <= vaultValue, "OUSD supply greater than vault value"); + } + + it("should correctly round rebases", async () => { + fixture = await loadFixture(defaultFixture); + const { matt, josh, dai, ousd, vault } = fixture; + await expectSolvantVault(); + + await dai.connect(matt).mint(daiUnits("1000000000000000000")); + await dai + .connect(matt) + .approve(vault.address, daiUnits("1000000000000000000")); + await vault + .connect(matt) + .mint(dai.address, daiUnits("1000000000000000000"), 0); + await expectSolvantVault(); + + await dai.mint(1); + await dai.transfer(vault.address, 1); + await vault.rebase(); + await expectSolvantVault(); + + const whaleBalance = await ousd.balanceOf(matt.address); + await vault.connect(matt).redeem(whaleBalance, 0); + await expectSolvantVault(); + + const joshBalance = await ousd.balanceOf(josh.address); + await vault.connect(josh).redeem(joshBalance, 0); + await expectSolvantVault(); + }); +}); From f8489ecc4ae8d851dc421a57f065ed41758e954b Mon Sep 17 00:00:00 2001 From: Daniel Von Fange Date: Mon, 15 Feb 2021 10:00:01 -0500 Subject: [PATCH 2/3] Comments update and remove our return early for no change. --- contracts/contracts/token/OUSD.sol | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/contracts/contracts/token/OUSD.sol b/contracts/contracts/token/OUSD.sol index 99f7ebf3df..69413abf32 100644 --- a/contracts/contracts/token/OUSD.sol +++ b/contracts/contracts/token/OUSD.sol @@ -451,26 +451,18 @@ contract OUSD is Initializable, InitializableERC20Detailed, Governable { { // Prevents division by zero require(_totalSupply > 0, "Cannot increase 0 supply"); - // We currently require the OUSD balance to only go up. - // If in the future OUSD is changed to rebase down, then all math in - // this contract needs to be rechecked. + + // We currently require the OUSD balance to only go up. If in the + // future OUSD is changed to also rebase down, then all math in this + // contract needs to be rechecked. require(_newTotalSupply >= _totalSupply); - // In order for our multiple-then-divide operations to work, we need to - // keep enough bit headroom. - require(_newTotalSupply <= MAX_SUPPLY); - // If we would divide by zero, emit our event and return. - if (_totalSupply == _newTotalSupply) { - emit TotalSupplyUpdated( - _totalSupply, - rebasingCredits, - rebasingCreditsPerToken - ); - return; - } + // Ensures headroom for mathmatical operations + require(_newTotalSupply <= MAX_SUPPLY); // Calculates the inverse value of each credit. The add(1) at the end - // ensures that any rounding error is in favor of the pool + // ensures that any rounding errors round rebasing accounts down. + // If we rounded up, we would exceed the total supply. rebasingCreditsPerToken = rebasingCredits .divPrecisely(_newTotalSupply.sub(nonRebasingSupply)) .add(1); @@ -478,7 +470,7 @@ contract OUSD is Initializable, InitializableERC20Detailed, Governable { // If rebasingCreditsPerToken is ever 0, then all the accounting // between rebasing and non-rebasing accounts will be wrong. The above // add(1) should prevent this from ever reaching 0, but it is a - // critical invarient and should have a require. + // critical invarient and so we make this explicit. require(rebasingCreditsPerToken > 0, "Invalid change in supply"); // actualSupply is the sum of all OUSD accounts From 8f2df077b6652d48cbd56878c07116b3c56d01b9 Mon Sep 17 00:00:00 2001 From: Daniel Von Fange Date: Mon, 15 Feb 2021 10:09:37 -0500 Subject: [PATCH 3/3] Spelling FTW. --- contracts/contracts/token/OUSD.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/contracts/token/OUSD.sol b/contracts/contracts/token/OUSD.sol index 69413abf32..4161f27117 100644 --- a/contracts/contracts/token/OUSD.sol +++ b/contracts/contracts/token/OUSD.sol @@ -451,13 +451,13 @@ contract OUSD is Initializable, InitializableERC20Detailed, Governable { { // Prevents division by zero require(_totalSupply > 0, "Cannot increase 0 supply"); - + // We currently require the OUSD balance to only go up. If in the // future OUSD is changed to also rebase down, then all math in this // contract needs to be rechecked. require(_newTotalSupply >= _totalSupply); - // Ensures headroom for mathmatical operations + // Ensures headroom for mathematical operations require(_newTotalSupply <= MAX_SUPPLY); // Calculates the inverse value of each credit. The add(1) at the end @@ -470,7 +470,7 @@ contract OUSD is Initializable, InitializableERC20Detailed, Governable { // If rebasingCreditsPerToken is ever 0, then all the accounting // between rebasing and non-rebasing accounts will be wrong. The above // add(1) should prevent this from ever reaching 0, but it is a - // critical invarient and so we make this explicit. + // critical invariant and so we make this explicit. require(rebasingCreditsPerToken > 0, "Invalid change in supply"); // actualSupply is the sum of all OUSD accounts