Behodler contest - kirk-baird's results

Handle

kirk-baird

Vulnerability details

Impact

The function assertGovernanceApproved(address sender, address target, bool emergency) has public visibility and may be called by any user.

Since the user who creates the transaction is able to specify the sender address they are able to force users who have approved FlashGovernanceArbiter to transfer their tokens to the FlashGovernanceArbiter.

The result is that the address which is the sender field will have their tokens locked as if they've made a flash governance decision and won't be able to withdraw them until unlockTime passes. They also will not be able to make any flash decisions while they wait for unlockTime to pass.

Proof of Concept

The function assertGovernanceApproved() does not validate either the caller of the function of the field sender.

  function assertGovernanceApproved(
    address sender,
    address target,
    bool emergency
  ) public {
    if (
      IERC20(flashGovernanceConfig.asset).transferFrom(sender, address(this), flashGovernanceConfig.amount) &&
      pendingFlashDecision[target][sender].unlockTime < block.timestamp
    ) {
      require(
        emergency || (block.timestamp - security.lastFlashGovernanceAct > security.epochSize),
        "Limbo: flash governance disabled for rest of epoch"
      );
      pendingFlashDecision[target][sender] = flashGovernanceConfig;
      pendingFlashDecision[target][sender].unlockTime += block.timestamp;

      security.lastFlashGovernanceAct = block.timestamp;
      emit flashDecision(sender, flashGovernanceConfig.asset, flashGovernanceConfig.amount, target);
    } else {
      revert("LIMBO: governance decision rejected.");
    }
  }

Recommended Mitigation Steps

Consider whitelisting a set of addresses who may call assertGovernanceApproved(). These whitelisted addresses will be those who have implemented Governable.sol.

Handle

kirk-baird

Vulnerability details

Impact

Users who have not called withdrawGovernanceAsset() after they have locked their tokens from a previous proposal (i.e. assertGovernanceApproved), will lose their tokens if assertGovernanceApproved() is called again with the same target and sender.

The sender will lose pendingFlashDecision[target][sender].amount tokens and the tokens will become unaccounted for and locked in the contract. Since the new amount is not added to the previous amount, instead the previous amount is overwritten with the new amount.

The impact of this is worsened by another vulnerability, that is assertGovernanceApproved() is a public function and may be called by any arbitrary user so long as the sender field has called approve() for FlashGovernanceArbiter on the ERC20 token. This would allow an attacker to make these tokens inaccessible for any arbitrary sender.

Proof of Concept

In assertGovernanceApproved() as seen below, the linependingFlashDecision[target][sender] = flashGovernanceConfig will overwrite the previous contents. Thereby, making any previous rewards unaccounted for and inaccessible to anyone.

Note that we must wait pendingFlashDecision[target][sender].unlockTime between calls.

  function assertGovernanceApproved(
    address sender,
    address target,
    bool emergency
  ) public {
    if (
      IERC20(flashGovernanceConfig.asset).transferFrom(sender, address(this), flashGovernanceConfig.amount) &&
      pendingFlashDecision[target][sender].unlockTime < block.timestamp
    ) {
      require(
        emergency || (block.timestamp - security.lastFlashGovernanceAct > security.epochSize),
        "Limbo: flash governance disabled for rest of epoch"
      );
      pendingFlashDecision[target][sender] = flashGovernanceConfig;
      pendingFlashDecision[target][sender].unlockTime += block.timestamp;

      security.lastFlashGovernanceAct = block.timestamp;
      emit flashDecision(sender, flashGovernanceConfig.asset, flashGovernanceConfig.amount, target);
    } else {
      revert("LIMBO: governance decision rejected.");
    }
  }

Recommended Mitigation Steps

Consider updating the initial if statement to ensure the pendingFlashDecision for that target and sender is empty, that is:

  function assertGovernanceApproved(
    address sender,
    address target,
    bool emergency
  ) public {
    if (
      IERC20(flashGovernanceConfig.asset).transferFrom(sender, address(this), flashGovernanceConfig.amount) &&
      pendingFlashDecision[target][sender].unlockTime == 0
    ) {
...

Note we cannot simply add the new amount to the previous amount incase the underlying asset has been changed.

Handle

kirk-baird

Vulnerability details

Impact

The function withdrawGovernanceAsset() is vulnerable to reentrancy, which would allow the attacker to drain the balance of the flashGoverananceConfig.asset.

Note: this attack assumes the attacker may gain control of the execution flow in asset.tranfer() which is the case for many ERC20 tokens such as those that implement ERC777 but will depend on which asset is chosen in the configuration.

Proof of Concept

withdrawGovernanceAsset() does not follow the check-effects-interactions pattern as seen from the following code snippet, where an external call is made before state modifications.

  function withdrawGovernanceAsset(address targetContract, address asset) public virtual {
    require(
      pendingFlashDecision[targetContract][msg.sender].asset == asset &&
        pendingFlashDecision[targetContract][msg.sender].amount > 0 &&
        pendingFlashDecision[targetContract][msg.sender].unlockTime < block.timestamp,
      "Limbo: Flashgovernance decision pending."
    );
    IERC20(pendingFlashDecision[targetContract][msg.sender].asset).transfer(
      msg.sender,
      pendingFlashDecision[targetContract][msg.sender].amount
    );
    delete pendingFlashDecision[targetContract][msg.sender];
  }

The attacker can exploit this vulnerability through the following steps:

1. assertGovernanceApproved(userA, target, false)
2. wait for unlockTime seconds to pass
3. withdrawGovernanceAsset(target, asset) and gain control of the execution during asset.transfer()
4. repeat step 3) until there balance of FlashGovernanceArbiter is less than pendingFlashDecision[target][msg.sender].amount

Recommended Mitigation Steps

There are two possible mitigations, the first is to implement the check-effects-interactions patter. This involves doing as checks and state changes before making external calls. To implement this in the current context delete the pendingFlashDecision before making the external call as follows.

  function withdrawGovernanceAsset(address targetContract, address asset) public virtual {
    require(
      pendingFlashDecision[targetContract][msg.sender].asset == asset &&
        pendingFlashDecision[targetContract][msg.sender].amount > 0 &&
        pendingFlashDecision[targetContract][msg.sender].unlockTime < block.timestamp,
      "Limbo: Flashgovernance decision pending."
    );
    uint256 amount = pendingFlashDecision[targetContract][msg.sender].amount;
    IERC20 asset = IERC20(pendingFlashDecision[targetContract][msg.sender].asset);
    delete pendingFlashDecision[targetContract][msg.sender];
    asset.transfer(msg.sender, amount);
  }

Handle

kirk-baird

Vulnerability details

Impact

The proposal to burn a user's tokens for a flash governance proposal does not result in the user losing any funds and may in fact unlock their funds sooner.

Proof of Concept

The function burnFlashGovernanceAsset() will simply overwrite the user's state with pendingFlashDecision[targetContract][user] = flashGovernanceConfig; as seen below.

  function burnFlashGovernanceAsset(
    address targetContract,
    address user,
    address asset,
    uint256 amount
  ) public virtual onlySuccessfulProposal {
    if (pendingFlashDecision[targetContract][user].assetBurnable) {
      Burnable(asset).burn(amount);
    }

    pendingFlashDecision[targetContract][user] = flashGovernanceConfig;
  }

Since flashGovernanceConfig is not modified in BurnFlashStakeDeposit.execute() the user will have amount set to the current config amount which is likely what they originally transferred in {assertGovernanceApproved()](https://github.com/code-423n4/2022-01-behodler/blob/main/contracts/DAO/FlashGovernanceArbiter.sol#L60).

Furthermore, unlockTime will be set to the config unlock time. The config unlock time is the length of time in seconds that proposal should lock tokens for not the future timestamp. That is unlock time may be say 7 days rather than now + 7 days. As a result the check in withdrawGovernanceAsset() pendingFlashDecision[targetContract][msg.sender].unlockTime < block.timestamp, will always pass unless there is a significant misconfiguration.

Recommended Mitigation Steps

Consider deleting the user's data (i.e. delete pendingFlashDecision[targetContract][user]) rather than setting it to the config. This would ensure the user cannot withraw any funds afterwards.

Alternatively, only update pendingFlashDecision[targetContract][user].amount to subtract the amount sent as a function parameter and leave the remaining fields untouched.

Handle

kirk-baird

Vulnerability details

Impact

There is a denial of service (DoS) attack on UniswapHelper.sol which will cause the ensurePriceStability modifier to revert whenever it is called by regularly calling generateFLNQuote().

By calling generateFLNQuote at intervals of less than VARS.minQuoteWaitDuration we will update the blockProduced time of each element in latestFlanQuotes.

As a result the final require statement in _ensurePriceStability() will revert.

The impact is that we could prevent users from calling stabilizeFlan() and minAPY_to_FPS(), which would prevent Limbo.sol from calling migrate() and attemptToTargetAPY() respectively.

Proof of Concept

generateFLNQuote is public and will set latestFlanQuotes[0].blockProduced and latestFlanQuotes[1].blockProduced respectively.

  function generateFLNQuote() public override {
    latestFlanQuotes[1] = latestFlanQuotes[0];
    (
      latestFlanQuotes[0].DaiScxSpotPrice,
      latestFlanQuotes[0].DaiBalanceOnBehodler
    ) = getLatestFLNQuote();
    latestFlanQuotes[0].blockProduced = block.number;
  }

_ensurePriceStability() will revert is VARS.minQuoteWaitDuration amount of time has not passed between the last two calls to generateFLNQuote().

    require(
      localFlanQuotes[0].blockProduced - localFlanQuotes[1].blockProduced > VARS.minQuoteWaitDuration &&
        localFlanQuotes[1].blockProduced > 0,
      "EH"
    );

Recommended Mitigation Steps

Consider preventing generateFLNQuote() from being called unless at least VARS.minQuoteWaitDuration seconds have passed.

Handle

kirk-baird

Vulnerability details

Impact

It is possible to bypass the additional 2hrs added to the length of voting when the vote() flips from positive to negative or vice versa.

This can be done by breaking the vote() into two steps first sending enough fate to make the proposal zero. Then sending fate to change the proposal to the opposite direction.

The impact is that we will bypass the protections that stop an attacker from swinging the vote in the final seconds with a large sum of fate. Thus, an attacker may pre-calculate the cost of swinging the proposal in the last seconds and weight it against the benefit of swinging the proposal. For example a proposal like withdrawERC20 may allow an attacker to withdraw ERC20 tokens and thus they may easily decide the cost vs benefit of taking these tokens.

Proof of Concept

The following lines can be found in vote() and represent the flipping logic.

      } else if (
        //The following if statement checks if the vote is flipped by fate
        fate * currentFate < 0 && //sign different
        (fate + currentFate) * fate > 0 //fate flipped current fate onto the same side of zero as fate
      ) {
        //extend voting duration when vote flips decision. Suggestion made by community member
        currentProposalState.start = currentProposalState.start + 2 hours;
      }

If transfer exactly enough fate that currentFate + fate = 0 then we will never pass (fate + currentFate) * fate > 0 since 0 * fate = 0. Then when we move from zero to the value we desire (either positive or negative) and this will aways pass due to the first condition fate * currentFate < 0 as currentFate = 0.

Steps:

• vote() with fate = -currentProposalState.fate
• vote() with fate set to the desired amount

Recommended Mitigation Steps

Consider handling the case where the vote moves from a non-zero integer to zero. Note the first edge case where initial state where currentProposalState.fate = 0 will need to be handled. Potentially modify the code such that zero is always considered positive.