code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/main/core/solidity/contracts/core/Vault.sol#L210 https://github.com/code-423n4/2023-07-amphora/blob/main/core/solidity/contracts/core/AMPHClaimer.sol#L114 https://github.com/code-423n4/2023-07-amphora/blob/main/core/solidity/contracts/core/AMPHClaimer.sol#L169

Vulnerability details

Impact

Lack of the check for the case of the _cvxTotalRewards == 0 in the condition, which lead to the unexpected-behavior of the AmphClaimer#claimable() function.

Proof of Concept

Within the Vault#claimRewards(), the AmphClaimer#claimable() would be called like this: https://github.com/code-423n4/2023-07-amphora/blob/main/core/solidity/contracts/core/Vault.sol#L210

  /// @notice Claims available rewards from multiple tokens
  /// @dev    Transfers a percentage of the crv and cvx rewards to claim AMPH tokens
  /// @param _tokenAddresses The addresses of the erc20 tokens
  function claimRewards(address[] memory _tokenAddresses) external override onlyMinter {
    ...
    if (_totalCrvReward > 0 || _totalCvxReward > 0) {
      if (address(_amphClaimer) != address(0)) {
        // Approve amounts for it to be taken
        (uint256 _takenCVX, uint256 _takenCRV, uint256 _claimableAmph) =
          _amphClaimer.claimable(address(this), this.id(), _totalCvxReward, _totalCrvReward); /// @audit
          ...

Within the AmphClaimer#claimable(), the AMPHClaimer#_claimable() would be called like this: https://github.com/code-423n4/2023-07-amphora/blob/main/core/solidity/contracts/core/AMPHClaimer.sol#L114

  /// @notice Returns the claimable amount of AMPH given a CVX and CRV quantity
  /// @param _sender The address of the account claiming
  /// @param _vaultId The vault id that is claiming
  /// @param _cvxTotalRewards The max CVX amount to exchange from the sender
  /// @param _crvTotalRewards The max CVR amount to exchange from the sender
  /// @return _cvxAmountToSend The amount of CVX the user will have to send
  /// @return _crvAmountToSend The amount of CRV the user will have to send
  /// @return _claimableAmph The amount of AMPH that would be received by the beneficiary
  function claimable(
    address _sender,
    uint96 _vaultId,
    uint256 _cvxTotalRewards,
    uint256 _crvTotalRewards
  ) external view override returns (uint256 _cvxAmountToSend, uint256 _crvAmountToSend, uint256 _claimableAmph) {
    (_cvxAmountToSend, _crvAmountToSend, _claimableAmph) =
      _claimable(_sender, _vaultId, _cvxTotalRewards, _crvTotalRewards); /// @audit
  }

Within the AMPHClaimer#_claimable(), if amounts are zero, or AMPH balance is zero, all zeros would simply be returned like this: https://github.com/code-423n4/2023-07-amphora/blob/main/core/solidity/contracts/core/AMPHClaimer.sol#L169

  /// @dev Doesn't revert but returns 0 so the vault contract doesn't revert on calling the claim function
  /// @dev Returns the claimable amount of AMPH, also the CVX and CRV the contract will take from the user
  function _claimable(
    address _sender,
    uint96 _vaultId,
    uint256 _cvxTotalRewards,
    uint256 _crvTotalRewards
  ) internal view returns (uint256 _cvxAmountToSend, uint256 _crvAmountToSend, uint256 _claimableAmph) {
    if (_sender != vaultController.vaultIdVaultAddress(_vaultId)) return (0, 0, 0);

    uint256 _amphBalance = AMPH.balanceOf(address(this));

    // if amounts are zero, or AMPH balance is zero simply return all zeros
    if (_crvTotalRewards == 0 || _amphBalance == 0) return (0, 0, 0);  /// @audit
    ...

Within the condition at the line AMPHClaimer.sol#L169 in the AMPHClaimer#_claimable() above, the following cases is supposed to be checked:

_crvTotalRewards == 0
_cvxTotalRewards == 0
_amphBalance == 0

However, within the condition at the line AMPHClaimer.sol#L169 in the AMPHClaimer#_claimable() above, there is no check for the case of the _cvxTotalRewards == 0.

Although all zeros are supposed to be returned if amounts are zero or AMPH balance like this (which is written in the NatSpec), the AMPHClaimer#_claimable() does not return all zeros.

// if amounts are zero, or AMPH balance is zero simply return all zeros

Thus, this lead to the unexpected-behavior of the AmphClaimer#claimable() function.

Tools Used

Foundry

Recommended Mitigation Steps

Within the AMPHClaimer#_claimable(), consider adding a check for the case of the _cvxTotalRewards == 0 like this:

  function _claimable(
    address _sender,
    uint96 _vaultId,
    uint256 _cvxTotalRewards,
    uint256 _crvTotalRewards
  ) internal view returns (uint256 _cvxAmountToSend, uint256 _crvAmountToSend, uint256 _claimableAmph) {
    if (_sender != vaultController.vaultIdVaultAddress(_vaultId)) return (0, 0, 0);

    uint256 _amphBalance = AMPH.balanceOf(address(this));

    // if amounts are zero, or AMPH balance is zero simply return all zeros
+   if (_crvTotalRewards == 0 || _cvxTotalRewards == 0 || _amphBalance == 0) return (0, 0, 0);
-   if (_crvTotalRewards == 0 || _amphBalance == 0) return (0, 0, 0);
    ...

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-sponsor commented 1 year ago

0xShaito marked the issue as disagree with severity

0xShaito commented 1 year ago

It would still return 0. Just a tiny bit more gas usage at max. CVX rewards from convex are dependant on CRV rewards as the protocol mints CVX prorrata to the CRV rewarded

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)