search

sherlock-audit / 2023-01-derby-judging

4 stars 1 forks source link

hyh - Deposit allows for stealing underlying tokens from TruefiProvider, IdleProvider, YearnProvider, BetaProvider, CompoundProvider and AaveProvider balances #411

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

hyh

high

Deposit allows for stealing underlying tokens from TruefiProvider, IdleProvider, YearnProvider, BetaProvider, CompoundProvider and AaveProvider balances

Summary

TruefiProvider, IdleProvider, YearnProvider deposit() accept both underlying and yield bearing tokens from user and allow for stealing all underlying tokens from contract balances whenever there is a real yield bearing token for it.

Vulnerability Detail

Consider the following, for TruefiProvider case:

Attacker needs to supply fake pre-cooked _uToken, real _tToken, and _amount = IERC20(real_uToken).balanceOf(address(this)).

Fake _uToken will report exactly _amount balance increase after safeTransferFrom() call.

TruefiProvider will join real _tToken with all balance of its own, and send all the proceeds to msg.sender.

Impact

Any underlying tokens for which there is a market can be stolen from Provider balance.

Even if all these contracts aren't supposed to hold balances, there are a spectre of cases when they end up possessing some meaningful funds (accumulated residuals, additional rewards supplied from the markets, user operational mistakes), which are attributed to protocol users, but can be stolen this way.

Code Snippet

TruefiProvider's deposit():

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/TruefiProvider.sol#L19-L41

  function deposit(
    uint256 _amount,
    address _tToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(address(this));

    IERC20(_uToken).safeTransferFrom(msg.sender, address(this), _amount);
    IERC20(_uToken).safeIncreaseAllowance(_tToken, _amount);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(address(this));
    require((balanceAfter - balanceBefore - _amount) == 0, "Error Deposit: under/overflow");

    uint256 tTokenBefore = ITruefi(_tToken).balanceOf(address(this));
    ITruefi(_tToken).join(_amount);
    uint256 tTokenAfter = ITruefi(_tToken).balanceOf(address(this));

    uint tTokensReceived = tTokenAfter - tTokenBefore;

    ITruefi(_tToken).transfer(msg.sender, tTokensReceived);

    return tTokensReceived;
  }

Similarly, IdleProvider's deposit() also allows for stealing balance of any token for which there exists Idle market:

Attacker needs to supply fake pre-cooked _uToken, real _iToken, and _amount = IERC20(real_uToken).balanceOf(address(this)).

Fake _uToken will report exactly _amount balance increase after safeTransferFrom() call.

IdleProvider will mint real _iToken with all real_uToken balance of its own (_amount), and send all the proceeds to msg.sender:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/IdleProvider.sol#L20-L42

  function deposit(
    uint256 _amount,
    address _iToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(address(this));

    IERC20(_uToken).safeTransferFrom(msg.sender, address(this), _amount);
    IERC20(_uToken).safeIncreaseAllowance(_iToken, _amount);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(address(this));
    require((balanceAfter - balanceBefore - _amount) == 0, "Error Deposit: under/overflow");

    uint256 tTokenBefore = IIdle(_iToken).balanceOf(address(this));
    // expensive mint
    IIdle(_iToken).mintIdleToken(_amount, true, address(0));
    uint256 tTokenAfter = IIdle(_iToken).balanceOf(address(this));

    uint tTokensReceived = tTokenAfter - tTokenBefore;
    IIdle(_iToken).transfer(msg.sender, tTokensReceived);

    return tTokensReceived;
  }

Similarly, YearnProvider's deposit() also allows for stealing balance of any token for which there exists Yearn market:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/YearnProvider.sol#L19-L36

  function deposit(
    uint256 _amount,
    address _yToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(address(this));

    IERC20(_uToken).safeTransferFrom(msg.sender, address(this), _amount);
    IERC20(_uToken).safeIncreaseAllowance(_yToken, _amount);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(address(this));
    require((balanceAfter - balanceBefore - _amount) == 0, "Error Deposit: under/overflow");

    uint256 yTokenReceived = IYearn(_yToken).deposit(_amount);
    IYearn(_yToken).transfer(msg.sender, yTokenReceived);

    return yTokenReceived;
  }

