Behodler contest - shw's results

Handle

shw

Vulnerability details

Impact

Lack of access control on the assertGovernanceApproved function of FlashGovernanceArbiter allows anyone to lock other users' funds in the contract as long as the users have approved the contract to transfer flashGovernanceConfig.amount of flashGovernanceConfig.asset from them.

Proof of Concept

1. Alice wants to execute a flash governance decision (e.g., disable to the protocol), so she first calls approve on the flashGovernanceConfig.asset to allow FlashGovernanceArbiter to transfer flashGovernanceConfig.amount of assets from her.
2. An attacker Bob, who listens to the mempool, notices Alice's approve transaction and decides to front-run it. He calls assertGovernanceApproved with sender being Alice, target being any address, and emergency being true.
3. As a result, Alice cannot execute her flash governance decision, and her funds are locked in the contract for the flashGovernanceConfig.unlockTime period.

Referenced code: DAO/FlashGovernanceArbiter.sol#L60-L81

Recommended Mitigation Steps

Only allow certain addresses to call the assertGovernanceApproved function on FlashGovernanceArbiter.

Handle

shw

Vulnerability details

Impact

The implementation of the transferAndCall function in ERC677 is incorrect. It transfers the _value amount of tokens twice instead of once. Since the Flan contract inherits ERC667, anyone calling the transferAndCall function on Flan is affected by this double-transfer bug.

Proof of Concept

Below is the implementation of transferAndCall:

function transferAndCall(
    address _to,
    uint256 _value,
    bytes memory _data
) public returns (bool success) {
    super.transfer(_to, _value);
    _transfer(msg.sender, _to, _value);
    if (isContract(_to)) {
        contractFallback(_to, _value, _data);
    }
    return true;
}

We can see that super.transfer(_to, _value); and _transfer(msg.sender, _to, _value); are doing the same thing - transfering _value of tokens from msg.sender to _to.

Referenced code: ERC677/ERC677.sol#L28-L29

Recommended Mitigation Steps

Remove _transfer(msg.sender, _to, _value); in the transferAndCall function.

Handle

shw

Vulnerability details

Impact

A logic error in the burnFlashGovernanceAsset function that resets a user's pendingFlashDecision allows that user to steal other user's assets locked in future flash governance decisions. As a result, attackers can get their funds back even if they execute a malicious flash decision and the community burns their assets.

Proof of Concept

1. An attacker Alice executes a malicious flash governance decision, and her assets are locked in the FlashGovernanceArbiter contract.
2. The community disagrees with Alice's flash governance decision and calls burnFlashGovernanceAsset to burn her locked assets. However, the burnFlashGovernanceAsset function resets Alice's pendingFlashDecision to the default config (see line 134).
3. A benign user, Bob executes another flash governance decision, and his assets are locked in the contract.
4. Now, Alice calls withdrawGovernanceAsset to withdraw Bob's locked asset, effectively the same as stealing Bob's assets. Since Alice's pendingFlashDecision is reset to the default, the unlockTime < block.timestamp condition is fulfilled, and the withdrawal succeeds.

Referenced code: DAO/FlashGovernanceArbiter.sol#L134 DAO/FlashGovernanceArbiter.sol#L146

Recommended Mitigation Steps

Change line 134 to delete pendingFlashDecision[targetContract][user] instead of setting the pendingFlashDecision to the default.

Handle

shw

Vulnerability details

Impact

The LP pricing formula used in the burnAsset function of LimboDAO is vulnerable to flashloan manipulation. By swapping a large number of EYE into the underlying pool, an attacker can intentionally inflate the value of the LP tokens to get more fate than he is supposed to with a relatively low cost.

With the large portion of fate he gets, he has more voting power to influence the system's decisions, or even he can convert his fate to Flan tokens for a direct profit.

Proof of Concept

Below is an example of how the attack works:

1. Suppose that there are 1000 EYE and 1000 LINK tokens in the UniswapV2 LINK-EYE pool. The pool's total supply is 1000, and the attacker has 100 LP tokens.
2. If the attacker burns his LP tokens, he earns 1000 * 100/1000 * 20 = 2000 amount of fate.
3. Instead, the attacker swaps in 1000 EYE and gets 500 LINK from the pool (according to x * y = k, ignoring fees for simplicity). Now the pool contains 2000 EYE and 500 LINK tokens.
4. After the manipulation, he burns his LP tokens and gets 2000 * 100/1000 * 20 = 4000 amount of fate.
5. Lastly, he swaps 500 LINK into the pool to get back his 1000 EYE.
6. Compared to Step 2, the attacker earns a double amount of fate by only paying the swapping fees to the pool. The more EYE tokens he swaps into the pool, the more fate he can get. This attack is practically possible by leveraging flashloans or flashswaps from other pools containing EYE tokens.

The setEYEBasedAssetStake function has the same issue of using a manipulatable LP pricing formula. For more detailed explanations, please refer to the analysis of the Cheese Bank attack and the Warp Finance attack.

