Maia DAO Ecosystem - MohammedRizwan's results

Judge has assessed an item in Issue #727 as 2 risk. The relevant finding follows:

ERC20 check success issue

Judge has assessed an item in Issue #727 as 2 risk. The relevant finding follows:

ERC20 revert due to sigs issue

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/cfed0dfa3bebdac0993b1b42239b4944eb0b196c/src/talos/libraries/PoolActions.sol#L46-L52

Vulnerability details

Impact

While making a swap on UniswapV3 the caller should use the slippage parameter amountOutMinimum parameter to avoid losing funds.

In swapToEqualAmounts() does not use the slippage parameter amountOutMinimum.

File: /src/talos/libraries/PoolActions.sol#L46-L52

    function swapToEqualAmounts(ActionParams memory actionParams, int24 baseThreshold) internal {
        (bool zeroForOne, int256 amountSpecified, uint160 sqrtPriceLimitX96) = actionParams
            .pool
            .getSwapToEqualAmountsParams(
            actionParams.optimizer, actionParams.tickSpacing, baseThreshold, actionParams.token0, actionParams.token1
        );

        //Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
        actionParams.pool.swap(
            address(this),
            zeroForOne,
            amountSpecified,
            sqrtPriceLimitX96,
            abi.encode(SwapCallbackData({zeroForOne: zeroForOne}))
        );
    }

amountOutMinimum is used to specify the minimum amount of tokens the caller wants to be returned from a swap. Using amountOutMinimum tells the swap that the caller will accept a minimum amount of output tokens from the swap, opening up the user to a catastrophic loss of funds via MEV bot sandwich attacks.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/cfed0dfa3bebdac0993b1b42239b4944eb0b196c/src/talos/libraries/PoolActions.sol#L46-L52

Tools Used

Manual review

Recommended Mitigation Steps

Use/Introduce parameters amountOutMinimum to avoid loss of funds.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/cfed0dfa3bebdac0993b1b42239b4944eb0b196c/src/talos/libraries/PoolActions.sol#L46-L52

Vulnerability details

Impact

In PoolActions.sol, swapToEqualAmounts() function has no deadline check for the transaction when swapping.

File: src/talos/libraries/PoolActions.sol

    function swapToEqualAmounts(ActionParams memory actionParams, int24 baseThreshold) internal {
        (bool zeroForOne, int256 amountSpecified, uint160 sqrtPriceLimitX96) = actionParams
            .pool
            .getSwapToEqualAmountsParams(
            actionParams.optimizer, actionParams.tickSpacing, baseThreshold, actionParams.token0, actionParams.token1
        );

        //Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
        actionParams.pool.swap(
            address(this),
            zeroForOne,
            amountSpecified,
            sqrtPriceLimitX96,
            abi.encode(SwapCallbackData({zeroForOne: zeroForOne}))
        );
    }

AMMs provide their users with an option to limit the execution of their pending actions, such as swaps or adding and removing liquidity. The most common solution is to include a deadline timestamp as a parameter (for example see Uniswap V2 and Uniswap V3). If such an option is not present, users can unknowingly perform bad trades:

1. Alice wants to swap 100 tokens for 1 ETH and later sell the 1 ETH for 1000 DAI.
2. The transaction is submitted to the mempool, however, Alice chose a transaction fee that is too low for miners to be interested in including her transaction in a block. The transaction stays pending in the mempool for extended periods, which could be hours, days, weeks, or even longer.
3. When the average gas fee dropped far enough for Alice’s transaction to become interesting again for miners to include it, her swap will be executed. In the meantime, the price of ETH could have drastically changed. She will still get 1 ETH but the DAI value of that output might be significantly lower. She has unknowingly performed a bad trade due to the pending transaction she forgot about.

An even worse way this issue can be maliciously exploited is through MEV:

1. The swap transaction is still pending in the mempool. Average fees are still too high for miners to be interested in it. The price of tokens has gone up significantly since the transaction was signed, meaning Alice would receive a lot more ETH when the swap is executed. But that also means that her maximum slippage value (sqrtPriceLimitX96 and minOut in terms of the Spell contracts) is outdated and would allow for significant slippage.
2. A MEV bot detects the pending transaction. Since the outdated maximum slippage value now allows for high slippage, the bot sandwiches Alice, resulting in significant profit for the bot and significant loss for Alice.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/cfed0dfa3bebdac0993b1b42239b4944eb0b196c/src/talos/libraries/PoolActions.sol#L46-L52

Tools Used

Manual review

Recommended Mitigation Steps

Introduce a deadline parameter to all functions which potentially perform a swap.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L18-L27 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L68-L72 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L397-L423

Vulnerability details

Impact

In GovernorBravoDelegateMaias.sol contract, There are wrong calculation in MIN_VOTING_PERIOD,MAX_VOTING_PERIOD, MIN_VOTING_DELAY, MAX_VOTING_DELAY because of incorrect consideration of block formation period.

The contracts will be deployed on Ethereum mainnet Chain too and in a Ethereum mainnet chain, the blocks are made every 12 seconds but the votingPeriod and votingDelay variables has used 15 seconds while calculating their values.

For example:

MIN_VOTING_PERIOD is considered for 2 weeks.

   uint256 public constant MIN_VOTING_PERIOD = 80640; // About 2 weeks

2 weeks(in seconds) = 12,09,600 Considered Ethereum blockformation time in second = 15

Therefore, MAX_VOTING_PERIOD = 12,09,600 / 15 = 80,640 (blocks).

This is how the calculations have arrived for other votingPeriod and votingDelay state variables.

However, Ethereum block formation happens on every 12 seconds and it is confirmed from below sources, Reference-01 Reference-02

The correct calculation should be with 12 seconds as block formation period.

For example: 2 weeks(in seconds) = 12,09,600 Actual Ethereum blockformation time in second = 12

Therefore, MAX_VOTING_PERIOD = 12,09,600 / 12 = 100,800 (blocks).

Total number of block difference for 2 week duration = 100,800 - 80,640 = 20,160 ~ 5.6 hours This much time difference will affect the function validations which will cause unexpected design failure.

MIN_VOTING_PERIOD,MAX_VOTING_PERIOD, MIN_VOTING_DELAY, MAX_VOTING_DELAY are used in functions which are further explained as below,

File: src/governance/GovernorBravoDelegateMaia.sol

56    function initialize(
57        address timelock_,
58        address govToken_,
59        uint256 votingPeriod_,
60        uint256 votingDelay_,
61        uint256 proposalThreshold_
62    ) public virtual {
63        require(address(timelock) == address(0), "GovernorBravo::initialize: can only initialize once");
64        require(msg.sender == admin, "GovernorBravo::initialize: admin only");
65        require(timelock_ != address(0), "GovernorBravo::initialize: invalid timelock address");
66        require(govToken_ != address(0), "GovernorBravo::initialize: invalid govToken address");
67        require(
68            votingPeriod_ >= MIN_VOTING_PERIOD && votingPeriod_ <= MAX_VOTING_PERIOD,
69            "GovernorBravo::initialize: invalid voting period"
70        );
71        require(
72            votingDelay_ >= MIN_VOTING_DELAY && votingDelay_ <= MAX_VOTING_DELAY,
73            "GovernorBravo::initialize: invalid voting delay"
74        );

         // some code

At L-68 and L-72, these state variables are used to validate the conditions in initialize() function which can be called only once. These incorrect values makes the conditions at L-68 and L-72 obsolete and the conditions wont work as expected by design.

Further MIN_VOTING_PERIOD,MAX_VOTING_PERIOD, MIN_VOTING_DELAY, MAX_VOTING_DELAY these state variables are used in below setter functions which for sure wont work as per expected design.

397  function _setVotingDelay(uint256 newVotingDelay) external {


413   function _setVotingPeriod(uint256 newVotingPeriod) external {

Discussion with Sponsors

I had a discussion with sponsor(@0xbuzzlightyear) on this finding and the sponsor has confirmed the issue. Below is the discord discussion with sponsor for reference and finding confirmation only,

Mohammed Rizwan — 06/30/2023 at 5:00 PM

uint256 public constant MIN_VOTING_PERIOD = 80640; // About 2 weeks

Here, it is considered 15 sec for block formation considering Ethereum chain. On ethereum, the average block formation time is 12 sec. Reference- https://ycharts.com/indicators/ethereum_average_block_time Ethereum Average Block Time In depth view into Ethereum Average Block Time including historical data from 2015 to 2023, charts and stats.

0xbuzzlightyear — 06/30/2023 at 5:06 PM true, nice finding, we did that before the merge :pepepalm:

Mohammed Rizwan — 06/30/2023 at 5:07 PM Thank you. If you allow me, can i add this discussion in report?

0xbuzzlightyear — 06/30/2023 at 5:09 PM yes, of course. you are free to do what you wish fren!

Mohammed Rizwan — 06/30/2023 at 5:09 PM Thanks again!!!

0xbuzzlightyear — 06/30/2023 at 5:11 PM you're welcome! :pepoloveleaf:

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L18-L27

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L68-L72

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L397-L423

Tools Used

Manual Review

Recommended Mitigation Steps

Consider 12 seconds block formation period and correct the calculations.

Assessed type

Other

Gas Optimizations

[G‑01] Catch num parameters as local variables in ERC20Boost.gauges() to save gas

Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read. It will save for each iteration. There are 2 instances of this issue:

In ERC20Boost.sol. at L-50. Link to code

In ERC20Boost.sol at L-96. Link to code

Recommended Mitigation steps

File: src/erc-20/ERC20Boost.sol

    function gauges(uint256 offset, uint256 num) external view returns (address[] memory values) {
+       uint256 _num = num;
-        values = new address[](num);
+        values = new address[](_num);
-        for (uint256 i = 0; i < num;) {
+        for (uint256 i = 0; i < _num;) {
            unchecked {
                values[i] = _gauges.at(offset + i); // will revert if out of bounds
                i++;
            }
        }
    }

File: src/erc-20/ERC20Boost.sol

    function userGauges(address user, uint256 offset, uint256 num) external view returns (address[] memory values) {
+       uint256 _num = num;
-        values = new address[](num);
+        values = new address[](_num);
-        for (uint256 i = 0; i < num;) {
+        for (uint256 i = 0; i < _num;) {
            unchecked {
                values[i] = _userGauges[user].at(offset + i); // will revert if out of bounds
                i++;
            }
        }
    }

[G‑02] It costs more gas to initialize variables with their default value than letting the default value be applied

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

There are 10 instances of this issue: In ERC20Boost.sol at L-50, Link to code

In ERC20Boost.sol at L-98, Link to code

In ERC20Boost.sol at L-156, Link to code

In ERC20Boost.sol at L-207, Link to code

In ERC20Boost.sol at L-236, Link to code

In ERC20Gauges.sol at L-114, Link to code

In ERC20Gauges.sol at L-259, Link to code

In ERC20Gauges.sol at L-341 Link to code

In ERC20Gauges.sol at L-536 Link to code

In ERC20MultiVotes.sol at L-328 Link to code

Recommended Mitigation steps

For example:

-      for (uint256 i = 0; i < num;) {
+      for (uint256 i; i < num;) {

[G‑03] Use modifier instead of repeating code for access validation in GovernorBravoDelegateMaia.sol

In GovernorBravoDelegateMaia.sol. There is repeated code for access validation in functions like initialize(), _setVotingDelay(), _setVotingPeriod(), _setProposalThreshold(), _setWhitelistGuardian(), _initiate(), _setPendingAdmin(). However a modifier can be used which will save lots of bytes thereby saving good amount of gas. There are 7 instance of this issue:

Recommended Mitigation steps

For example,

+   modifier onlyAdmin(){
+   require(msg.sender == admin, "GovernorBravo::access to admin only");
+   _; 
+    }

-    function _initiate(address governorAlpha) external {
+    function _initiate(address governorAlpha) external onlyAdmin{
-       require(msg.sender == admin, "GovernorBravo::_initiate: admin only");
        require(initialProposalId == 0, "GovernorBravo::_initiate: can only initiate once");
        proposalCount = GovernorAlpha(governorAlpha).proposalCount();
        initialProposalId = proposalCount;
        timelock.acceptAdmin();
    }

[G‑04] Catch function paramters as local variables to save gas

Catching function parameters as local variables. Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read. It will save for each iteration. There are 2 instances of this issue: In ERC20Gauges.sol at L-112 Link to code

In ERC20Gauges.sol at L-153 Link to code

Recommended Mitigation steps

For example as taken from G-02:

    function gauges(uint256 offset, uint256 num) external view returns (address[] memory values) {
+       uint256 _num = num;
-        values = new address[](num);
+        values = new address[](_num);
-        for (uint256 i = 0; i < num;) {
+        for (uint256 i = 0; i < _num;) {
            unchecked {
                values[i] = _gauges.at(offset + i); // will revert if out of bounds
                i++;
            }
        }
    }