Same with BetaProvider deposit(), fake _uToken can just report current real underlying token balance of BetaProvider and real _bToken minted from that is sent to msg.sender:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/BetaProvider.sol#L19-L40

  function deposit(
    uint256 _amount,
    address _bToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(address(this));

    IERC20(_uToken).safeTransferFrom(msg.sender, address(this), _amount);
    IERC20(_uToken).safeIncreaseAllowance(_bToken, _amount);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(address(this));
    require((balanceAfter - balanceBefore - _amount) == 0, "Error Deposit: under/overflow");

    uint256 tTokenBefore = IBeta(_bToken).balanceOf(address(this));
    IBeta(_bToken).mint(address(this), _amount);
    uint256 tTokenAfter = IBeta(_bToken).balanceOf(address(this));

    uint tTokensReceived = tTokenAfter - tTokenBefore;
    IBeta(_bToken).transfer(msg.sender, tTokensReceived);

    return tTokensReceived;
  }

Same with CompoundProvider deposit(), fake _uToken can just report current real underlying token balance and real _cToken minted from that is send to msg.sender:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/CompoundProvider.sol#L27-L48

  function deposit(
    uint256 _amount,
    address _cToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(address(this));

    IERC20(_uToken).safeTransferFrom(msg.sender, address(this), _amount);
    IERC20(_uToken).safeIncreaseAllowance(_cToken, _amount);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(address(this));
    require((balanceAfter - balanceBefore - _amount) == 0, "Error Deposit: under/overflow");

    uint256 cTokenBefore = ICToken(_cToken).balanceOf(address(this));
    require(ICToken(_cToken).mint(_amount) == 0, "Error minting Compound");
    uint256 cTokenAfter = ICToken(_cToken).balanceOf(address(this));

    uint cTokensReceived = cTokenAfter - cTokenBefore;
    ICToken(_cToken).transfer(msg.sender, cTokensReceived);

    return cTokensReceived;
  }

Same with AaveProvider deposit(), fake _uToken reports current real underlying token balance of AaveProvider, real _aToken deposits it on behalf of msg.sender:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/AaveProvider.sol#L20-L41

  function deposit(
    uint256 _amount,
    address _aToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(address(this));

    IERC20(_uToken).safeTransferFrom(msg.sender, address(this), _amount);
    IERC20(_uToken).safeIncreaseAllowance(address(IAToken(_aToken).POOL()), _amount);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(address(this));
    require((balanceAfter - balanceBefore - _amount) == 0, "Error Deposit: under/overflow");

    IALendingPool(IAToken(_aToken).POOL()).deposit(
      IAToken(_aToken).UNDERLYING_ASSET_ADDRESS(),
      _amount,
      msg.sender,
      0
    );

    return _amount;
  }

Tool used

Manual Review

Recommendation

One way is maintaining a whitelist mapping {underlying token -> yield bearing token -> acceptance flag}. The flag for the pair used in a call is then required to proceed.

Also, a balance check for token that is sent to a user can be useful: for attacker to benefit the token that is sent to them has to be real, so another approach is controlling its balance of the contract before and after the operation, and require that no loss of the initial balance took place.

A version of that is implemented in CompoundProvider's withdraw(), where real token balance ends up controlled, as an example:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/CompoundProvider.sol#L56-L84

  function withdraw(
    uint256 _amount,
    address _cToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(msg.sender);

    uint256 balanceBeforeRedeem = IERC20(_uToken).balanceOf(address(this));

    require(
      ICToken(_cToken).transferFrom(msg.sender, address(this), _amount) == true,
      "Error: transferFrom"
    );
    // Compound redeem: 0 on success, otherwise an Error code
    require(ICToken(_cToken).redeem(_amount) == 0, "Error: compound redeem");

    uint256 balanceAfterRedeem = IERC20(_uToken).balanceOf(address(this));
    uint256 uTokensReceived = balanceAfterRedeem - balanceBeforeRedeem;

    IERC20(_uToken).safeTransfer(msg.sender, uTokensReceived);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(msg.sender);
    require(
      (balanceAfter - balanceBefore - uTokensReceived) == 0,
      "Error Withdraw: under/overflow"
    );

    return uTokensReceived;
  }
