Sherlock contest - hickuphh3's results

Handle

hickuphh3

Vulnerability details

Impact

In the payout() function, the deduction is calculated as follows.

uint256 deduction =
	excludeUsd.div(curTotalUsdPool.div(SherXERC20Storage.sx20().totalSupply)).div(10e17);

However, it is possible for curTotalUsdPool to be less than the SherX total supply if premiums and token prices are small enough. This causes the division to return zero, and so excludeUsd.div(0) will revert with SafeMath: division by zero.

Proof of Concept

In L230-231 of Payout.js, change to

[parseEther('0.9'), parseUnits('0.05', this.tokenC.dec)],
[parseEther('0.9'), parseUnits('0.05', this.tokenC.usdDec)],

Then run the test file. There would be 3 failing tests, of which the last 2 will throw the expected VM exception.

Recommended Mitigation Steps

1. The sherUsd calculation in _doSherX() should be consistent with subUsdPool.

   sherUsd = amounts[i].mul(sx.tokenUSD[tokens[i]]).div(10**18);

2. Change the deduction calculation to

   uint256 deduction =
   	excludeUsd.mul(SherXERC20Storage.sx20().totalSupply)).div(curTotalUsdPool);

It is unlikely for curTotalUsdPool to be zero (if it does, sherX tokens / premium payments are considered worthless), so this calculation would be valid.

Handle

hickuphh3

Vulnerability details

Impact

PoolStrategy trusts the implemented strategy ps.strategy (Eg. AaveV2.sol) to

• return the right amount for ps.strategy.balanceOf()
• have sent back the withdrawn funds when ps.strategy.withdraw() is called
• report the correct withdrawn amount when ps.strategy.withdrawAll() is called

While ps.strategy is assumed to have been scrutinised / its code verified before adding it as a strategy, and can therefore be trusted, consider minimising trust between PoolStrategy and ps.strategy, since strategies are themselves reliant on other protocols and therefore subject to external risk.

Recommended Mitigation Steps

• Verify the amount sent back to PoolStrategy for withdrawals instead
• The reliance on balanceOf() can be mitigated slightly by using a counter uint256 depositedAmount that increments / decrements upon deposits and withdrawals to the strategy respectively. This value can then be used in lieu of ps.strategy.balanceOf(). However, the downsides to this are that
  • this counter does not account for yield amounts from the strategy and
  • it increases complexity

A simple implementation to checking the withdrawal amounts is provided below.

function strategyWithdraw(uint256 _amount, IERC20 _token) external override {
		...
		uint256 balanceBefore = _token.balanceOf(address(this));
    ps.strategy.withdraw(_amount);
		require(balanceBefore.add(_amount) == _token.balanceOf(address(this)), "REASON");
    ps.stakeBalance = ps.stakeBalance.add(_amount);
  }

  function strategyWithdrawAll(IERC20 _token) external override {
    PoolStorage.Base storage ps = baseData();
    _enforceGovPool(ps);
    _enforceStrategy(ps);

		uint256 balanceBefore = _token.balanceOf(address(this));
		ps.strategy.withdrawAll();
		// alternatively, verify amount returned by withdrawAll() method
		uint256 amount = _token.balanceOf(address(this)).sub(balanceBefore);
    ps.stakeBalance = ps.stakeBalance.add(amount);
  }

Handle

hickuphh3

Vulnerability details

Impact

uint256 unallocatedSherX;, uint16 sherXWeight; and uint40 sherXLastAccrued; can be shifted below mapping(address => uint256) sWithdrawn; and uint256 sWeight; so that they can be packed into 1 storage slot with bool premiums;

While the deployment of PoolBase will increase by about 2.3k, execution costs of relevant methods will decrease, such as setProtocolPremiumAndTokenPrice() of about 19k and Gov.tokenRemove() of about 1.3k.

Recommended Mitigation Steps

mapping(address => uint256) sWithdrawn;
uint256 sWeight;
uint256 unallocatedSherX;
uint16 sherXWeight;
uint40 sherXLastAccrued;
bool premiums;

Handle

hickuphh3

Vulnerability details

Impact

protocolAdd() calls protocolUpdate() and protocolDepositAdd(), resulting in duplicate checks like onlyGovMain() and require(gs.protocolIsCovered[_protocol], 'NOT_COVERED');.

Consider separating the checks and actual updates of protocolUpdate() and protocolDepositAdd() so that they can be external functions, then have internal functions _protocolUpdate() and protocolDepositAdd().

