code-423n4 / 2022-03-sublime-findings

0 stars 0 forks source link

QA Report #25

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/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".

ritik99 commented 2 years ago
  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