Referenced code: DAO/LimboDAO.sol#L356 DAO/LimboDAO.sol#L392

Recommended Mitigation Steps

Use a fair pricing formula for the LP tokens, for example, the one proposed by Alpha Finance.

Handle

shw

Vulnerability details

Impact

The generateFLNQuote is permissionless, meaning that anyone can call this function to update the latestFlanQuotes variables. However, when a token migrates from Limbo to Beholder, Limbo calls the stabilizeFlan function on UniswapHelper, which ensures block times between two latestFlanQuotes should be at least VARS.minQuoteWaitDuration apart.

Therefore, an attacker can continuously call the generateFLNQuote every VARS.minQuoteWaitDuration blocks to prevent tokens from migrating to Beholder, which effectively causes a DoS attack on the system. Although the attacker may not gain direct financial profit by launching such an attack, he might have an incentive to do it if it would cause a reputation loss on the Beholder protocol.

Besides, it might not be a good idea to let anyone update generateFLNQuote since the person can update the quotes when the prices have an advantage to himself (e.g., a higher or lower SCX price than the average). It further leads to an issue where people with conflicting preferences compete for the chance of updating the quote, causing the quotes to be updated too frequently and never be "stablized".

Proof of Concept

Referenced code: UniswapHelper.sol#L138-L145 UniswapHelper.sol#L162 Limbo.sol#L234

Recommended Mitigation Steps

Only allow authorized people or contracts to update the quotes.

Handle

shw

Vulnerability details

Impact

Most of the functions with a governanceApproved modifier call flashGoverner.enforceTolerance to ensure the provided parameters are restricted to some range of their original values. However, in the governanceApproved modifier, flashGoverner.setEnforcement(true); is called after the function body is executed, and thus the changed values are not restricted during the function execution.

An attacker can exploit this bug to change some critical parameters to arbitrary values by flash governance decisions. The effect will last until the community executes another proposal to correct the values. In the meanwhile, the attacker may make use of the corrupted values to launch an attack.

Proof of Concept

1. An attacker executes a flash governance decision, for example, the adjustSoul function of Limbo, and sets the fps of a soul to an extremely large value.
2. During the flash governance decision, some of his assets, for example, EYE, are locked in the FlashGovernanceArbiter contract.
3. He calls claimReward to get his rewards on the corresponding soul (assume that he has staked some number of the token before). Because of the manipulated fps, he gets a large number of Flan tokens as the reward.
4. Surely, he will lose his EYE tokens because of the malicious flash governance decision. However, as long as the attacker gets large enough Flan tokens, he is incentivized to launch such an attack.

Referenced code: DAO/Governable.sol#L46-L57 Limbo.sol#L380-L381 Limbo.sol#L327-L329 Limbo.sol#L530 Limbo.sol#L628-L630

Recommended Mitigation Steps

Rewrite the _governanceApproved function and the governanceApproved modifier as follows:

  function _governanceApproved(bool emergency) internal {
    bool successfulProposal = LimboDAOLike(DAO).successfulProposal(msg.sender);
    if (successfulProposal) {
      flashGoverner.setEnforcement(false);
    } else if (configured) {
      flashGoverner.setEnforcement(true);
      flashGoverner.assertGovernanceApproved(msg.sender, address(this), emergency);
    }
  }

  modifier governanceApproved(bool emergency) {
    _governanceApproved(emergency);
    _;
  }

Handle

shw

Vulnerability details

Impact

Most of the proposal contracts have a parameterize function for setting the proposal parameters, and these functions are protected only by the notCurrent modifier. When the proposal is proposed through a lodgeProposal transaction, an attacker can front-run it, modify the proposal parameters, and let the community vote it down. As a result, the person proposing loses his fate deposit.

Proof of Concept

1. A benign user Alice wants to make a proposal, so she deploys one of the proposal contracts and sets the intended parameters. Her proposal is approved by the ProposalFactory and is ready to be proposed.
2. Alice calls the lodgeProposal function of ProposalFactory to propose her proposal.
3. An attacker Bob, who listens to the mempool, notices Alice's transaction and front-runs it. He calls the parameterize function to change the parameters to undesirable ones.
4. Alice's proposal becomes the current proposal. However, the community rejects the proposal because of the changed parameters, causing Alice to lose her deposit.

Referenced code: DAO/Proposals/BurnFlashStakeDeposit.sol#L25-L37 DAO/Proposals/SetAssetApprovalProposal.sol#L21-L24 DAO/Proposals/ToggleWhitelistProposalProposal.sol#L22-L28 DAO/Proposals/UpdateMultipleSoulConfigProposal.sol#L40-L61 DAO/Proposals/WithdrawERC20Proposal.sol#L26-L32 DAO/ProposalFactory.sol#L74-L78

Recommended Mitigation Steps

Only allow the creator of the proposal to modify the parameters.