Recommended Mitigation Steps

The gas reporter from running the tests indicates that protocolAdd() decreases by 798, but protocolUpdate() and protocolDepositAdd() increases slightly by 44 and 27 respectively. Depending on the expected frequency of the function calls, it will be a tradeoff to decide which function to optimize for (protocolAdd() vs protocolUpdate() and protocolDepositAdd()).

function protocolAdd(
  bytes32 _protocol,
  address _eoaProtocolAgent,
  address _eoaManager,
  IERC20[] memory _tokens
) external override onlyGovMain {
  require(_protocol != bytes32(0), 'ZERO_PROTOCOL');
  GovStorage.Base storage gs = GovStorage.gs();
  require(!gs.protocolIsCovered[_protocol], 'COVERED');
  gs.protocolIsCovered[_protocol] = true;

  _protocolUpdate(gs, _protocol, _eoaProtocolAgent, _eoaManager);
  _protocolDepositAdd(_protocol, _tokens);
}

function protocolUpdate(
  bytes32 _protocol,
  address _eoaProtocolAgent,
  address _eoaManager
) external override onlyGovMain {
  require(_protocol != bytes32(0), 'ZERO_PROTOCOL');
  GovStorage.Base storage gs = GovStorage.gs();
  require(gs.protocolIsCovered[_protocol], 'NOT_COVERED');
  _protocolUpdate(gs, _protocol, _eoaProtocolAgent, _eoaManager);
}

function protocolDepositAdd(bytes32 _protocol, IERC20[] memory _tokens)
  external
  override
  onlyGovMain
{
  require(_protocol != bytes32(0), 'ZERO_PROTOCOL');
  require(GovStorage.gs().protocolIsCovered[_protocol], 'NOT_COVERED');
  _protocolDepositAdd(_protocol, _tokens);
}

function _protocolUpdate(
  GovStorage.Base storage gs,
  bytes32 _protocol,
  address _eoaProtocolAgent,
  address _eoaManager
) internal {
  require(_eoaProtocolAgent != address(0), 'ZERO_AGENT');
  require(_eoaManager != address(0), 'ZERO_MANAGER');
  gs.protocolManagers[_protocol] = _eoaManager;
  gs.protocolAgents[_protocol] = _eoaProtocolAgent;
}

function _protocolDepositAdd(bytes32 _protocol, IERC20[] memory _tokens) internal {
  require(_tokens.length > 0, 'ZERO');
  for (uint256 i; i < _tokens.length; i++) {
    PoolStorage.Base storage ps = PoolStorage.ps(_tokens[i]);
    require(ps.premiums, 'INIT');
    require(!ps.isProtocol[_protocol], 'ALREADY_ADDED');

    ps.isProtocol[_protocol] = true;
    ps.protocols.push(_protocol);
  }
}

Gas Costs Before:

protocolAdd: 168058

protocolUpdate: 42210

protocolDepositAdd: 101736

Gas Costs After:

protocolAdd: 167260

protocolUpdate: 42254

protocolDepositAdd: 101763

Note that solc optimizer runs of 1 million was used.

Handle

hickuphh3

Vulnerability details

Impact

3 gas can be saved by avoiding the token assignment in protocolRemove().

Recommended Mitigation Steps

Change

IERC20 token = gs.tokensSherX[i];

PoolStorage.Base storage ps = PoolStorage.ps(token);

to

PoolStorage.Base storage ps = PoolStorage.ps(gs.tokensSherX[i]);

Handle

hickuphh3

Vulnerability details

Impact

The tokenInit() function has the following lines:

if (address(ps.lockToken) == address(_lock)) {
	require(!ps.stakes, 'STAKES_SET');
	ps.stakes = true;
	gs.tokensStaker.push(_token);
} else {
	revert('WRONG_LOCK');
}

but the if-else check can be simplified to a require statement that ps.lockToken should be equal to lock. In fact, the require statement can be the else case of the previous condition where ps.token has not been initialized since it will set it to be so (ie. ps.lockToken = _lock if address(ps.lockToken) == address(0)). This avoids a duplicate check that we know will be true and thus saves gas.

Recommended Mitigation Steps

