Sublime contest - WatchPug's results

Lines of code

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

Vulnerability details

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

/**
    * @notice Function invoked when pooled credit line is terminated by admin
    * @dev only pooledCreditLineContract can invoke
    * @param _id identifier for the pooled credit line
    * @param _to address to which all the borrow tokens are transfered
    */
function terminate(uint256 _id, address _to) external override onlyPooledCreditLine nonReentrant {
    address _strategy = pooledCLConstants[_id].borrowAssetStrategy;
    address _borrowAsset = pooledCLConstants[_id].borrowAsset;
    uint256 _borrowedTokens = pooledCLConstants[_id].borrowLimit;

    uint256 _notBorrowed = _borrowedTokens.sub(POOLED_CREDIT_LINE.getPrincipal(_id));
    uint256 _notBorrowedInShares = IYield(_strategy).getSharesForTokens(_notBorrowed, _borrowAsset);
    uint256 _sharesHeld = pooledCLVariables[_id].sharesHeld;
    uint256 _totalInterestInShares = _sharesHeld.sub(_notBorrowedInShares);

    uint256 _principalWithdrawable = _notBorrowed.mul(totalSupply[_id]).div(_borrowedTokens);

    SAVINGS_ACCOUNT.withdrawShares(_borrowAsset, _strategy, _to, _principalWithdrawable.add(_totalInterestInShares), false);
    delete pooledCLConstants[_id];
    delete pooledCLVariables[_id];
}

LenderPool.sol#terminate() will be called when a pooled credit line is terminated by admin.

However, in the current implementation, a wrong value is used as the shares parameter in SAVINGS_ACCOUNT.withdrawShares().

Instead of using the amount of shares in terms of the _strategy's shares, it's actually using _principalWithdrawable which is in terms of _borrowAsset.

As a result, when the pricePerShare of _strategy's shares is higher than 1, terminate() a pooled credit line will withdraw more funds from SAVINGS_ACCOUNT than expected and all the benificatories of the SAVINGS_ACCOUNT will suffer a fund loss.

Recommendation

Change to:

/**
    * @notice Function invoked when pooled credit line is terminated by admin
    * @dev only pooledCreditLineContract can invoke
    * @param _id identifier for the pooled credit line
    * @param _to address to which all the borrow tokens are transfered
    */
function terminate(uint256 _id, address _to) external override onlyPooledCreditLine nonReentrant {
    address _strategy = pooledCLConstants[_id].borrowAssetStrategy;
    address _borrowAsset = pooledCLConstants[_id].borrowAsset;
    uint256 _borrowedTokens = pooledCLConstants[_id].borrowLimit;

    uint256 _notBorrowed = _borrowedTokens.sub(POOLED_CREDIT_LINE.getPrincipal(_id));
    uint256 _notBorrowedInShares = IYield(_strategy).getSharesForTokens(_notBorrowed, _borrowAsset);
    uint256 _sharesHeld = pooledCLVariables[_id].sharesHeld;
    uint256 _totalInterestInShares = _sharesHeld.sub(_notBorrowedInShares);

    uint256 _principalWithdrawableInShares = _notBorrowedInShares.mul(totalSupply[_id]).div(_borrowedTokens);

    SAVINGS_ACCOUNT.withdrawShares(_borrowAsset, _strategy, _to, _principalWithdrawableInShares.add(_totalInterestInShares), false);
    delete pooledCLConstants[_id];
    delete pooledCLVariables[_id];
}

Lines of code

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

Vulnerability details

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

function withdrawInterest(uint256 _id, address _lender) external nonReentrant {
    _withdrawInterest(_id, _lender);
}

function _withdrawInterest(uint256 _id, address _lender) internal {
    address _strategy = pooledCLConstants[_id].borrowAssetStrategy;
    address _borrowAsset = pooledCLConstants[_id].borrowAsset;

    (uint256 _interestToWithdraw, uint256 _interestSharesToWithdraw) = _calculateInterestToWithdraw(
        _id,
        _lender,
        _strategy,
        _borrowAsset
    );
    pooledCLVariables[_id].sharesHeld = pooledCLVariables[_id].sharesHeld.sub(_interestSharesToWithdraw);

    if (_interestToWithdraw != 0) {
        SAVINGS_ACCOUNT.withdraw(_borrowAsset, _strategy, _lender, _interestToWithdraw, false);
    }
    emit InterestWithdrawn(_id, _lender, _interestSharesToWithdraw);
}

withdrawInterest() at a certain time may not be in the best interest of the specific lender.

It's unconventional and can potentially cause leak of value for the lender. For example, the lender may still want to accrued more interest from the strategy.

Recommendation

Change to:

function withdrawInterest(uint256 _id, address _lender) external nonReentrant {
    require(msg.sender == _lender);
    _withdrawInterest(_id, _lender);
}

[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