mister0y commented 1 year ago

no real issue cause provider never has a balance of the underlying

hrishibhat commented 1 year ago

Given that the providers can hold some funds through secondary rewards or other operations mistakes, loss of these funds is not considered a valid high/medium based on Sherlock rules. Considering this issue as low

dmitriia commented 1 year ago

Escalate for 10 USDC The only requirements for token to be stolen is that it should have the market for itself. I.e. it can be a reward token that is distributed to Provider that performs the deposits, but there should be a market for it in the same Provider.

This condition is actually satisfied in Beta Finance case.

There are BETA rewards distributed to stablecoin markets on mainnet:

https://app.betafinance.org/

The major part of yield there is BETA rewards, say it is 0.01% USDC + 2.87% BETA APY for USDC market:

https://app.betafinance.org/deposit/0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48

And there is market for BETA token:

https://app.betafinance.org/deposit/0xbe1a001fe942f96eea22ba08783140b9dcc09d28

As mentioned in 360 comments, there is a shortcoming of the current design, that treats Vault as the destination of reward tokens, while it is Provider address interacting with the markets and Vault address has literally no link with the pools, so if the additional non-underlying rewards are due, they be due to the Provider's address.

It is assumed that govToken should end up being on the Vault balance:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Vault.sol#L404-L419

  function claimTokens() public {
    uint256 latestID = controller.latestProtocolId(vaultNumber);
    for (uint i = 0; i < latestID; i++) {
      if (currentAllocations[i] == 0) continue;
>>    bool claim = controller.claim(vaultNumber, i);
      if (claim) {
        address govToken = controller.getGovToken(vaultNumber, i);
>>      uint256 tokenBalance = IERC20(govToken).balanceOf(address(this));
        Swap.swapTokensMulti(
          Swap.SwapInOut(tokenBalance, govToken, address(vaultCurrency)),
          controller.getUniswapParams(),
          false
        );
      }
    }
  }

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Controller.sol#L57-L70

  function claim(
    uint256 _vaultNumber,
    uint256 _protocolNumber
  ) external override onlyVault returns (bool) {
    if (claimable[protocolInfo[_vaultNumber][_protocolNumber].LPToken]) {
      return
>>      IProvider(protocolInfo[_vaultNumber][_protocolNumber].provider).claim(
          protocolInfo[_vaultNumber][_protocolNumber].LPToken,
>>        msg.sender  // @audit it's Vault
        );
    } else {
      return false;
    }
  }

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/CompoundProvider.sol#L131-L139

  /// @notice Claims/harvest COMP tokens from the Comptroller
  /// @param _cToken Address of protocol LP Token eg cUSDC
  function claim(address _cToken, address _claimer) external override returns (bool) {
    address[] memory cTokens = new address[](1);
    cTokens[0] = _cToken;
>>  comptroller.claimComp(_claimer, cTokens);

    return true;
  }

But it is BetaProvider address interacts with Beta markets:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/BetaProvider.sol#L19-L40

  function deposit(
    uint256 _amount,
    address _bToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(address(this));

>>  IERC20(_uToken).safeTransferFrom(msg.sender, address(this), _amount);
    IERC20(_uToken).safeIncreaseAllowance(_bToken, _amount);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(address(this));
    require((balanceAfter - balanceBefore - _amount) == 0, "Error Deposit: under/overflow");

    uint256 tTokenBefore = IBeta(_bToken).balanceOf(address(this));
>>  IBeta(_bToken).mint(address(this), _amount);
    uint256 tTokenAfter = IBeta(_bToken).balanceOf(address(this));

    uint tTokensReceived = tTokenAfter - tTokenBefore;
    IBeta(_bToken).transfer(msg.sender, tTokensReceived);

    return tTokensReceived;
  }

