Sherlock contest - shw's results

Handle

shw

Vulnerability details

Impact

The Gov contract uses the SafeERC20 library to handle the transfer of tokens that are not compliant with the ERC20 specification. However, in line 300, the approve function is used instead of the safeApprove function. Tokens not compliant with the ERC20 specification could return false from the approve call to indicate the approval fails, while the calling contract would not notice the failure if the return value is not checked.

Proof of Concept

Referenced code: Gov.sol#L300

Recommended Mitigation Steps

Use safeApprove instead of approve, which reverts the transaction with a proper error message when the return value of approve is false. A better approach is to use safeIncreaseAllowance instead, which mitigates the multiple withdrawal attack on ERC20 tokens.

Handle

shw

Vulnerability details

Impact

The decreaseApproval method of SherXERC20 is considered unsafe since the allower has no trivial way to know whether the spender transferred any number of tokens from him before a decreaseApproval call is executed. Please refer to the following link for more details.

Proof of Concept

Referenced code: SherXERC20.sol#L79-L83

OpenZeppelin/openzeppelin-contracts - Method decreaseApproval in unsafe

Recommended Mitigation Steps

Consider using increaseAllowance and decreaseAllowance instead. Besides, increaseApproval and decreaseApproval are replaced in the OpenZeppelin library in this commit.

Handle

shw

Vulnerability details

Impact

The calcUnderlyingInStoredUSD() function of SherX should return calcUnderlyingInStoredUSD(getSherXBalance()) instead of calcUnderlyingInStoredUSD(sx20.balances[msg.sender]) since there could be SherX unallocated to the user at the time of the function call. A similar function, calcUnderlying(), calculates the user's underlying tokens based on the user's current balance plus the unallocated ones.

Proof of Concept

Referenced code: SherX.sol#L141

Recommended Mitigation Steps

Change sx20.balances[msg.sender] to getSherXBalance() at line 141.

Handle

shw

Vulnerability details

Impact

When withdrawing funds from the strategy, the PoolStrategy assumes that the exact amount of want tokens is withdrawn and increases the stake balance by amount. However, this may not always be true, depending on the strategy or the yield source of the strategy. The actual return number of tokens could be less than the provided parameter amount, thus resulting in an inaccurate stake balance. (For example, some vaults return only their total balance when the desired withdrawal amount exceeds it.)

Proof of Concept

Referenced code: PoolStrategy.sol#L79-L80

Recommended Mitigation Steps

Consider modifying the withdraw function of IStrategy to return a uint indicating the actual withdraw amount from the yield source. Take AaveV2 as an example, directly return the result from Aave (line 95), which is the actual withdraw amount according to Aave's documentation.

Handle

shw

Vulnerability details

Impact

The transferFrom function of SherXERC20 checks the _from address to be non-zero, while the _to address is not checked. Neither the _from address nor the _to address is checked for the' transfer' function.

Proof of Concept

Referenced code: SherXERC20.sol#L105 SherXERC20.sol#L94-L97

Recommended Mitigation Steps

Consider adding non-zero address checks for the _from and _to addresses in the internal _transfer function instead of the transfer or transferFrom functions.

Handle

shw

Vulnerability details

Impact

SafeMath library functions are not always used in arithmetic operations in the PoolBase contract, which could potentially cause integer underflow/overflows. Although in the reference lines of code, there are upper limits on the variables to ensure an integer underflow/overflow could not happen, using SafeMath is always a best practice, which prevents underflow/overflows completely (even if there were no assumptions on the variables) and increases code consistency as well.

Proof of Concept

Referenced code: PoolBase.sol#L136 PoolBase.sol#L325 PoolBase.sol#L344 PoolBase.sol#L362 PoolBase.sol#L364

Recommended Mitigation Steps

Consider using the SafeMath library functions in the referenced lines of code.

Handle

shw

Vulnerability details

Impact

A possible divide-by-zero error could happen in the getSherXPerBlock(uint256, IERC20) function of PoolBase when the totalSupply of lockToken and _lock are both 0.

Proof of Concept

Referenced code: PoolBase.sol#L215

Recommended Mitigation Steps

Check if baseData().lockToken.totalSupply().add(_lock) equals to 0 before line 214. If so, then return 0.

Handle

shw

Vulnerability details

Impact

Adding non-zero address checks on the following function's parameters can help ensure the ownership of contracts is not lost or the contracts do not need to be redeployed if any of them is provided as zero accidentally.

Proof of Concept

Referenced code: GovDev.sol#L19-L23 NativeLock.sol#L19 ForeignLock.sol#L20

Recommended Mitigation Steps

Consider adding non-zero address checks on the parameters.

Handle

shw

Vulnerability details

The getInitialUnstakeEntry function of PoolBase returns the first active unstaking entry of a staker, which requires the current block to be strictly before the last block in the unstaking window. However, the unstake function allows the current block to be exactly the same as the last block (same logic in unstakeWindowExpiry).

Proof of Concept

Referenced code: PoolBase.sol#L136 PoolBase.sol#L344 PoolBase.sol#L364

Recommended Mitigation Steps

Change the <= comparison at line 136 to < for consistency.

Handle

shw

Vulnerability details

Impact

Declaring state variables in the AaveV2 contract as constant or immutable (whose value is assigned only at the construction time) can save gas.

Proof of Concept

Can be declared as constant: AaveV2.sol#L24

Can be declared as immutable: AaveV2.sol#L26 AaveV2.sol#L29 AaveV2.sol#L31-L32

Recommended Mitigation Steps

Add a constant or immutable keyword in the referenced lines of code.

Handle

shw

Vulnerability details

Impact

A token cannot be reinitialized with a new lock token once it is set to a non-zero address. If the lock token needs to be changed (for example, because of implementation errors), the token must be removed and added again.

Proof of Concept

Referenced code: Gov.sol#L218-L227

Recommended Mitigation Steps

Consider removing the if condition at line 219 to allow the lock token to be reinitialized.

Handle

shw

Vulnerability details

Impact

In general, declaring dynamic function parameters as calldata instead of memory could save gas since the parameters are not copied into the memory but directly read from the call data.

Proof of Concept

Referenced code: Gov.sol#L138 Gov.sol#L165 Manager.sol#L46 Manager.sol#L74-L75 Manager.sol#L95-L97 Manager.sol#L142-L144 Manager.sol#L166 Manager.sol#L168 Manager.sol#L203-L206 Payout.sol#L110-L113 SherX.sol#L193-L194 SherXERC20.sol#L53

Recommended Mitigation Steps

Consider changing the memory keyword to calldata in the referenced functions. However, two of them require local variables to be rearranged or scoped to avoid "stack too deep" compiler errors. Please refer to this link for the detailed implementation.

Handle

shw

Vulnerability details

Impact

In the PoolStorage library, declaring the POOL_STORAGE_PREFIX constant with type bytes32, and change abi.encode ti abi.encodePacked at line 87 can save gas.

Proof of Concept

Referenced code: PoolStorage.sol#L14 PoolStorage.sol#L87

Related links: Change string to byteX if possible Solidity-Encode-Gas-Comparison

Recommended Mitigation Steps

See above

Handle

shw

Vulnerability details

Impact

A storage read cost more gas than a memory read. State variables that do not change during a loop can be stored in local variables and be read from memory multiple times to save gas.

Proof of Concept

Referenced code: LibPool.sol#L89 LibSherX.sol#L60 LibSherX.sol#L94 PoolBase.sol#L131 SherX.sol#L76 SherX.sol#L98 SherX.sol#L152 SherX.sol#L184 SherX.sol#L243 Gov.sol#L190

Recommended Mitigation Steps

For example, consider re-writing the harvestFor(address) function of SherX as follows:

function harvestFor(address _user) public override {
  GovStorage.Base storage gs = GovStorage.gs();
  uint256 len = gs.tokensStaker.length;
  for (uint256 i; i < len; i++) {
    PoolStorage.Base storage ps = PoolStorage.ps(gs.tokensStaker[i]);
    harvestFor(_user, ps.lockToken);
  }
}

Handle

shw

Vulnerability details

Impact

At line 185 of Payout, the deduction is calculated by excludeUsd / (curTotalUsdPool / totalSupply) instead of excludeUsd * totalSupply / curTotalUsdPool. However, the former one is consider less precise than the latter, and could cause a divide-by-zero error if curTotalUsdPool < totalSupply.

Proof of Concept

Referenced code: Payout.sol#L185

Recommended Mitigation Steps

Change the calculation to excludeUsd * totalSupply / curTotalUsdPool.

Handle

shw

Vulnerability details

Impact

The _accrueSherX function of LibSherX and the payOffDebtAll function of LibPool can be called multiple times in the same block (from different users and transactions). If the current block number is the same as the last-recorded one, it is possible to save gas by early returning at the beginning of the functions.

Proof of Concept

Referenced code: LibSherX.sol#L123-L141 LibPool.sol#L84-L95

Recommended Mitigation Steps

For example, consider re-writing _accrueSherX as follows:

function _accrueSherX(IERC20 _token, uint256 sherXPerBlock) private returns (uint256 sherX) {
  PoolStorage.Base storage ps = PoolStorage.ps(_token);
  if (block.number == ps.sherXLastAccrued) {
    return 0;
  }
  sherX = block.number.sub(ps.sherXLastAccrued).mul(sherXPerBlock).mul(ps.sherXWeight).div(
    uint16(-1)
  );
  // need to settle before return, as updating the sherxperlblock/weight
  // after it was 0 will result in a too big amount (accured will be < block.number)
  ps.sherXLastAccrued = uint40(block.number);
  if (address(_token) == address(this)) {
    ps.stakeBalance = ps.stakeBalance.add(sherX);
  } else {
    ps.unallocatedSherX = ps.unallocatedSherX.add(sherX);
    ps.sWeight = ps.sWeight.add(sherX);
  }
}