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

Open
code423n4 opened this issue May 14, 2022 · 0 comments
Open

QA Report #233

code423n4 opened this issue May 14, 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

Low

  1. Users can create vaults with a malicious token addresses

In Cally.sol a user can call the createVault() function passing in a malicious attacker controlled token as the address token. This will then create a vault setting the evil token address as in storage. Attacker controlled assets should never be allowed in a protocol as they can return arbitrary values when called upon performing other malicious tasks then whats expected.

##POC
https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L160

##Tool Used
Manual Review

##Recommended Mitigation
Consider adding a mapping of approved tokens that can be passed in as an token address to the createVault() function to avoid malicious tokens.

  1. Used safeMint() rather than mint()

This POC It can be consider for dev if may necessary for better way

The `_safeMint` can be using if minting causes the recipient of the tokens, if it is a smart contract, to react upon receipt of the tokens.

Here is a general consideration to help decide which one to use:

If YOU are paying for the minting of tokens, use `_mint`. The `_safeMint` might cost you an arbitrary amount of money because of choices made by the recipient of the tokens.

If THEY are paying for the minting of tokens and you expect buyers to be composing functionality with smart contracts, use _safeMint. There is some marginal benefit of allowing the extra features with smart contracts this allows.

If THEY are paying and you expect INDIVIDUAL PEOPLE to buy tokens, then use _mint. The extra features in _safeMint are not expected.

The extra cost from using _safeMint is non-zero. And cost is also always a concern.

since Cally.sol was used to be main actor, it can be used safemint() instead of mint()

##Tool Used
Manual Review

  1. Title : comment @return was not set

This was not set information for @return, since other function() was set if @return was used so it can be added for good information to the others.

##Tool Used
Manual Review

Non Critical

  1. TItle : Typo Comment

It should be an overrides function but instead of overrides, dev was used ovverides. It can be remain the same, or it can be changed instead.

##Tool Used
Manual Review

@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 May 14, 2022
code423n4 added a commit that referenced this issue May 14, 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