And BetaProvider can have BETA part of the yield as the result of withdrawal:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/BetaProvider.sol#L48-L75

  function withdraw(
    uint256 _amount,
    address _bToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(msg.sender);

    uint256 balanceBeforeRedeem = IERC20(_uToken).balanceOf(address(this));

    require(
      IBeta(_bToken).transferFrom(msg.sender, address(this), _amount) == true,
      "Error: transferFrom"
    );
>>  IBeta(_bToken).burn(address(this), _amount);

    uint256 balanceAfterRedeem = IERC20(_uToken).balanceOf(address(this));
    uint256 uTokensReceived = balanceAfterRedeem - balanceBeforeRedeem;

    IERC20(_uToken).safeTransfer(msg.sender, uTokensReceived);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(msg.sender);
    require(
      (balanceAfter - balanceBefore - uTokensReceived) == 0,
      "Error Withdraw: under/overflow"
    );

    return uTokensReceived;
  }

This way the reward portion of the yield for the whole Vault can be stolen fully, as it will sit on the BetaProvider balance in the form of BETA tokens and there is a market for these tokens in Beta Finance, so this looks like valid high severity and needs to be fixed.

sherlock-admin commented 1 year ago

Escalate for 10 USDC The only requirements for token to be stolen is that it should have the market for itself. I.e. it can be a reward token that is distributed to Provider that performs the deposits, but there should be a market for it in the same Provider.

This condition is actually satisfied in Beta Finance case.

There are BETA rewards distributed to stablecoin markets on mainnet:

https://app.betafinance.org/

The major part of yield there is BETA rewards, say it is 0.01% USDC + 2.87% BETA APY for USDC market:

https://app.betafinance.org/deposit/0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48

And there is market for BETA token:

https://app.betafinance.org/deposit/0xbe1a001fe942f96eea22ba08783140b9dcc09d28

As mentioned in 360 comments, there is a shortcoming of the current design, that treats Vault as the destination of reward tokens, while it is Provider address interacting with the markets and Vault address has literally no link with the pools, so if the additional non-underlying rewards are due, they be due to the Provider's address.

It is assumed that govToken should end up being on the Vault balance:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Vault.sol#L404-L419

  function claimTokens() public {
    uint256 latestID = controller.latestProtocolId(vaultNumber);
    for (uint i = 0; i < latestID; i++) {
      if (currentAllocations[i] == 0) continue;
>>    bool claim = controller.claim(vaultNumber, i);
      if (claim) {
        address govToken = controller.getGovToken(vaultNumber, i);
>>      uint256 tokenBalance = IERC20(govToken).balanceOf(address(this));
        Swap.swapTokensMulti(
          Swap.SwapInOut(tokenBalance, govToken, address(vaultCurrency)),
          controller.getUniswapParams(),
          false
        );
      }
    }
  }

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Controller.sol#L57-L70

  function claim(
    uint256 _vaultNumber,
    uint256 _protocolNumber
  ) external override onlyVault returns (bool) {
    if (claimable[protocolInfo[_vaultNumber][_protocolNumber].LPToken]) {
      return
>>      IProvider(protocolInfo[_vaultNumber][_protocolNumber].provider).claim(
          protocolInfo[_vaultNumber][_protocolNumber].LPToken,
>>        msg.sender  // @audit it's Vault
        );
    } else {
      return false;
    }
  }

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/CompoundProvider.sol#L131-L139

  /// @notice Claims/harvest COMP tokens from the Comptroller
  /// @param _cToken Address of protocol LP Token eg cUSDC
  function claim(address _cToken, address _claimer) external override returns (bool) {
    address[] memory cTokens = new address[](1);
    cTokens[0] = _cToken;
>>  comptroller.claimComp(_claimer, cTokens);

    return true;
  }

