Digital garden

code4rena-2021-06-pooltogether-m01

4 min read

code4rena-2021-06-pooltogether-m01

[M-01] safeApprove() for Yearn Vault may revert preventing deposits causing DoS

보고서

Summary

반복적으로 호출될 수 있는 함수에서 safeApprove를 호출한다. 이로 인해 트랜잭션이 revert 되어 DoS의 가능성이 있다.

Keyword

safeApprove, openzeppelin, dos

Vulnerability

Openzeppelin의 SafeERC20 라이브러리의 safeApprove 함수는 한 번 approve 한 뒤 allowance가 0이 되지 않으면 재호출할 수 없다. 이러한 문제때문에 deprecated 되었다. Openzeppelin은 safeApprove를 이용한다면 초기 allowance를 설정할 때만 사용하고, 이후에는 safeIncreaseAllowance나 safeDecreaseAllowance를 이용하라고 권고한다.

  /**
    * @dev Deprecated. This function has issues similar to the ones found in
    * {IERC20-approve}, and its usage is discouraged.
    *
    * Whenever possible, use {safeIncreaseAllowance} and
    * {safeDecreaseAllowance} instead.
    */
  function safeApprove(
    IERC20 token,
    address spender,
    uint256 value
  ) internal {
    // safeApprove should only be called when setting an initial allowance,
    // or when resetting it to zero. To increase and decrease it, use
    // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
    require(
      (value == 0) || (token.allowance(address(this), spender) == 0),
      "SafeERC20: approve from non-zero to non-zero allowance"
    );
    _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
  }

safeApprove 호출 후 허용했던 금액을 그대로 사용하는 상황에는 이슈가 없다. 하지만 type(uint256).max만큼 허용해두고 사용하려는 의도라면 allowance를 0으로 초기화하지 않은 상태에 safeApprove가 재호출되는 상황이 발생하지 않아야 한다.

YearnV2YieldSource._depositInVault 함수에서 token.allowance(address(this), address(v)) < token.balanceOf(address(this))라면 safeApprove를 호출한다. 이는 allowance가 0이 아닌 상황에도 호출할 가능성이 있다. safeApprove가 allowance가 0이 아닌 상황에 호출된다면 _depositInVault 함수가 실패하고 트랜잭션이 취소될 것이다. 이는 잠재적으로 DoS를 발생한다.

  function _depositInVault() internal returns (uint256) {
    IYVaultV2 v = vault; // NOTE: for gas usage
    if(token.allowance(address(this), address(v)) < token.balanceOf(address(this))) {
      token.safeApprove(address(v), type(uint256).max);
    }
    // this will deposit full balance (for cases like not enough room in Vault)
    return v.deposit();
  }
 
  function supplyTokenTo(uint256 _amount, address to) external override nonReentrant {
    uint256 shares = _tokenToShares(_amount);
 
    _mint(to, shares);
 
    // NOTE: we have to deposit after calculating shares to mint
    token.safeTransferFrom(msg.sender, address(this), _amount);
 
    _depositInVault();
 
    emit SuppliedTokenTo(msg.sender, shares, _amount, to);
  }

Impact

유저의 입금이 revert 되어 DoS를 야기할 수 있다.

Mitigation

safeApprove 대신 safeIncreaseAllowance를 이용한다.

Memo

내가 생각하기에는 정확한 익스플로잇은 불가하고, 가능성 뿐인 취약점같다. initialize 함수에서 type(uint256).max로 approve를 하기때문에 토큰을 옮겨도 allowance는 type(uint256).max로 유지되기 때문이다. (allowance type(uint256).max는 예외 상황으로 처리되어 allowance를 깎지 않음)

tags: bughunting, pooltogether, smart contract, solidity, dos, safeApprove, openzeppelin, severity medium

Graph View