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

QA Report #40

Open
code423n4 opened this issue Mar 18, 2022 · 1 comment
Open

QA Report #40

code423n4 opened this issue Mar 18, 2022 · 1 comment
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

Lows and Non-Critical

check that the deposit cap is greater than the deposit amount in setGlobalDepositCap in the CollateralDepositRecord contract

This might be a thing you want to check, to avoid from a situation where the system is in an invalid state.

inconsistent usage of safe and unsafe transfer functions

Use safeTransfer and safeTransferFrom instead of using transfer and transferFrom in the PrePOMarket contract.

wrong comment in the deposit function of the Collateral contract

cost per share is total value / total supply, and not total supply / total value (the implementation is correct, just the comment is wrong).

/**
* # of shares owed = amount deposited / cost per share, cost per
* share = total supply / total value.
*/

front-runnable initializer (can be solved using access controls)

An attacker can front run and call the initialize function with unwanted values and taking over the ownership of the contract, making the contract to be redeployed. This can be avoided by giving access controls in the constructor, which means that only the address that created the contract will be able to call the initialize function.

LongShortToken contract doesn't implement the corresponding ILongShortToken interface (it does implement its functions, just not declaring that it is implementing it)

@code423n4 code423n4 added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax bug Something isn't working labels Mar 18, 2022
code423n4 added a commit that referenced this issue Mar 18, 2022
@ramenforbreakfast
Copy link
Collaborator

deposit cap issue is a duplicate of #1, and is not valid
inconsistent usage of safe/unsafe is not valid because we know the Collateral contract reverts safely on failed transfers and do not need the additional weight of the safe wrapper.
wrong comment is a valid submission
front runnable initializer is a duplicate of #4 and not an issue.

This should have severity lowered to 0 for valid wrong comment submission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

2 participants