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.

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

Fuse fixed accounting #10

Open
wants to merge 30 commits into
base: fuse-reactive-audit
Choose a base branch
from
Open

Conversation

sriyantra
Copy link
Collaborator

No description provided.

@kryptoklob
Copy link

Something to check before we merge this: need to make sure that there is now way to game the protocol / exploit something during the upgrade, ie between the first and second transactions.

@eswak
Copy link

eswak commented May 6, 2022

need to make sure that there is now way to game the protocol / exploit something during the upgrade, ie between the first and second transactions

you could deploy a contract that does all calls atomically

@kryptoklob
Copy link

need to make sure that there is now way to game the protocol / exploit something during the upgrade, ie between the first and second transactions

you could deploy a contract that does all calls atomically

Flashbots would also work. Yea.

@crypto-perry
Copy link

need to make sure that there is now way to game the protocol / exploit something during the upgrade, ie between the first and second transactions

you could deploy a contract that does all calls atomically

Flashbots would also work. Yea.

Hmm, I wouldn't risk it with flashbots as the block can be uncled and then txs are in mempool. It is very rare but I have seen frontruns on txs sent to flashbots due to this issue.

@kryptoklob
Copy link

need to make sure that there is now way to game the protocol / exploit something during the upgrade, ie between the first and second transactions

you could deploy a contract that does all calls atomically

Flashbots would also work. Yea.

Hmm, I wouldn't risk it with flashbots as the block can be uncled and then txs are in mempool. It is very rare but I have seen frontruns on txs sent to flashbots due to this issue.

Actually a great point

@Austin-Williams
Copy link

Austin-Williams commented May 6, 2022

need to make sure that there is now way to game the protocol / exploit something during the upgrade, ie between the first and second transactions

you could deploy a contract that does all calls atomically

Flashbots would also work. Yea.

Hmm, I wouldn't risk it with flashbots as the block can be uncled and then txs are in mempool. It is very rare but I have seen frontruns on txs sent to flashbots due to this issue.

You can use uncle protection to prevent this.

You can do a require(blockhash(block.number - 1) == _expectedParentHash, "unexpected parent block") check.
Then, on each new block, you submit a new copy of the bundle with the new _expectedParentHash until one of the bundles lands.

@sriyantra
Copy link
Collaborator Author

Something to check before we merge this: need to make sure that there is now way to game the protocol / exploit something during the upgrade, ie between the first and second transactions.

by first and 2nd transactions you mean the 1st and 2nd upgrades? They'll be executed atomically through fuseAdmin's callPool(arbitrary logic) anyway so I don't think this'll be an issue. Also don't see how anything could be gamed but maybe I'm missing something.

describe("CEtherDelegateTempExploitAccounting", function () {
it("Should merge the attacker's supply and borrow balances", async function () {
// Enable using 0 gas price
await hre.network.provider.send("hardhat_setNextBlockBaseFeePerGas", ["0x0"]);

Choose a reason for hiding this comment

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

This should be done in a before hook, not in the test itself

require(accrueInterest() == uint(Error.NO_ERROR), "!accrue");

// Get secondary accounts from data
(address[] memory secondaryAccounts) = abi.decode(data, (address[]));
Copy link

@zerosnacks zerosnacks May 13, 2022

Choose a reason for hiding this comment

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

Please ignore my previous comment.

Just a heads up: yarn lint currently throws on this line - not sure why exactly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants