yAxis contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

_setupRole is used in function setSentinel. This is not a recommended approach. See the warning: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.1.0/contracts/access/AccessControl.sol#L183-L189

Recommended Mitigation Steps

OZ suggests using _grantRole when you need to assign roles not from the constructor.

Handle

pauliax

Vulnerability details

Impact

Please note that function setSentinel does not actually remove an existing sentinel but adds a new address with this role. I am not sure if this is intended behavior and you are aware of this, but the function name is a bit misleading in my opinion, so submitting this FYI. Anyway, roles can be managed directly if necessary (grantRole/revokeRole).

Handle

pauliax

Vulnerability details

Impact

Either the comment is misleading or the code is incorrect, as it says about the admin role but checks against sentinel:

  /// This function reverts if the caller does not have the admin role.
  function pauseAlchemist(address _toPause, bool _state) external onlySentinel
  /// This function reverts if the caller does not have the admin role.
  function setBlacklist(address _toBlacklist) external onlySentinel

Also, it is strange to see that the admin can setWhitelist, but sentinel can setBlacklist.

Recommended Mitigation Steps

Make the codebase and comments coherent.

Handle

pauliax

Vulnerability details

Impact

function setBorrowFee emits HarvestFeeUpdated event. This event based on its name is for harvest fee and is also emitted in setHarvestFee. Because it has only one parameter of uint, there is no way for an external agent that fetches these events to know which fee was updated by just looking at the event.

Recommended Mitigation Steps

Consider introducing a separate event for the borrow fee.

Handle

pauliax

Vulnerability details

Impact

Expressions can be extracted to constants as they do not change and do not need to be recalculated again and again:

  10**decimals
  uint256(-1)

Handle

pauliax

Vulnerability details

Impact

Assigned operations to constant variables are re-evalueted every time.

    bytes32 public constant ADMIN_ROLE = keccak256('ADMIN');
    bytes32 public constant SENTINEL_ROLE = keccak256('SENTINEL');

See https://github.com/ethereum/solidity/issues/9232

Change from 'constant' to 'immutable'.

Handle

pauliax

Vulnerability details

Impact

function deposit in vault adapters does not have any auth checks, thus anyone can call it directly on the strategy. This deposits tokens to the vault but will not issue the corresponding shares. Thus, by depositing directly these tokens will basically be sacrificed to the existing owners of shares. It is a very unlikely scenario in practice as it requires 2 steps:

1. Send tokens directly to the strategy contract.
2. Call deposit. As I mentioned, this is not very likely to occur in practice and maybe can be counted as an intended behavior, so submitting this FYI.

Recommended Mitigation Steps

deposit onlyAdmin

Handle

pauliax

Vulnerability details

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. An example of an error message that could be shortened:

  'Alchemist: not an emergency, not governance, and user does not have permission to recall funds from active vault'

Handle

pauliax

Vulnerability details

Impact

'immutable' greatly reduces gas costs. There are variables that do not change so they can be marked as immutable to greatly improve the gas costs. Examples of such variables are: vault and admin in YaxisVaultAdapter. vault admin, and decimals in YearnVaultAdapter.

Handle

pauliax

Vulnerability details

Impact

Consider removing unused code to save some gas. Examples are:

  address public _linkGasOracle;
  function _expectCaller