Sublime contest - cmichel's results

Handle

cmichel

Vulnerability details

The initialize function that initializes important contract state can be called by anyone.

See:

• PriceOracle.initialize
• PoolFactory.initialize
• Repayments.initialize
• CreditLine.initialize

Impact

The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract. In the best case for the victim, they notice it and have to redeploy their contract costing gas.

Recommended Mitigation Steps

Use the constructor to initialize non-proxied contracts. For initializing proxy contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify the transaction succeeded.

Handle

cmichel

Vulnerability details

The Extension contract uses the current balance (IPool.getBalanceDetails(msg.sender)) as the voting power. If this passes the threshold the extension is granted, doesn't matter that removeVotes removes the votes when transferring the tokens away from the voter.

If there's a secondary market for the pool tokens that allows flashloans, this means a lender (for example, the borrower could have approved itself before) can flashloan this amount, extend their repayment duration, and pay back the flashloan.

Recommended Mitigation Steps

Use a snapshot-based voting system like the Comp token does, or ungrant the extension in removeVotes if it doesn't reach the threshold anymore.

Handle

cmichel

Vulnerability details

Direct deposits to the savings account from the pool that don't have a strategy take the following path: Pool.depositCollateral -> Pool._deposit(..., _toSavingsAccount=true, strategy=poolConstants.poolSavingsStrategy) -> SavingsAccountUtils.directDeposit -> SavingsAccountUtils.directSavingsAccountDeposit -> SavingsAccount.deposit -> SavingsAccount._deposit -> SavingsAccount._depositToYield -> _strategy.lockTockens

While directSavingsAccountDeposit has a special case for _strategy == address(0), the _depositToYield function ignores it and reverts in the registry check or when calling lockTokens on the zero address.

function _depositToYield(
    uint256 _amount,
    address _token,
    address _strategy
) internal returns (uint256 _sharesReceived) {
    // @audit-info sometimes this is called with _strategy == 0 from `directSavingsAccountDeposit`
    require(IStrategyRegistry(strategyRegistry).registry(_strategy), 'SavingsAccount::deposit strategy do not exist');
    uint256 _ethValue;

    if (_token == address(0)) {
        _ethValue = _amount;
        require(msg.value == _amount, 'SavingsAccount::deposit ETH sent must be equal to amount');
    }
    // @audit-ok if strategy is attacker controlled, can mint billion shares. checks strategy here, but not everywhere? actually everywhere it gets increased
    _sharesReceived = IYield(_strategy).lockTokens{value: _ethValue}(msg.sender, _token, _amount);
}

One cannot deposit to the savings account if the pool.SavingsStrategy is zero.

Recommended Mitigation Steps

Handle the case where _strategy == 0 in _deposit or _depositToYield.

Handle

cmichel

Vulnerability details

When transferring pool tokens to oneself the Pool._beforeTokenTransfer overwrites the effectiveInterestWithdrawn of the user with a higher amount than expected. It uses the previous balance + the transfer amount instead of just the previous balance:

// @audit if from == to: overwrites with last _to statement => bug
lenders[_from].effectiveInterestWithdrawn = (_fromBalance.sub(_amount)).mul(_totalRepaidAmount).div(_totalSupply);
lenders[_to].effectiveInterestWithdrawn = (_toBalance.add(_amount)).mul(_totalRepaidAmount).div(_totalSupply);

Impact

The bug is not in the user's favor and would lead to them being able to withdraw fewer repayments in the future.

POC

• user calls Pool.transfer(from=user, to=user, amount=pool.balanceOf(user))
• pending repayments are withdrawn first by the _withdrawRepayment calls. (second one does not lead to a second withdrawal as the effectiveInterestWithdrawn is already increased in the first call)
• lenders[user].effectiveInterestWithdrawn is then set using 2 * userBalance.
• This has the effect that the user appears to have claimed twice as many repayments as their balance indicates already and they won't be able to claim anymore for a while.

Recommended Mitigation Steps

We still recommend fixing this bug, for example, by disallowing self-transfers.

Handle

cmichel

Vulnerability details

Certain ERC20 tokens make modifications to their ERC20's transfer or balanceOf functions. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

The CreditLine._depositCollateral function does not work well with fee-on-transfer tokens. An _amount is first transferred to this address and then a strategy is approved with the same amount. However, when the strategy tries to transfer this _amount from the pool, it will fail as the pool received less than _amount due to the fee.

Recommended Mitigation Steps

One possible mitigation is to measure the asset change right before and after the asset-transferring calls.

Handle

cmichel

Vulnerability details

The strategy contracts define an unlockShares function that must accept an asset parameter as the share token (yield token, aToken, cToken, etc.), otherwise, the code does not work. However, all comments say that asset is the address of the underlying token.

/**
  * @notice Used to unlock shares
  * @param asset the address of underlying token
  * @param amount the amount of shares to unlock
  * @return received amount of shares received
  **/
function unlockShares(address asset, uint256 amount) external override onlySavingsAccount nonReentrant returns (uint256) {
    if (amount == 0) {
        return 0;
    }
}

Recommended Mitigation Steps

Fix the comments for all unlockShares by saying asset is the share token, not the underlying token.

Handle

cmichel

Vulnerability details

The function computes uint256 _amount = balanceInShares[msg.sender][_token][_strategyList[i]]; but this value is never used and reassigned in the next line to _amount = IYield(_strategyList[i]).unlockTokens(_token, balanceInShares[msg.sender][_token][_strategyList[i]]);. The first line can therefore be removed.

Handle

cmichel

Vulnerability details

The if conditions in Pool.withdrawLiquidity are distinct conditions on the pool status. Therefore, else if is semantically equivalent but more gas efficient.

if (_loanStatus == LoanStatus.DEFAULTED || _loanStatus == LoanStatus.TERMINATED) {
    uint256 _totalAsset;
    if (poolConstants.borrowAsset != address(0)) {
        _totalAsset = IERC20(poolConstants.borrowAsset).balanceOf(address(this));
    } else {
        _totalAsset = address(this).balance;
    }
    //assuming their will be no tokens in pool in any case except liquidation (to be checked) or we should store the amount in liquidate()
    _toTransfer = _toTransfer.mul(_totalAsset).div(totalSupply());
}
// @audit gas: use else if, status fields are distinct, only one of the branches is (if ever) executed anyway
if (_loanStatus == LoanStatus.CANCELLED) {
    _toTransfer = _toTransfer.add(_toTransfer.mul(poolVariables.penaltyLiquidityAmount).div(totalSupply()));
}

if (_loanStatus == LoanStatus.CLOSED) {
    //transfer repayment
    _withdrawRepayment(msg.sender);
}