But it is BetaProvider address interacts with Beta markets:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/BetaProvider.sol#L19-L40

  function deposit(
    uint256 _amount,
    address _bToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(address(this));

>>  IERC20(_uToken).safeTransferFrom(msg.sender, address(this), _amount);
    IERC20(_uToken).safeIncreaseAllowance(_bToken, _amount);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(address(this));
    require((balanceAfter - balanceBefore - _amount) == 0, "Error Deposit: under/overflow");

    uint256 tTokenBefore = IBeta(_bToken).balanceOf(address(this));
>>  IBeta(_bToken).mint(address(this), _amount);
    uint256 tTokenAfter = IBeta(_bToken).balanceOf(address(this));

    uint tTokensReceived = tTokenAfter - tTokenBefore;
    IBeta(_bToken).transfer(msg.sender, tTokensReceived);

    return tTokensReceived;
  }

And BetaProvider can have BETA part of the yield as the result of withdrawal:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/BetaProvider.sol#L48-L75

  function withdraw(
    uint256 _amount,
    address _bToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(msg.sender);

    uint256 balanceBeforeRedeem = IERC20(_uToken).balanceOf(address(this));

    require(
      IBeta(_bToken).transferFrom(msg.sender, address(this), _amount) == true,
      "Error: transferFrom"
    );
>>  IBeta(_bToken).burn(address(this), _amount);

    uint256 balanceAfterRedeem = IERC20(_uToken).balanceOf(address(this));
    uint256 uTokensReceived = balanceAfterRedeem - balanceBeforeRedeem;

    IERC20(_uToken).safeTransfer(msg.sender, uTokensReceived);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(msg.sender);
    require(
      (balanceAfter - balanceBefore - uTokensReceived) == 0,
      "Error Withdraw: under/overflow"
    );

    return uTokensReceived;
  }

This way the reward portion of the yield for the whole Vault can be stolen fully, as it will sit on the BetaProvider balance in the form of BETA tokens and there is a market for these tokens in Beta Finance, so this looks like valid high severity and needs to be fixed.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

hrishibhat commented 1 year ago

Escalation accepted

Considering issues #359 and #360 duplicates of this issue. These issues point to different ways in which the rewards in the provider can be stolen. However, considering the fact that derby protocol optimizes yield, this issue points towards the fact that the provider could eventually end up with some additional rewards/gov tokens from different protocols(which is not expected) and they can be lost/stolen. Considering this issue a valid high in this case.

Note: @sjoerdsommen care must be taken while adding code to sweep these additional rewards to avoid attacks mentioned in this issue and its duplicates.

sherlock-admin commented 1 year ago

Escalation accepted

Considering issues #359 and #369 duplicates of this issue. These issues point to different ways in which the rewards in the provider can be stolen. However, considering the fact that derby protocol optimizes yield, this issue points towards the fact that the provider could eventually end up with some additional rewards/gov tokens from different protocols(which is not expected) and they can be lost/stolen. Considering this issue a valid high in this case.

Note: @sjoerdsommen care must be taken while adding code to sweep these additional rewards to avoid attacks mentioned in this issue and its duplicates.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

hrishibhat commented 1 year ago

Note: The primary reason for considering the 3 issues collectively is that they identify the additional rewards being collected in these providers which is not originally intended. So being able to steal them is not a primary issue because these providers were not supposed to have any underlying balance in the first place. This is a subjective decision in this case after further discussion & consideration, and cannot be referenced for future contests. However, Sherlock will consider addressing such scenarios in the judging guide going forward.

Theezr commented 1 year ago

Fix: https://github.com/derbyfinance/derby-yield-optimiser/pull/201

dmitriia commented 1 year ago

Fix: derbyfinance/derby-yield-optimiser#201

Providers' deposit() and withdraw() now has onlyVault access control, looks ok.