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

evm-contracts/deployment-audit-xhack #472

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

swimivan
Copy link
Contributor

fixed version of #463 which accidentally included changes to pool-math package

see individual commit messages for an overview of the introduced changes

Checklist

  • Github project and label have been assigned
  • Tests have been added or are unnecessary
  • Docs have been updated or are unnecessary
  • Preview deployment works (visit the Cloudflare preview URL)
  • Manual testing in #product-testing completed or unnecessary
  • New i18n strings do not contain any security vulnerabilities (e.g. should not allow translator to update email/url)
  • When fetching new translations from external sources, do a sanity check (e.g. get people who speak the language to check, shove all new translations into Google translate)

…ployment

* audit: add missing whenNotPaused modifier
* audit: fix amp factor logic in Pool
* audit: emit additional events in Routing and Pool
* fix: throw in propellerComplete() when not a Propeller transaction
* fix: cap remunerated gas price by tx.gasprice
* feat: make remunerated Propeller gas price depend on specific chain
* feat: implement Decimal interface for fixedSwimUsdPerGasToken
* feat: expose Wormhole nonce in function signatures for upcoming BatchVAAs
* feat: catch Panic thrown by failed Wormhole interactions
* feat: compile contracts using viaIR optimization
* chore: improve gas estimation for Propeller gas fee remuneration
* chore: have registerToken() clean up old registrations and check inputs
* chore: clean-up and relocate contract structs, enums, events, and constants
* chore: update constants after deployment
* feat: use token-projects package to determine token numbers
* feat: use pool-math package for testing to replace hardcoded values
* feat: include attestation of SwimUSD in deployment process when possible
* feat: implement MOCK for Routing contract itself to isolate pool tests
* refactor: replace BigNumber with more suited Decimal in test code
* refactor: further improve test code and Wrapper classes
@swimivan swimivan added bug Something isn't working refactoring feature New feature or request improvement Improving an existing feature dependencies Pull requests that update a dependency file labels Oct 27, 2022
@swimivan swimivan requested a review from arvakr October 27, 2022 00:58
@swimivan swimivan self-assigned this Oct 27, 2022
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 27, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9843e55
Status: ✅  Deploy successful!
Preview URL: https://b6373d22.swim-ui.pages.dev
Branch Preview URL: https://evm-contracts-deployment-aud-wga2.swim-ui.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented Oct 27, 2022

✨ Deployment complete! Take a peek over at https://da62a50d.ui-storybook.pages.dev

Copy link
Collaborator

@arvakr arvakr left a comment

Choose a reason for hiding this comment

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

Didn't go into details on many functions because I'm lacking context. If you'd consider them security-critical, probably worth doing a follow-up audit soon.

print("---------------------")
return [fr_profit, user_output]

def profitability_threshold(swap_amount, debug=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love this whole setup

@@ -0,0 +1,3 @@
MNEMONIC=test test test test test test test test test test test junk
FACTORY_MNEMONIC=try exercise column boring supreme corn fabric idea federal today hood equip
Copy link
Collaborator

Choose a reason for hiding this comment

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

"try exercise" :D

uint64 serviceFee; //specified in swimUSD
//service fee specified in swimUSD
uint64 serviceFee;
//specified in atomic with 18 decimals, i.e. how many atomic swimUSD per 1 wei?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd have put a new line above, just to make sure it's not accidentally read as relating to serviceFee

@@ -393,12 +485,16 @@ contract Routing is
toTokenInfo = tokenNumberMapping[swimPayload.toTokenNumber];
if (toTokenInfo.tokenNumber != 0) {
//if toTokenNumber in swimPayload was invalid for whatever reason then just return
//swimUsd to prevent propeller transaction from getting stuck
// swimUsd to prevent propeller transaction from getting stuck
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious about the added space? I prefer this but most other comments don't have a leading space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use the indentation to signify "same paragraph" or even more generally to indicate that the line break is only a "line wrap".

}

function setup(address[] calldata tokens_, uint[] calldata amounts_) external {
require(msg.sender == _owner, "computer says no");
Copy link
Collaborator

Choose a reason for hiding this comment

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

😂

@@ -12,10 +12,11 @@ import { task } from "hardhat/config";
import type { HardhatUserConfig, HttpNetworkUserConfig } from "hardhat/types";

dotenv.config();
//update .env.examples if you add additional environment variables!
Copy link
Collaborator

Choose a reason for hiding this comment

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

.env.example

@@ -38,6 +39,7 @@ task(
async ({ proxy, logic, owner }, hre) => {
const { ethers } = hre;
const _owner = owner ? await ethers.getSigner(owner as string) : (await ethers.getSigners())[0];
//TODO check that proxy and logic exist
Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct to still be a todo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not anymore in a second

? 0
: Object.values(TokenProjectId)
.map((id) => TOKEN_PROJECTS_BY_ID[id])
//TODO we're using includes() and lenght checking here instead of === here because e.g.
Copy link
Collaborator

Choose a reason for hiding this comment

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

*length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

impressive catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file feature New feature or request improvement Improving an existing feature refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants