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

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

QA Report #61

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

[L] LenderPool.sol#lend() Wrong event emitted

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

    function lend(uint256 _id, uint256 _amount) external nonReentrant {
        require(VERIFICATION.isUser(msg.sender, pooledCLConstants[_id].lenderVerifier), 'L1');
        require(block.timestamp < pooledCLConstants[_id].startTime, 'L2');

        uint256 _totalLent = totalSupply[_id];
        uint256 _maxLent = pooledCLConstants[_id].borrowLimit;
        require(_maxLent > _totalLent, 'L3');

        uint256 _amountToLend = _amount;
        if (_totalLent.add(_amount) > _maxLent) {
            _amountToLend = _maxLent.sub(_totalLent);
        }
        address _borrowAsset = pooledCLConstants[_id].borrowAsset;

        IERC20(_borrowAsset).safeTransferFrom(msg.sender, address(this), _amountToLend);
        _mint(msg.sender, _id, _amountToLend, '');

        emit Lend(_id, msg.sender, _amount);

        // If borrowLimit reached, accept CL
        if (_totalLent.add(_amountToLend) == _maxLent) {
            _accept(_id, _maxLent);
        }
    }

_amount should be changed to : _amountToLend.

[L] Wrong value for LimitsUpdated event parameter

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

    event LimitsUpdated(string indexed limitType, uint256 max, uint256 min);

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

    function updateBorrowLimitLimits(uint256 _min, uint256 _max) external onlyOwner {
        require(_min < _max || _min.mul(_max) == 0, 'UBLL1');
        require(!(borrowLimitLimits.min == _min && borrowLimitLimits.max == _max), 'UBLL2');
        borrowLimitLimits = Limits(_min, _max);
        emit LimitsUpdated('borrowLimit', _min, _max);
    }

Recommendation

Change to:

        emit LimitsUpdated('borrowLimit', _max, _min);

[N] Outdated compiler version

It's a best practice to use the latest compiler version.

The specified minimum compiler version is quite old. Older compilers might be susceptible to some bugs. We recommend changing the solidity version pragma to the latest version to enforce the use of an up to date compiler.

List of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo

[N] PooledCreditLine.sol LenderPool.sol should use the Upgradeable variant of OpenZeppelin Contracts

As per the OpenZeppelin docs:

If your contract is going to be deployed with upgradeability, such as using the OpenZeppelin Upgrades Plugins, you will need to use the Upgradeable variant of OpenZeppelin Contracts.

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

import '@openzeppelin/contracts/utils/ReentrancyGuard.sol';

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

import '@openzeppelin/contracts/utils/ReentrancyGuard.sol';

Recommendation

Change to:

import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";

[L] Precision loss due to div before mul

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

_maxPossible = _totalCollateralToken.mul(_ratioOfPrices).div(_collateralRatio).mul(SCALING_FACTOR).div(10**_decimals);

Can be changed to:

_maxPossible = _totalCollateralToken.mul(_ratioOfPrices).mul(SCALING_FACTOR).div(_collateralRatio).div(10**_decimals);

[N] Fee on transfer tokens are not supported

@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 12, 2022

All suggestions except second last ("[L] Precision loss due to div before mul") are valid. The operations have been defined in that sequence to prevent overflows.

The report is of high quality

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