PoolTogether micro contest #1 - hickuphh3's results

Handle

hickuphh3

Vulnerability details

Impact

The logic of _requireYieldSource() is fine, but the variable isInvalidYieldSource within the function does the opposite of its intended purpose. The lines of interest are below.

isInvalidYieldSource = depositTokenAddress != address(0);
require(isInvalidYieldSource, "SwappableYieldSource/invalid-yield-source");

The intention is to check that depositTokenAddress is non-null, which makes the yield source valid (not invalid like the variable name suggests).

Recommended Mitigation Steps

Change isInvalidYieldSource to isValidYieldSource.

Handle

hickuphh3

Vulnerability details

Impact

The FundsTransferred() event in _transferFunds() will report a smaller amount than expected if currentBalance > _amount.

This would affect applications utilizing event logs like subgraphs.

Recommended Mitigation Steps

Update the event emission to emit FundsTransferred(_yieldSource, currentBalance);

Handle

hickuphh3

Vulnerability details

Impact

setYieldSource() changes the current yield source to a new yield source. It has similar functionality as swapYieldSource(), except that it doesn't transfer deposited funds from the current to the new one. However, it fails to check that it does not have any remaining deposited funds in the current yield source before the transfer.

It is highly recommended for this check to be in place so that funds aren't forgotten / unintentionally lost.

Recommended Mitigation Steps

Add a require check

require(yieldSource.balanceOfToken(address(this)); == 0, "SwappableYieldSource/existing-funds-in-current-yield-source") **before calling _setYieldSource()

Handle

hickuphh3

Vulnerability details

Impact

Assuming that depositToken of a yield source doesn't change, it would make sense to save its value as a storage variable in the contract as well, so that an external call to yieldSource to retrieve it can be avoided whenever it is needed.

Recommended Mitigation Steps

Define address public override depositToken; or IERC20Upgradeable public depositToken; which gets initialized in the initialize() function. The nice thing is that it also doesn't need to be updated when swapping sources because a requirement is that the new yield source must have the same deposit token.

As an optimization, since the _requireYieldSource() function already retrieves the depositToken address, it can return it so that its value need not be externally retrieved again in the initialize() function.

The depositToken() function can be removed if the former suggestion is implemented (ie. address public override depositToken).

Then, yieldSource.depositToken() can be replaced with depositToken where applicable (with appropriate casting).

A part of the former implementation is provided below.

address public override depositToken;

function initialize(...) {
	address depositTokenAddress = _requireYieldSource(_yieldSource);
	yieldSource = _yieldSource;
  depositToken = depositTokenAddress;
	...
	IERC20Upgradeable(depositTokenAddress).safeApprove(address(_yieldSource), type(uint256).max);
}

function _requireYieldSource(IYieldSource _yieldSource) internal view returns (address depositTokenAddress) {
	...
	(depositTokenAddress) = abi.decode(depositTokenAddressData, (address));
}

// function depositToken() can be removed
// yieldSource.depositToken() can be replaced with depositToken in other functions
// Example: _setYieldSource
function _setYieldSource(IYieldSource _newYieldSource) internal {
	_requireDifferentYieldSource(_newYieldSource);
	// Commented out check below should be shifted to inside _requireDifferentYieldSource()
	// Optimization: it can also return depositToken to avoid another SLOAD
	// similar to _requireYieldSource() above
	// require(_newYieldSource.depositToken() == depositToken, "SwappableYieldSource/different-deposit-token");

  yieldSource = _newYieldSource;
  IERC20Upgradeable(depositToken).safeApprove(address(_newYieldSource), type(uint256).max);

  emit SwappableYieldSourceSet(_newYieldSource);
}

Handle

hickuphh3

Vulnerability details

Impact

approveMax() and depositToken() are not called internally in the contract. Making these methods external would help reduce execution gas costs.

Handle

hickuphh3

Vulnerability details

Impact

The immutable mAsset is assigned to the immutable savings contract. Hence, we can avoid an external function call to the savings contract in the approveMax function by replacing it with mAsset.

Recommended Mitigation Steps

function approveMax() public {
	mAsset.safeApprove(address(savings), type(uint256).max);

	emit ApprovedMax(msg.sender);
}

Handle

hickuphh3

Vulnerability details

Impact

The number of solc runs used for contract compilation is 200. 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.ts, increase solc runs significantly. Contract sizes and thus deployment costs will increase, but functions will cost less gas to execute.

Handle

hickuphh3

Vulnerability details

Impact

Since mAsset is immutable and is initialized to a valid token address, a redundant extcodesize check can be avoided, just like how UniswapV3 does it.

Recommended Mitigation Steps

function mAssetBalance() private view returns (uint256) {
	(bool success, bytes memory data) =
		mAsset.staticcall(abi.encodeWithSelector(IERC20.balanceOf.selector, address(this)));
		require(success && data.length >= 32);
		return abi.decode(data, (uint256));
}

// Replace lines mAsset.balanceOf(address(this)) with mAssetBalance()
function redeemToken(uint256 mAssetAmount)
  external
  override
  nonReentrant
  returns (uint256 mAssetsActual)
{   
	uint256 mAssetBalanceBefore = mAssetBalance();

  uint256 creditsBurned = savings.redeemUnderlying(mAssetAmount);

  imBalances[msg.sender] -= creditsBurned;
  uint256 mAssetBalanceAfter = mAssetBalance();
  mAssetsActual = mAssetBalanceAfter - mAssetBalanceBefore;

  mAsset.safeTransfer(msg.sender, mAssetsActual);

  emit Redeemed(msg.sender, mAssetAmount, mAssetsActual);
}

Handle

hickuphh3

Vulnerability details

Impact

Revert messages tend to be long, like SwappableYieldSource/yield-source-token-transfer-not-allowed and SwappableYieldSource/yieldSource-not-zero-address. Reducing these messages to < 32 bytes will help save on deployment gas.

Recommended Mitigation Steps

Consider shortening the messages. One suggestion is to use error codes, like PT_001, PT_002 etc (similar to hardhat error codes). Alternatively, find ways to abbreviate error messages such that they are suggestive enough to make debugging easy, but are short as well.