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

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

QA Report #25

code423n4 opened this issue Mar 31, 2022 · 1 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

Comments

@code423n4
Copy link
Contributor

1) function _accept() Use UnSafe and Deprecated safeApprove

Risk Rating: Low

Proof of Concept

https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L335

Recommended

The OpenZeppelin SafeERC20 safeApprove() function has been deprecated. Using this deprecated function can lead to unintended reverts and potentially the locking of funds.
Discussion: OpenZeppelin/openzeppelin-contracts#2219

As suggested by the OpenZeppelin comment, replace safeApprove() with safeIncreaseAllowance().

2) Suggest function liquidate() Open to Public Rather than Lenders Only

Risk Rating: Informational

Proof of Concept

https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L606-L627

Recommended

@dev only one of the lenders can liquidate their pooled credit line

Is Impossible All Lenders know how to monitor their pooled credit line and call function liquidate(). Suggest Open function liquidate() to Public and so Bot Developer can built bot to monitor pooled credit line and call function liquidate() when needed.

3) registerSelf() Incorrect @dev note

Risk Rating: Informational

Proof of Concept

https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/Verification/twitterVerifier.sol#L105

Recommended

The @dev note in registerSelf() mention "@dev only owner can register users" but the function actually allow Users to register themselve. Suggest change to "@dev users themselves can register themself".

4) updateVerification() Lack of Zero Address Check

Risk Rating: Low

Proof of Concept

https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/Verification/twitterVerifier.sol#L189-L195

Recommended

require(_verification != address(0), "Address Can't Be Zero")

5) updateSignerAddress() Lack of Zero Address Check

Risk Rating: Low

Proof of Concept

https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/Verification/twitterVerifier.sol#L203-L209

Recommended

require(_signerAddress != address(0), "Address Can't Be Zero")

6) Spelling Mistake "idenitifer"

Risk Rating: Informational

Proof of Concept

https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/LenderPool.sol#L143

Recommended

There are multiple Spelling Mistake "idenitifer" in LenderPool.sol and PooledCreditLine.sol.

The correct spelling should be "identifier". Suggest use Find & Replace to find "idenitifer" and replace as "identifier".

@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 Mar 31, 2022
code423n4 added a commit that referenced this issue Mar 31, 2022
@ritik99
Copy link
Collaborator

ritik99 commented Apr 13, 2022

  1. "Suggest function liquidate() Open to Public Rather than Lenders Only": Restricting liquidations to lenders actually allows for finer-grained settlements which is usually the case in real-world loans. This suggestion is valid (and something we considered too), but for now we would be sticking to the current implementation
  2. "registerSelf() Incorrect @dev note": What the dev note means is that the arguments necessary to call the function require the admin (_v, _r, _s) to provide them. We will improve the language for that comment to make it clear

Rest of the issues are relevant/acknowledged

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

2 participants