Tolerance is not enforced during a flash governance decision

[code423n4]

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);
    _;
  }

[gititGoro]

So this is a vulnerability for the very first execution of flashgovernance decision on a contract, after which it's safe. This is the type of thing that won't be acted upon because it will have gone away by the time the public interacts with Limbo. However, it is technically true so I'm confirming the issue.

[jack-the-pug]

Valid finding, but the conditions are quite strict, downgraded to Med.

[gititGoro]

FIX Behodler/limbo#13

Note: an additional flash loan bug that wasn't picked up in the audit was fixed.