function tokenInit(
	IERC20 _token,
	address _govPool,
	ILock _lock,
	bool _protocolPremium
) external override onlyGovMain {
	...
	if (address(_lock) != address(0)) {
		if (address(ps.lockToken) == address(0)) {
			...
			ps.lockToken = _lock;
		} else {
			require(address(ps.lockToken) == address(_lock), 'WRONG_LOCK');
		}
		require(!ps.stakes, 'STAKES_SET');
    ps.stakes = true;
    gs.tokensStaker.push(_token);
  }
	...
}

Gas reporter yields a cost reduction of ~100 gas.

Handle

hickuphh3

Vulnerability details

Impact

The number of solc runs used for contract compilation is 200. Given that the bytecode size restriction isn't an issue because of the usage of the diamond standard, this number can be bumped significantly to produce more gas efficient code (max value of 2**32-1).

More information can be found in the solidity docs.

Recommended Mitigation Steps

In hardhat.config.js, increase solc runs significantly. An arbitrary value of 1 million 1000000 seems to be good enough (anything more achieves the same result). Using the convenient hardhat-contract-sizer plugin, the largest contract (without any changes) is Gov.sol with a size of 15.67kb, well within the EIP170 limit.

Handle

hickuphh3

Vulnerability details

Impact

• tokens can be directly be assigned to gs.tokensSherX.
• Redundant zero Initialization of the array element amounts[i] = 0; in the else case of calcUnderlying() (L69-L71).

Recommended Mitigation Steps

function calcUnderlying(uint256 _amount)
	external
	view
returns (IERC20[] memory tokens, uint256[] memory amounts)
{
	GovStorage.Base storage gs = GovStorage.gs();

  tokens = gs.tokensSherX;
  amounts = new uint256[](gs.tokensSherX.length);

  uint256 total = getTotalSherX();

  for (uint256 i; i < gs.tokensSherX.length; i++) {
    IERC20 token = tokens[i];
    if (total > 0) {
      PoolStorage.Base storage ps = PoolStorage.ps(token);
      amounts[i] = ps.sherXUnderlying.add(LibPool.getTotalAccruedDebt(token)).mul(_amount).div(
        total
      );
    }
  }
}

Gas reporter reports a gas reduction of ~150 gas. Gas savings should scale with number of underlying collaterals.

Handle

hickuphh3

Vulnerability details

Impact

In updateData(),

if (sub > add) {
  usdPerBlock = usdPerBlock.sub(sub.sub(add).div(10**18));
} else {
  usdPerBlock = usdPerBlock.add(add.sub(sub).div(10**18));
}

we can calculate the difference between sub and add in both cases without SafeMath because we already know one is greater than the other.

Also, the logic can be made similar to the usdPool calculation since nothing changes in the case where sub == add.

The same safemath subtraction avoidance can be implemented for the usdPool calculation.

Finally, the variables sub and add are confusing (makes the code difficult to read because of safemath's add and sub). It is suggested to rename them to oldUsdPerBlock and newUsdPerBlock respectively.

Recommended Mitigation Steps

// If oldUsdPerBlock == newUsdPerBlock, nothing changes
if (oldUsdPerBlock > newUsdPerBlock) {
  usdPerBlock = usdPerBlock.sub((oldUsdPerBlock - newUsdPerBlock).div(10**18));
} else if (oldUsdPerBlock < newUsdPerBlock) {
  usdPerBlock = usdPerBlock.add((newUsdPerBlock - oldUsdPerBlock).div(10**18));
}

if (_newUsd > _oldUsd) {
	usdPool = usdPool.add((_newUsd - _oldUsd).mul(ps.sherXUnderlying).div(10**18));
} else if (_newUsd < _oldUsd) {
	usdPool = usdPool.sub((_oldUsd - _newUsd).mul(ps.sherXUnderlying).div(10**18));
}

Handle

hickuphh3

Vulnerability details

Impact

The only value from poolStorage in updateData() is the sherXUnderlying value. It is cheaper to pass this uint256 variable instead of the storage variable itself.

Recommended Mitigation Steps

Change PoolStorage.Base storage ps to uint256 sherXUnderlying, saves about ~160 gas.

Handle

hickuphh3

Vulnerability details

Impact

This is probably an oversight since SafeERC20 was imported and safeTransfer() was used for ERC20 token transfers. Nevertheless, note that approve() will fail for certain token implementations that do not return a boolean value (Eg. OMG and ADX tokens). Hence it is recommend to use safeApprove().

Recommended Mitigation Steps

Update to _token.safeApprove(address(_native), totalToken) in tokenUnload().