Wild Credit contest - greiart's results

Handle

greiart

Vulnerability details

Impact

supplyInterest is split between the account and the system, ie. supplyInterest = newSupplyAccount + newSupplySystem. Hence, an additional call to the _systemRate can be avoided in the calculation of newSupplySystem, as well as potential rounding errors

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/LendingPair.sol#L401-L403

Recommended Mitigation Steps

L403 can be changed to newSupplySystem = supplyInterest - newSupplyAccount;

Handle

greiart

Vulnerability details

Impact

Will help prevent erraneous minObservations values from being set (ie. > 65535) by the owner without needing checks. Otherwise, the isPoolValid will always return false, causing reverts in calling tokenPrice and addPool functions (and other functions calling these).

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/UniswapV3Oracle.sol#L25

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/UniswapV3Oracle.sol#L101

Proof Of Concept

The maximum number of observations available is 65535 (see https://github.com/Uniswap/uniswap-v3-core/blob/main/contracts/UniswapV3Pool.sol#L39), which is equivalent to type(uint16).max.

Hence,

• uint public minObservations can be reduced to uint16 public minObservations.
• (, , , , uint observationSlots , ,) = IUniswapV3Pool(poolAddress).slot0(); becomes (, , , , uint16 observationSlots , ,) = IUniswapV3Pool(poolAddress).slot0();

Handle

greiart

Vulnerability details

Impact

The address casting of _token in lpToken[address(_token)] can be removed in the withdrawAll(), _withdraw() and _borrow() functions, since _token is already an address in these functions.

In other words, lpToken[address(_token)] → lpToken[token]

Handle

greiart

Vulnerability details

Impact

The _checkMinReserve() call in withdrawBorrowETH() function is redundant because it is already called in the sub-function _wethWithdrawTo() above it.

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/LendingPair.sol#L111

Recommended Mitigation Steps

L111 can be removed.

Handle

greiart

Vulnerability details

Impact

rate is of type uint so it can never be a negative value. Since MIN_RATE is already 0, this condition will always return rate

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/InterestRateModel.sol#L37

Proof Of Concept

if (utilization < TARGET_UTILIZATION) {
		return LOW_RATE * utilization / 100e18;
} else {
	  utilization = 100e18 * ( debt - (supply * TARGET_UTILIZATION / 100e18) ) / (supply * (100e18 - TARGET_UTILIZATION) / 100e18);
	  utilization = Math.min(utilization, 100e18);
	  return LOW_RATE + (HIGH_RATE - LOW_RATE) * utilization / 100e18;
}

Handle

greiart

Vulnerability details

Impact

It will save quite a bit of gas to cache tokenA and tokenB decimals value as uint128 so that both values can be stored in a single storage slot and retrieved, to avoid making the decimals() calls.

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/LendingPair.sol

Proof Of Concept

struct TokenDecimals {
	uint128 decimalsA;
	uint128 decimalsB;
}

TokenDecimals tokenDecimals;

function initialize(
    address _lpTokenMaster,
    address _controller,
    IERC20 _tokenA,
    IERC20 _tokenB
  ) external {
    require(address(tokenA) == address(0), "LendingPair: already initialized");
    require(address(_tokenA) != address(0) && address(_tokenB) != address(0), "LendingPair: cannot be ZERO address");
		
    controller = IController(_controller);
    tokenA = address(_tokenA);
    tokenB = address(_tokenB);
		uint256 _decimalsA = _tokenA.decimals();
		uint256 _decimalsB = _tokenB.decimals();
		require(_decimalsA > 0 && _decimalsA < type(uint128).max, 'invalid token decimals');
		require(_decimalsB > 0 && _decimalsB < type(uint128).max, 'invalid token decimals');
		tokenDecimals = TokenDecimals({
			decimalsA: uint128(_decimalsA),
			decimalsB: uint128(_decimalsB)
		});
    lastBlockAccrued = block.number;

    lpToken[tokenA] = _createLpToken(_lpTokenMaster);
    lpToken[tokenB] = _createLpToken(_lpTokenMaster);
  }

// note that both _fromToken and _toToken have to be tokenA and tokenB
// though each can be either token (eg. _fromToken = tokenA or tokenB)
// currently, all functions of this contract that call this function
// either perform the required validation checks
// or explicitly use tokenA and tokenB as the input arguments
function _convertTokenValues(
    address _fromToken,
    address _toToken,
    uint    _inputAmount
  ) internal view returns(uint) {
		// 1 sload for gas optimization
		TokenDecimals memory _tokenDecimals = tokenDecimals;
    uint priceFrom = controller.tokenPrice(_fromToken) * 1e18;
    uint priceTo   = controller.tokenPrice(_toToken)   * 1e18;
		if (_fromToken == tokenA) {
			priceFrom = priceFrom / 10 ** _tokenDecimals.decimalsA;
			priceTo = priceTo / 10 ** _tokenDecimals.decimalsB;
		} else {
			priceFrom = priceFrom / 10 ** _tokenDecimals.decimalsB;
			priceTo = priceTo / 10 ** _tokenDecimals.decimalsA;
		}
		return _inputAmount * priceFrom / priceTo;
}

Handle

greiart

Vulnerability details

Impact

Swapping the order of the calculation of supplyOutput and supplyBurn helps reduce 1 add operation. Also, this results in callerFee being used only once, so it would be better to do the calculation directly in supplyOutput. Adding a comment will mitigate the slight loss in code readability.

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/LendingPair.sol#L270-L273

Recommended Mitigation Steps

uint systemFee    = supplyDebt * controller.liqFeeSystem(_repayToken) / 100e18;
// supplyOutput = supplyDebt + callerFee
// callerFee = supplyDebt * controller.liqFeeCaller(_repayToken) / 100e18;
// alternatively, the calculation can be
// supplyOutput = (supplyDebt * (100e18 + controller.liqFeeCaller(_repayToken)) / 100e18;
uint supplyOutput = supplyDebt + supplyDebt * controller.liqFeeCaller(_repayToken) / 100e18;
uint supplyBurn   = supplyOutput + systemFee;

Handle

greiart

Vulnerability details

Impact

Reduce the need (and thus gas costs) for unnecessary logic to fetch the underlying token address from the pair contract by saving it as a variable upon contract initialisation.

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/LPTokenMaster.sol#L64-L66

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/LPTokenMaster.sol#L83-L86

Proof of Concept

Include an address _underlyingToken in the initialize() function and save it as address public underlyingToken. This replaces the underlying() function.

address public underlyingToken;

function initialize(address _underlyingToken) external {
    require(initialized != true, "LPToken: already intialized");
    owner = msg.sender;
		underlyingToken = _underlyingToken;
    initialized = true;
}

// L83-86
require(
      pair.borrowBalance(_recipient, underlyingToken, underlyingToken) == 0,
      "LendingPair: cannot deposit borrowed token"
);

The createLpToken() in LendingPair.sol should then be modified to include the underlying token address.

Handle

greiart

Vulnerability details

Impact

Currently, pid starts from 0 (L107), and a boolean flag added (L32) is used to prevent adding duplicates. If pid starts from 1 instead, then pid = 0 can become check for a un-initiated pool, which drops the need for the boolean flag, saving storage space.

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/RewardDistribution.sol#L107

Proof of Concept

// Note that struct PoolPosition is reduced to just uint pid
// Pair[token][isSupply] supply = true, borrow = false
mapping (address => mapping (address => mapping (bool => uint))) public pidByPairToken;

function addPool(
		address _pair,
    address _token,
    bool    _isSupply,
    uint    _points
) external onlyOwner {
  require(
      pidByPairToken[_pair][_token][_isSupply] == 0,
      "RewardDistribution: already added"
   );
...
	pidByPairToken[_pair][_token][_isSupply] = PoolPosition({
      pid: pools.length,
      added: true
    });
...
}

function _getPid(address _pair, address _token, bool _isSupply) internal view returns(uint _pid) {
		_pid = pidByPairToken[_pair][_token][_isSupply];
		require(_pid > 0, "RewardDistribution: invalid pool");
    return _pid;
		// note that this function implementation can simply be reduced to 
		// return pidByPairToken[_pair][_token][_isSupply];
		// because the require check is redundant due to how
		// the internal functions are called
}

function _poolExists(address _pair, address _token, bool _isSupply) internal view returns(bool) {
    return (pidByPairToken[_pair][_token][_isSupply] > 0);
}

Handle

greiart

Vulnerability details

Impact

In the pendingSupplyReward(), pendingBorrowReward() and _distributeReward() functions, the _poolExists() function is called prior to _getPid(), resulting in a duplicate check for an existing pool. The only exception for when _getPid() is called without _poolExists() is in the _getPool() method. However, note that _getPool() is called after the _poolExists() function as well. Hence, the boolean check in _getPid() is redundant and can be removed.

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/RewardDistribution.sol#L143

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/RewardDistribution.sol#L151

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/RewardDistribution.sol#L196

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/RewardDistribution.sol#L252

Recommended Mitigation Steps

L247 can be removed.

Handle

greiart

Vulnerability details

Impact

A repeated call to pidByPairToken[_pair][_token][_isSupply] can be avoided since it is stored in poolPosition. Simply return poolPosition.pid.

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/RewardDistribution.sol#L245-L250

Proof of Concept

function _getPid(address _pair, address _token, bool _isSupply) internal view returns(uint) {
    PoolPosition memory poolPosition = pidByPairToken[_pair][_token][_isSupply];
    require(poolPosition.added, "RewardDistribution: invalid pool");

    return poolPosition.pid;
}

Handle

greiart

Vulnerability details

Impact

In these 4 functions, _poolExists is called, followed by either _getPid or _getPool, both of which calls pidByPairToken[_pair][_token][_isSupply]. This can be optimized by removing _getPid entirely and reuse the same struct stored in memory for getting Pid or pool. An example is provided below.

Referenced Codelines

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/RewardDistribution.sol#L143

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/RewardDistribution.sol#L151

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/RewardDistribution.sol#L196

https://github.com/code-423n4/2021-07-wildcredit/blob/main/contracts/RewardDistribution.sol#L210

Proof of Concept

function _distributeReward(address _account, address _pair, address _token, bool _isSupply) internal {
		PoolPosition memory poolPosition = pidByPairToken[_pair][_token][_isSupply];
    if (poolPosition.added) {

      uint pid = poolPosition.pid;

      accruePool(pid);
      _transferReward(_account, _pendingAccountReward(pid, _account));

      Pool memory pool = _getPool(pid);
      rewardSnapshot[pid][_account] = pool.accRewardsPerToken;
    }
}