Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vault security improvements #5440

Merged
merged 6 commits into from
May 27, 2022
Merged

vault security improvements #5440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
692773b
refactor(interest): eschew partialAssign which made state vulnerable …
turadg May 26, 2022
b34e98d
don't use partialAssign in contract state
turadg May 27, 2022
9a35d3c
refactor: inline shortfall/overage calculation
turadg May 26, 2022
109145e
test: stop spamming Ava test info output
turadg May 26, 2022
2a808fa
chore: logging cleanup
turadg May 26, 2022
848148c
chore: type fixes
turadg May 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/deploy-script-support/test/unitTests/test-startInstance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ test('startInstance', async t => {
const zoeInvitationIssuer = E(zoe).getInvitationIssuer();
const zoeInvitationPurse = E(zoeInvitationIssuer).makeEmptyPurse();

/** @type {import('../../src/startInstance.js').IssuerManager} */
// @ts-expect-error cast mock
const issuerManager = {
get: petname => {
if (petname === MOOLA_BRAND_PETNAME) {
Expand All @@ -47,10 +49,15 @@ test('startInstance', async t => {
},
};

/** @type {Petname=} */
let addedPetname;

/** @type {import('../../src/startInstance.js').InstanceManager} */
// @ts-expect-error cast mock
const instanceManager = {
add: (petname, _instance) => (addedPetname = petname),
add: async (petname, _instance) => {
addedPetname = petname;
},
};

const startInstance = makeStartInstance(
Expand Down
2 changes: 1 addition & 1 deletion packages/run-protocol/src/proposals/econ-behaviors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@ export const startRewardDistributor = async ({
brand: centralBrand,
})
.catch(e => {
console.log('Cannot create fee collector deposit facet', e);
console.error('Cannot create fee collector deposit facet', e);
return undefined;
});

Expand Down
6 changes: 4 additions & 2 deletions packages/run-protocol/src/runStake/runStakeManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { makeRatio } from '@agoric/zoe/src/contractSupport/ratio.js';
import { fit, getCopyBagEntries, M } from '@agoric/store';
import { makeNotifierKit, observeNotifier } from '@agoric/notifier';
import { E } from '@endo/far';
import { defineKindMulti, partialAssign } from '@agoric/vat-data';
import { defineKindMulti } from '@agoric/vat-data';
import { makeTracer } from '../makeTracer.js';
import { chargeInterest } from '../interest.js';
import { ManagerKW as KW } from './constants.js';
Expand Down Expand Up @@ -162,7 +162,9 @@ const helper = {
},
updateTime,
);
partialAssign(state, changes);
state.compoundedInterest = changes.compoundedInterest;
state.latestInterestUpdate = changes.latestInterestUpdate;
state.totalDebt = changes.totalDebt;

const payload = harden({
compoundedInterest: state.compoundedInterest,
Expand Down
29 changes: 9 additions & 20 deletions packages/run-protocol/src/vaultFactory/liquidation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,6 @@ import { makeTracer } from '../makeTracer.js';

const trace = makeTracer('LIQ', false);

/**
*
* @param {Amount<'nat'>} debt
* @param {Amount<'nat'>} proceeds
*/
const discrepancy = (debt, proceeds) => {
if (AmountMath.isGTE(debt, proceeds)) {
return {
overage: AmountMath.makeEmptyFromAmount(debt),
shortfall: AmountMath.subtract(debt, proceeds),
};
} else {
return {
overage: AmountMath.subtract(proceeds, debt),
shortfall: AmountMath.makeEmptyFromAmount(debt),
};
}
};

/**
* Liquidates a Vault, using the strategy to parameterize the particular
* contract being used. The strategy provides a KeywordMapping and proposal
Expand Down Expand Up @@ -86,7 +67,15 @@ const liquidate = async (
]);
// NB: all the proceeds from AMM sale are on the vault seat instead of a staging seat

const { shortfall, overage } = discrepancy(debt, proceeds.RUN);
const [overage, shortfall] = AmountMath.isGTE(debt, proceeds.RUN)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is better. read better before, imho.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either way is OK to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have gone with

  const short = AmountMath.isGTE(debt, proceeds.RUN);
  const overage = short
    ? AmountMath.makeEmptyFromAmount(debt)
    : AmountMath.subtract(proceeds.RUN, debt);
  const shortfall = short
    ? AmountMath.subtract(debt, proceeds.RUN)
    : AmountMath.makeEmptyFromAmount(debt);

I'm not a fan of the trinary assigning two values

? [
AmountMath.makeEmptyFromAmount(debt),
AmountMath.subtract(debt, proceeds.RUN),
]
: [
AmountMath.subtract(proceeds.RUN, debt),
AmountMath.makeEmptyFromAmount(debt),
];

const runToBurn = AmountMath.min(proceeds.RUN, debt);
trace('before burn', { debt, proceeds, overage, shortfall, runToBurn });
Expand Down
7 changes: 5 additions & 2 deletions packages/run-protocol/src/vaultFactory/prioritizedVaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import { Far } from '@endo/marshal';
import { keyEQ, keyLT } from '@agoric/store';
import { makeOrderedVaultStore } from './orderedVaultStore.js';
import { toVaultKey } from './storeUtils.js';
import { makeTracer } from '../makeTracer.js';

const trace = makeTracer('PV');

/** @typedef {import('./vault').Vault} Vault */

Expand Down Expand Up @@ -99,7 +102,7 @@ export const makePrioritizedVaults = (reschedulePriceCheck = () => {}) => {
// don't call reschedulePriceCheck, but do reset the highest.
// This could be expensive if we delete individual entries in
// order. Will know once we have perf data.
console.log('removeVault', firstKey, key);
trace('removeVault', firstKey, key);
if (keyEQ(key, firstKey)) {
const [secondKey] = vaults.keys();
firstKey = secondKey;
Expand All @@ -125,7 +128,7 @@ export const makePrioritizedVaults = (reschedulePriceCheck = () => {}) => {
*/
const addVault = (vaultId, vault) => {
const key = vaults.addVault(vaultId, vault);
console.log('addVault', firstKey, key);
trace('addVault', firstKey, key);
if (!firstKey || keyLT(key, firstKey)) {
firstKey = key;
reschedulePriceCheck();
Expand Down
13 changes: 8 additions & 5 deletions packages/run-protocol/src/vaultFactory/vaultManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
} from '@agoric/notifier';
import { AmountMath } from '@agoric/ertp';

import { defineKindMulti, partialAssign, pickFacet } from '@agoric/vat-data';
import { defineKindMulti, pickFacet } from '@agoric/vat-data';
import { makeVault } from './vault.js';
import { makePrioritizedVaults } from './prioritizedVaults.js';
import { liquidate } from './liquidation.js';
Expand Down Expand Up @@ -232,7 +232,7 @@ const helperBehavior = {

// Update state with the results of charging interest

const stateUpdates = chargeInterest(
const changes = chargeInterest(
{
mint: state.debtMint,
mintAndReallocateWithFee: state.factoryPowers.mintAndReallocate,
Expand All @@ -255,7 +255,11 @@ const helperBehavior = {
},
updateTime,
);
partialAssign(state, stateUpdates);

state.compoundedInterest = changes.compoundedInterest;
state.latestInterestUpdate = changes.latestInterestUpdate;
state.totalDebt = changes.totalDebt;

facets.helper.assetNotify();
trace('chargeAllVaults complete');
facets.helper.reschedulePriceCheck();
Expand Down Expand Up @@ -326,7 +330,7 @@ const helperBehavior = {
// eslint-disable-next-line consistent-return
return facets.helper
.processLiquidations()
.catch(e => console.log('Liquidator failed', e))
.catch(e => console.error('Liquidator failed', e))
.finally(() => {
liquidationInProgress = false;
});
Expand Down Expand Up @@ -426,7 +430,6 @@ const helperBehavior = {
factoryPowers.getGovernedParams().getLiquidationPenalty(),
)
.then(accounting => {
console.log('liquidateAndRemove accounting', accounting);
state.totalProceedsReceived = AmountMath.add(
state.totalProceedsReceived,
accounting.proceeds,
Expand Down
4 changes: 3 additions & 1 deletion packages/run-protocol/test/supports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { makeFakeVatAdmin } from '@agoric/zoe/tools/fakeVatAdmin.js';
import buildManualTimer from '@agoric/zoe/tools/manualTimer.js';
import { makeLoopback } from '@endo/captp';
import { E } from '@endo/far';
import { makeTracer } from '../src/makeTracer.js';

/**
* @param {*} t
Expand Down Expand Up @@ -60,7 +61,8 @@ export const setUpZoeForTest = (setJig = () => {}) => {
harden(setUpZoeForTest);

export const setupBootstrap = (t, optTimer = undefined) => {
const space = /** @type {any} */ (makePromiseSpace(t.log));
const trace = makeTracer('PromiseSpace');
const space = /** @type {any} */ (makePromiseSpace(trace));
const { produce, consume } =
/** @type { import('../src/proposals/econ-behaviors.js').EconomyBootstrapPowers & BootstrapPowers } */ (
space
Expand Down
3 changes: 3 additions & 0 deletions packages/zoe/src/zoeService/zoe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,6 @@ const makeZoeKit = (
};

export { makeZoeKit };
/**
* @typedef {ReturnType<typeof makeZoeKit>} ZoeKit
*/