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 #303

Open
code423n4 opened this issue Jun 26, 2022 · 0 comments
Open

QA Report #303

code423n4 opened this issue Jun 26, 2022 · 0 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

Use self-explanatory variable name instead of abbreviations to increase readability

With variable names like p, u, m, a, and so on it become really difficult to read the code for developers, auditors and users (those natspec comments will also be used by external tools/services like Etherscan).

Recommendation: rename variable names to be self-explanatory

Mixed use of require and custom error

The code seems to use a mix of require and custom errors to revert the transaction.
Consider using only a pattern to be more consistent across the project.
Custom error are usually gas cheaper compared to require.

Missing event emission

Event emission are important to monitor contract activity with external tool/services.
Consider adding event emission to the following function:

Redeemer.sol

  • setAdmin
  • setMarketPlace
  • setLender
  • setLender
  • setSwivel

MarketPlace.sol

  • setPrincipal
  • setAdmin
  • setPool
  • sellPrincipalToken
  • buyPrincipalToken
  • sellUnderlying
  • buyUnderlying
  • mint
  • mintWithUnderlying
  • burn
  • burnForUnderlying

Lender.sol

  • approve (both of them)
  • setAdmin
  • setFee
  • setMarketPlace
  • setSwivel
  • withdrawFee
  • pause

Consider implementing the event emission for these functions

Lender setFee allow the admin to drain all the user's lent funds

feenominator is initialized in the contract's constructor as feenominator = 1000; but the admin of the contract can update that value via setFee.

The setFee function has no check on the max value that the feenominator state variable could be updated to.

Consider adding a max value to the function.

Lender setSwivel natspec is wrong

The current natspec for the function say /// @notice sets the feenominator to the given value but the function is updating the swivelAddr state variable and not the feenominator.

Consider updating the natspec comment to correctly describe what the function does.

Lender withdraw should also reset the fees variable

When the withdraw(token) method is called, the function is transferring all the amount of token from the Lender contract to admin.

Before the transfer is called, the fees[token] state variable should also be resetted if fees[token] > 0

@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 26, 2022
code423n4 added a commit that referenced this issue Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 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

1 participant