Behodler contest - Dravee's results

Handle

Dravee

Vulnerability details

Impact

Increased gas cost (possible DOS)

Proof of Concept

Here's the parameterize function:

function parameterize(
    address token,
    uint256 crossingThreshold,
    uint256 soulType,
    uint256 state,
    uint256 index,
    uint256 targetAPY,
    uint256 daiThreshold
  ) public notCurrent {
    require(morgothApprover.approved(token), "MORGOTH: token not approved for listing on Behodler");
    params.push(
      Parameters({
        token: token,
        crossingThreshold: crossingThreshold,
        soulType: soulType,
        state: state,
        index: index,
        targetAPY: targetAPY,
        daiThreshold: daiThreshold
      })
    );
  }

As we can see, as long as the token address has been approved, the parameters can be pushed multiple times in the array Parameters[] params. Also, there's no access control capabilities here.

Furthermore, the state variable Parameters[] params is used in a loop reading from storage and making external calls on all values:

  function execute() internal override returns (bool) {
    for (uint256 i = 0; i < params.length; i++) {
      uint256 fps = ammHelper.minAPY_to_FPS(params[i].targetAPY, params[i].daiThreshold);
      limbo.configureSoul(
        params[i].token,
        params[i].crossingThreshold,
        params[i].soulType,
        params[i].state,
        params[i].index,
        fps
      );
    }

    return true;
  }

While this will indeed update existing tokens, but it will first update them in every previous state they were in in the past. Having duplicates is not a good idea for gas cost and DOS potential here.

Tools Used

VS Code

Recommended Mitigation Steps

Consider making an existence check on the token parameter in the params storage variable and only push in the array if it's not found. Otherwise: edit the existing configuration values in params corresponding to the existing token

Handle

Dravee

Vulnerability details

Impact

Silent failures (lack of failure detection / revert in case of failure).

Proof of Concept

ERC20 transfer()/transferFrom() do not return booleans: Contracts compiled with solc >= 0.4.22 interacting with such functions will revert. Use OpenZeppelin's SafeERC20 (wrappers around ERC20 operations that throw on failure when the token contract implementation returns false. Tokens that return no value and instead revert or throw on failure are also supported with non-reverting calls assumed to be successful. Adds safeTransfer, safeTransferFrom, safeApprove, safeDecreaseAllowance, and safeIncreaseAllowance)

Instances include:

DAO\FlashGovernanceArbiter.sol:66:      IERC20(flashGovernanceConfig.asset).transferFrom(sender, address(this), flashGovernanceConfig.amount) &&  //@audit-ok checking return value
DAO\FlashGovernanceArbiter.sol:149:    IERC20(pendingFlashDecision[targetContract][msg.sender].asset).transfer(
DAO\LimboDAO.sol:22:      require(IERC20(token).transferFrom(from, to, uint256(amount)), "LimboDAO: ERC20 transfer from failed."); //@audit-ok checking return value
DAO\LimboDAO.sol:24:      require(IERC20(token).transfer(from, uint256(amount * (-1))), "LimboDAO: ERC20 transfer failed."); //@audit-ok checking return value
DAO\LimboDAO.sol:386:    require(ERC677(asset).transferFrom(sender, address(this), amount), "LimboDAO: transferFailed"); //@audit-ok checking return value
FlanBackstop.sol:87:    IERC20(config.flan).transfer(flanLP, normalizedAmount / 4);
FlanBackstop.sol:88:    IERC20(stablecoin).transferFrom(msg.sender, flanLP, amount / 2);
FlanBackstop.sol:95:    IERC20(stablecoin).transferFrom(msg.sender, pyroFlanLP, amount / 2);
Limbo.sol:216:    IERC20(token).transfer(address(crossingConfig.morgothPower), tokenBalance);
Limbo.sol:233:    IERC20(crossingConfig.behodler).transfer(crossingConfig.ammHelper, adjustedRectangle);
UniswapHelper.sol:184:      IERC20(VARS.behodler).transfer(pair, rectangleOfFairness);
UniswapHelper.sol:192:      IERC20(VARS.behodler).transfer(pair, rectangleOfFairness);
UniswapHelper.sol:230:    IERC20(inputToken).transfer(pair, amount);
UniswapHelper.sol:233:    ERC20Burnable(VARS.flan).transfer(recipient, reward);

The return values are checked where I marked "@audit-ok" above

Tools Used

VS Code

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

Handle

Dravee

Vulnerability details

There's a minor precision loss.

Replace:

File: FlanBackstop.sol
88:     IERC20(stablecoin).transferFrom(msg.sender, flanLP, amount / 2);
...
95:     IERC20(stablecoin).transferFrom(msg.sender, pyroFlanLP, amount / 2); //@audit minor precision loss

with

File: FlanBackstop.sol
88:     IERC20(stablecoin).transferFrom(msg.sender, flanLP, amount / 2);
...
95:     IERC20(stablecoin).transferFrom(msg.sender, pyroFlanLP, amount - amount / 2);

Handle

Dravee

Vulnerability details

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

DAO\Proposals\UpdateMultipleSoulConfigProposal.sol:64:    for (uint256 i = 0; i < params.length; i++) {
DAO\LimboDAO.sol:212:    for (uint256 i = 0; i < sushiLPs.length; i++) {
DAO\LimboDAO.sol:217:    for (uint256 i = 0; i < uniLPs.length; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

Store the array's length in a variable before the for-loop, and use it instead.

Handle

Dravee

Vulnerability details

Impact

SLOADs are expensive (~100 gas) compared to MLOADs/MSTOREs (~3 gas). Minimizing them can save gas.

Proof of Concept

The code is as such (see @audit tags):

File: FlashGovernanceArbiter.sol
60:   function assertGovernanceApproved(
61:     address sender,
62:     address target,
63:     bool emergency
64:   ) public {
65:     if (
66:       pendingFlashDecision[target][sender].unlockTime < block.timestamp && 
67:       IERC20(flashGovernanceConfig.asset).transferFrom(sender, address(this), flashGovernanceConfig.amount) //@audit flashGovernanceConfig.amount SLOAD 1 //@audit flashGovernanceConfig.asset SLOAD 1
68:     ) {
69:       require(
70:         emergency || (block.timestamp - security.lastFlashGovernanceAct > security.epochSize), 
71:         "Limbo: flash governance disabled for rest of epoch"
72:       );
73:       pendingFlashDecision[target][sender] = flashGovernanceConfig;
74:       pendingFlashDecision[target][sender].unlockTime += block.timestamp;
75: 
76:       security.lastFlashGovernanceAct = block.timestamp;
77:       emit flashDecision(sender, flashGovernanceConfig.asset, flashGovernanceConfig.amount, target); //@audit flashGovernanceConfig.amount SLOAD 2 //@audit flashGovernanceConfig.asset SLOAD 2
78:     } else {
79:       revert("LIMBO: governance decision rejected.");
80:     }
81:   }

It's possible to save 2 SLOAD (~200 gas) by caching flashGovernanceConfig.asset in a address variable, and flashGovernanceConfig.amount in a uint256 memory variables and use them instead of reading them repeatedly from storage.

Tools Used

VS Code

Recommended Mitigation Steps

Cache the storage values in memory variables and use them instead of repeatedly reading them from storage.

Handle

Dravee

Vulnerability details

The lines are duplicated here: https://github.com/code-423n4/2022-01-behodler/blob/71d8e0cfd9388f975d6a90dffba9b502b222bdfe/contracts/FlanBackstop.sol#L93-L94

    redeemRate = PyroTokenLike(config.pyroFlan).redeemRate();
    redeemRate = PyroTokenLike(config.pyroFlan).redeemRate();

I don't think the code needs to call this twice

Handle

Dravee

Vulnerability details

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of Concept

Revert strings > 32 bytes are here:

DAO\FlashGovernanceArbiter.sol:71:        "Limbo: flash governance disabled for rest of epoch"
DAO\FlashGovernanceArbiter.sol:147:      "Limbo: Flashgovernance decision pending."
DAO\LimboDAO.sol:270:      "LimboDAO: stated proposal does not match current proposal"
DAO\LimboDAO.sol:364:        "LimboDAO: stake invariant check 2."
DAO\LimboDAO.sol:412:      "LimboDAO: transfer ownership of limbo and flan."
Flan.sol:105:            "ERC20: transfer amount exceeds balance" 

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Shorten the revert strings to fit in 32 bytes.

Handle

Dravee

Vulnerability details

Impact

Looping twice is expensive. It's possible to iterate only once from 0 to maxLength.

Proof of Concept

Original code iterating between N and 2N times here: https://github.com/code-423n4/2022-01-behodler/blob/71d8e0cfd9388f975d6a90dffba9b502b222bdfe/contracts/DAO/LimboDAO.sol#L195-L222

It's possible to iterate only N times on sushiLPs and uniLPs. Here's the refacto I suggest, it starts from the comment //@audit refacto starts here and ends with //@audit refacto ends here:

File: LimboDAO.sol
213:     //@audit refacto starts here
214:     bool isSushiBigger = sushiLPs.length >= uniLPs.length;
215:     (uint256 shortLength, uint256 longLength) = isSushiBigger ? (uniLPs.length, sushiLPs.length) : (sushiLPs.length, uniLPs.length);
216: 
217:     for (uint256 i; i < shortLength; ++i) {
218:       sushiLoop(sushiLPs[i]);
219:       uniLoop(uniLPs[i]);
220:     }
221:     
222:     if (isSushiBigger) {
223:       for (uint256 i = shortLength; i < longLength; ++i) {
224:         sushiLoop(sushiLPs[i]);
225:       }
226:     } else {
227:       for (uint256 i = shortLength; i < longLength; ++i) {
228:         uniLoop(uniLPs[i]);
229:       }
230:     }
231:   }
232: 
233:   function sushiLoop(
234:     address sushiLp
235:   ) private {
236:     require(UniPairLike(sushiLp).factory() == sushiFactory, "LimboDAO: invalid Sushi LP");
237:     if (IERC20(eye).balanceOf(sushiLp) > 1000) assetApproved[sushiLp] = true;
238:     fateGrowthStrategy[sushiLp] = FateGrowthStrategy.indirectTwoRootEye;
239:   }
240: 
241:   function uniLoop(
242:     address uniLP
243:   ) private {
244:     require(UniPairLike(uniLP).factory() == uniFactory, "LimboDAO: invalid Uni LP"); //@audit-info Dravee: I corrected the comment
245:     if (IERC20(eye).balanceOf(uniLP) > 1000) assetApproved[uniLP] = true;
246:     fateGrowthStrategy[uniLP] = FateGrowthStrategy.indirectTwoRootEye;
247:   }
248:   //@audit refacto ends here

Tools Used

VS Code

Recommended Mitigation Steps

Apply the suggested refacto

Handle

Dravee

Vulnerability details

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

Instances include:

DAO\Proposals\UpdateMultipleSoulConfigProposal.sol:64:    for (uint256 i = 0; i < params.length; i++) {
DAO\LimboDAO.sol:212:    for (uint256 i = 0; i < sushiLPs.length; i++) {
DAO\LimboDAO.sol:217:    for (uint256 i = 0; i < uniLPs.length; i++) {
Flan.sol:15:    uint8 public burnOnTransferFee = 0; //% between 1 and 100, recipient pays
Limbo.sol:558:    uint256 flanBonus = 0;

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove explicit initialization for default values.

Handle

Dravee

Vulnerability details

Impact

Strict inequalities add a check of non equality which costs around 3 gas.

Proof of Concept

The following should use an inclusive inequality to save gas:

File: LimboDAO.sol
287:     uint256 cost = fate > 0 ? uint256(fate) : uint256(-fate);

Tools Used

VS Code

Recommended Mitigation Steps

Use >= instead of >

Handle

Dravee

Vulnerability details

Impact

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

Proof of Concept

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

DAO\Proposals\UpdateMultipleSoulConfigProposal.sol:64:    for (uint256 i = 0; i < params.length; i++) {
DAO\LimboDAO.sol:212:    for (uint256 i = 0; i < sushiLPs.length; i++) {
DAO\LimboDAO.sol:217:    for (uint256 i = 0; i < uniLPs.length; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

Use ++i instead of i++ to increment the value of an uint variable.

Handle

Dravee

Vulnerability details

Impact

Increased gas cost due to unnecessary automatic underflow checks.

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers.

When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block.

https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Proof of Concept

See the @audit tags, lines 180 and 183 can't underflow due to the conditional flow:

File: FlashGovernanceArbiter.sol
178:     if (v1 > v2) {
179:       if (v2 == 0) require(v1 <= 1, "FE1");
180:       else require(((v1 - v2) * 100) < security.changeTolerance * v1, "FE1"); //@audit should be unchecked
181:     } else {
182:       if (v1 == 0) require(v2 <= 1, "FE1");
183:       else require(((v2 - v1) * 100) < security.changeTolerance * v1, "FE1");//@audit should be unchecked
184:     }

Tools Used

VS Code

Recommended Mitigation Steps

Lines 180's (v1 - v2) and 183's (v2 - v1) should be inside unchecked blocks.

Handle

Dravee

Vulnerability details

Impact

For maps that use the same key value: having separate fields is error prone (like in case of deletion or future new fields).

Proof of Concept

In struct ConfigVars, the 4 maps use the same address:

File: FlanBackstop.sol
39:   struct ConfigVars {
40:     address flan;
41:     address pyroFlan;
42:     mapping(address => address) flanLPs; 
43:     mapping(address => address) pyroFlanLPs;
44:     mapping(address => uint256) acceptableHighestPrice;
45:     mapping(address => uint8) decimalPlaces;
46:   }

See their usage below, it's the exact same address stablecoin:

File: FlanBackstop.sol
57:   function setBacker(
58:     address stablecoin,
59:     address flanLP,
60:     address pyroFlanLP,
61:     uint256 acceptableHighestPrice,
62:     uint8 decimalPlaces
63:   ) external onlySuccessfulProposal {
64:     config.flanLPs[stablecoin] = flanLP;
65:     config.pyroFlanLPs[stablecoin] = pyroFlanLP;
66:     config.acceptableHighestPrice[stablecoin] = acceptableHighestPrice;
67:     config.decimalPlaces[stablecoin] = decimalPlaces;
68:   }
...
File: FlanBackstop.sol
77:     address flanLP = config.flanLPs[stablecoin];
78:     address pyroFlanLP = config.pyroFlanLPs[stablecoin];

Tools Used

VS Code

Recommended Mitigation Steps

I'd suggest these 4 related data get grouped in a struct, let's name it FlanLpsInfo:

struct FlanLpsInfo{
  address flanLPs; 
  address pyroFlanLPs;
  uint256 acceptableHighestPrice;
  uint8 decimalPlaces;
}

And it would be used as a state variable in this manner:

mapping(address => FlanLpsInfo) flanLpsInfo;

Then, you can delete all related fields with a simple delete flanLpsInfo[stablecoin].

Handle

Dravee

Vulnerability details

Impact

A division by 2 can be calculated by shifting one to the right.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

POC

Instances include:

FlanBackstop.sol:86:    FlanLike(config.flan).mint(address(this), normalizedAmount / 2);
FlanBackstop.sol:88:    IERC20(stablecoin).transferFrom(msg.sender, flanLP, amount / 2);
FlanBackstop.sol:95:    IERC20(stablecoin).transferFrom(msg.sender, pyroFlanLP, amount / 2);
FlanBackstop.sol:108:    uint256 premium = (flanToMint * (growth / 2)) / 100;
Limbo.sol:225:      adjustedRectangle = scxMinted / 2;

Tools Used

VS Code

Recommended Mitigation Steps

Replace / 2 with >> 1

Handle

Dravee

Vulnerability details

Impact

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

See: ethereum/solidity#9232

Consequences: each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..). since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

Proof of Concept

UniswapHelper.sol:56:  uint256 constant year = (1 days * 365);

Tools Used

VS Code

Recommended Mitigation Steps

Replace with:

UniswapHelper.sol:56:  uint256 constant year = 365 days;

Handle

Dravee

Vulnerability details

Impact

Checking non-zero transfer values can avoid an external call to save gas.

Proof of Concept

Here, the code is as such:

File: LimboDAO.sol
21:     if (amount > 0) { 
22:       require(IERC20(token).transferFrom(from, to, uint256(amount)), "LimboDAO: ERC20 transfer from failed.");
23:     } else {
24:       require(IERC20(token).transfer(from, uint256(amount * (-1))), "LimboDAO: ERC20 transfer failed.");
25:     }

while it should be as such to avoid making an unnecessary external call:

File: LimboDAO.sol
21:     if (amount > 0) { 
22:       require(IERC20(token).transferFrom(from, to, uint256(amount)), "LimboDAO: ERC20 transfer from failed.");
23:     } else if (amount != 0) {
24:       require(IERC20(token).transfer(from, uint256(amount * (-1))), "LimboDAO: ERC20 transfer failed.");
25:     }

Tools Used

VS Code

Recommended Mitigation Steps

Check if amount != 0.

Handle

Dravee

Vulnerability details

Impact

Some of the require statements can be placed earlier to reduce gas usage on revert.

Proof of Concept

From:

File: UniswapHelper.sol
114:   function configure(
115:     address _limbo,
116:     address FlanSCXPair,
117:     address behodler,
118:     address flan,
119:     uint256 divergenceTolerance,
120:     uint256 minQuoteWaitDuration,
121:     uint8 precision,
122:     uint8 priceBoostOvershoot
123:   ) public onlySuccessfulProposal {
124:     limbo = _limbo;
125:     VARS.Flan_SCX_tokenPair = UniPairLike(FlanSCXPair);
126:     VARS.behodler = behodler;
127:     VARS.flan = flan;
128:     require(divergenceTolerance >= 100, "Divergence of 100 is parity"); //@audit should move up before lots of SSTOREs
129:     VARS.divergenceTolerance = divergenceTolerance;
130:     VARS.minQuoteWaitDuration = minQuoteWaitDuration;
131:     VARS.DAI = 0x6B175474E89094C44Da98b954EedeAC495271d0F;
132:     VARS.precision = precision == 0 ? precision : precision;
133:     require(priceBoostOvershoot < 100, "Set overshoot to number between 1 and 100.");//@audit should move up before lots of SSTOREs
134:     VARS.priceBoostOvershoot = priceBoostOvershoot;
135:   }

to

File: UniswapHelper.sol
114:   function configure(
115:     address _limbo,
116:     address FlanSCXPair,
117:     address behodler,
118:     address flan,
119:     uint256 divergenceTolerance,
120:     uint256 minQuoteWaitDuration,
121:     uint8 precision,
122:     uint8 priceBoostOvershoot
123:   ) public onlySuccessfulProposal {
124:     require(divergenceTolerance >= 100, "Divergence of 100 is parity"); //@audit-info moved
125:     require(priceBoostOvershoot < 100, "Set overshoot to number between 1 and 100.");//@audit-info moved
126:     
127:     limbo = _limbo;
128:     VARS.Flan_SCX_tokenPair = UniPairLike(FlanSCXPair);
129:     VARS.behodler = behodler;
130:     VARS.flan = flan;
131:     VARS.divergenceTolerance = divergenceTolerance;
132:     VARS.minQuoteWaitDuration = minQuoteWaitDuration;
133:     VARS.DAI = 0x6B175474E89094C44Da98b954EedeAC495271d0F;
134:     VARS.precision = precision == 0 ? precision : precision;
135:     VARS.priceBoostOvershoot = priceBoostOvershoot;
136:   }

Up to 8 SSTOREs can be saved on revert this way (8 * 20 000 = 160 000 gas)

Tools Used

VS Code

Recommended Mitigation Steps

Reorder these require statements