Maia DAO Ecosystem - 0xStalin's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L659-L696

Vulnerability details

Impact

Loss of funds caused by MEV bots sandwiching the transaction because the price can be easly manipulated.

Proof of Concept

• On the RootBridgeAgent:anyExecute() when swapping all the deposited gas on the _gasSwapIn() it is performed a swap, the problem is that the swap uses the instantaneous price from slot0 instead of the TWAP price. The slot0 price is calculated from the ratios of the assets of the pool. This ratio can however be easily manipulated and cause the swap to receive less funds than what it could've received.

Tools Used

Manual Review & This Article Explaining in depth the Uniswap v3 TWAP Oracle

Recommended Mitigation Steps

Use TWAP price instead of slot0 price. Here is an example implementation of TWAP.

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1335-L1342

Vulnerability details

Impact

• Incorrectly computing the magnitude of the token amounts will make that users transfer more tokens than what they really expect to transfer
• Also is possible that for a good number of users it becomes impossible to even cover the wrong computed amount of tokens, thus, they won't be able to use the systems
• Also using an incorrect magnitude can impact the internal accounting and balances in the system, which could lead to losses or even DoS in functions dependant on the internal accounting & balances.

Proof of Concept

• The current formula to normalize the decimals is the below one:

function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
    return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
}

• By doing the math we have:

• Example 1: A token that has 8 decimals:
  • _amount is scaled up by the tokensDecimals!
    • The formula: _amount * (10 ** _decimals) / 1 ether
    • Substituting values: [(10**8) * (10**8)] / (10**18) ===> (10**16) / (10**18) ===> 10**-2
    • result: This doesnt even compile in solidity, it will throw an error as follows:
      • TypeError: Return argument type rational_const 1 / 100 is not implicitly convertible to expected type (type of first return variable) uint256.

• Example 2: A token that has 30 decimals:

  • _amount is scaled up by the tokensDecimals!

    • The formula: _amount * (10 ** _decimals) / 1 ether
    • Substituting values: [(10**30) * (10**30)] / (10**18) ===> (10**60) / (10**18) ===> 10**42
    • result: 10**42

• As demonstrated in the above examples, the current formula doesn't compute correctly the normalization of the decimals to 18 decimals.

Tools Used

Manual Audit

Recommended Mitigation Steps

• Make sure to update the formula to normalize the decimals to 18 as follows:

function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
-   return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
+   return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);
}

• By doing the math we have:

• Example 1: A token that has 8 decimals:

  • _amount is scaled up by the tokensDecimals!

  • The formula: _amount * 1 ether / (10 ** _decimals)

  • Substituting values: [(10**8) * (10**18)] / (10**8) ===> (10**26) / (10**8) ===> 10**18

  • result: 10**18

• Example 2: A token that has 30 decimals:

  • _amount is scaled up by the tokensDecimals!

  • The formula: _amount * 1 ether / (10 ** _decimals)

  • Substituting values: [(10**30) * (10**18)] / (10**30) ===> (10**48) / (10**30) ===> 10**18

  • result: 10**18

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L383-L390

Vulnerability details

Impact

• Incorrectly computing the magnitude of the token amounts when denormalizing the decimals will make that that the contracts transfer to the users more tokens than what it should be transfered.
• Using an incorrect magnitude can impact the internal accounting and balances in the system, which could lead to losses or even DoS in functions dependant on the internal accounting & balances.

Proof of Concept

• The current formula to normalize the decimals is the below one:

function _denormalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
    return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);
}

• By doing the math we have:

• Example 1: A token that has 8 decimals:

  • _amount is scaled up by 10**18 because is the amount that is deposited inside the system, and all the deposits were normalized to 18 decimals

  • The formula: _amount * 1 ether / (10 ** _decimals)

  • Substituting values: [(10**18) * (10**18)] / (10**8) ===> (10**36) / (10**8) ===> 10**28

  • result: 10**28

• Example 2: A token that has 30 decimals:

  • _amount is scaled up by 10**18 because is the amount that is deposited inside the system, and all the deposits were normalized to 18 decimals

  • The formula: _amount * 1 ether / (10 ** _decimals)

  • Substituting values: [(10**18) * (10**18)] / (10**30) ===> (10**36) / (10**30) ===> 10**6

  • result: 10**6

• As demonstrated in the above examples, the current formula always returns the amount scaled up by a different magnitude than the tokenDecimals, which is incorrect because this formula should denormalize the amounts from 18 decimals to the tokenDecimals

Tools Used

Manual Audit

Recommended Mitigation Steps

• Make sure to update the formula as follows to correctly denormalize the decimals from 18 to the tokenDecimals :

function _denormalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
-   return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);
+   return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
}

• By doing the math using the suggested change we have:

• Example 1: A token that has 8 decimals:

  • _amount is scaled up by 10**18 because is the amount that is deposited inside the system, and all the deposits were normalized to 18 decimals

  • The formula: _amount * (10 ** _decimals) / 1 ether

  • Substituting values: [(10**18) * (10**8)] / (10**18) ===> (10**26) / (10**18) ===> 10**8

  • result: 10**8

• Example 2: A token that has 30 decimals:

  • _amount is scaled up by 10**18 because is the amount that is deposited inside the system, and all the deposits were normalized to 18 decimals

  • The formula: _amount * (10 ** _decimals) / 1 ether

  • Substituting values: [(10**18) * (10**30)] / (10**18) ===> (10**48) / (10**18) ===> 10**30

  • result: 10**30

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1083-L1116

Vulnerability details

Impact

• Not reading the correct offset where the nonce is located can lead to set as executed the incorrect nonce, which will cause unexpected behavior and potentially a DoS when attempting to execute a nonce that incorrectly was marked as already executed.

Proof of Concept

• The structure of the data is encoded as detailed in the IRootBridgeAgent contract

 *          |            Flag               |        Deposit Info        |             Token Info             |   DATA   |  Gas Info   |
 *          |           1 byte              |         4-25 bytes         |     3 + (105 or 128) * n bytes     |   ---	 |  32 bytes   |
 *          |_______________________________|____________________________|____________________________________|__________|_____________|

 *          | callOutSignedMultiple = 0x6   |   20b + 1b(n) + 4b(nonce)  |      32b + 32b + 32b + 32b + 3b 	  |   ---	 |  16b + 16b  |

• The actual encoding of the data happens on the BranchBridgeAgent contract, on these lines

• Based on the data structure we can decode and determine on what offset is located what data

  • data[0] => flag
  • data[1:21] => an address
  • data[21] => hTokens.length
  • data[22:26] => The 4 bytes of the nonce

• So, when flag is 0x06, the nonce is located at the offset data[22:26], that indicates that the current offset that is been accessed is wrong (data[PARAMS_START_SIGNED:25] === data[21:])

Tools Used

Manual Audit

Recommended Mitigation Steps

• Make sure to read the nonce from the correct offset, based on the data structure as explain in the IRootBridgeAgent contract

• For flag 0x06, read the offset as follows, either of the two options are correct:

  • nonce is located at: data[22:26]

nonce = uint32(bytes4(data[PARAMS_START_SIGNED + PARAMS_START : 26]));
nonce = uint32(bytes4(data[22:26]));

Assessed type

en/de-code

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1272-L1304

Vulnerability details

Impact

• Not reading the correct offset where the depositNonce is located can lead to set the status of the wrong deposit to Failed when the _clearDeposit() function is called.
  • The consequences of setting the incorrect depositNonce to False can be:
    • Getting stuck the deposits of the real depositNonce that is sent to the anyFallback() because that depositNonce won't be marked as Failed.
    • Causing troubles to other depositNonces that should not be marked as Failed.

Proof of Concept

• The structure of the data was encoded depending on the type of operation, that means that the depositNonce will be located at a different offset depending the flag.
• To see where exactly the depositNonce is located, is required to check the corresponding operation where the data was packed
  • Depending on the type of operation (flag) it will be the function we'll need to analyze to determine the correct offset where the depositNonce was packed.
• Let's analyze flag by flag the encoded data and determine the correct offset of the depositNonce for each flag

1. flag == 0x00
   • When encoding the data for the flag 0x00, we can see that the depositNonce is located at the data[1:5]

bytes memory packedData =
          abi.encodePacked(bytes1(0x00), depositNonce, _params, gasToBridgeOut, _remoteExecutionGas);

// data[0]    ==> flag === 0x00
// data[1:5]  ==> depositNonce

2. flag == 0x01
   • When encoding the data for the flag 0x01, we can see that the depositNonce is located at the data[1:5]

bytes memory packedData =
        abi.encodePacked(bytes1(0x01), depositNonce, _params, _gasToBridgeOut, _remoteExecutionGas);

// data[0]    ==> flag === 0x01
// data[1:5]  ==> depositNonce

3. flag == 0x02
   • When encoding the data for the flag 0x02, we can see that the depositNonce is located at the data[1:5]

bytes memory packedData = abi.encodePacked(
    bytes1(0x02),
    depositNonce,
    _dParams.hToken,
    _dParams.token,
    _dParams.amount,
    _normalizeDecimals(_dParams.deposit, ERC20(_dParams.token).decimals()),
    _dParams.toChain,
    _params,
    _gasToBridgeOut,
    _remoteExecutionGas
);

// data[0]    ==> flag === 0x02
// data[1:5]  ==> depositNonce

4. flag == 0x03
   • When encoding the data for the flag 0x03, we can see that the depositNonce is located at the data[2:6]

bytes memory packedData = abi.encodePacked(
    bytes1(0x03),
    uint8(_dParams.hTokens.length),
    depositNonce,
    _dParams.hTokens,
    _dParams.tokens,
    _dParams.amounts,
    deposits,
    _dParams.toChain,
    _params,
    _gasToBridgeOut,
    _remoteExecutionGas
);

// data[0]    ==> flag === 0x03
// data[1]    ==> hTones.length
// data[2:6]  ==> depositNonce

5. flag == 0x04
   • When encoding the data for the flag 0x04, we can see that the depositNonce is located at the data[21:25]

bytes memory packedData = abi.encodePacked(
    bytes1(0x04), msg.sender, depositNonce, _params, msg.value.toUint128(), _remoteExecutionGas
);

// data[0]    ==> flag === 0x04
// data[1:21] ==> msg.sender
// data[21:25]  ==> depositNonce

6. flag == 0x05
   • When encoding the data for the flag 0x05, we can see that the depositNonce is located at the data[21:25]

bytes memory packedData = abi.encodePacked(
    bytes1(0x05),
    msg.sender,
    depositNonce,
    _dParams.hToken,
    _dParams.token,
    _dParams.amount,
    _normalizeDecimals(_dParams.deposit, ERC20(_dParams.token).decimals()),
    _dParams.toChain,
    _params,
    msg.value.toUint128(),
    _remoteExecutionGas
);

// data[0]    ==> flag === 0x05
// data[1:21] ==> msg.sender
// data[21:25]  ==> depositNonce

7. flag == 0x06
   • When encoding the data for the flag 0x06, we can see that the depositNonce is located at the data[22:26]

bytes memory packedData = abi.encodePacked(
    bytes1(0x06),
    msg.sender,
    uint8(_dParams.hTokens.length),
    depositNonce,
    _dParams.hTokens,
    _dParams.tokens,
    _dParams.amounts,
    _deposits,
    _dParams.toChain,
    _params,
    msg.value.toUint128(),
    _remoteExecutionGas
);

// data[0]     ==> flag === 0x06
// data[1:21]  ==> msg.sender
// data[21]    ==> hTokens.length
// data[22:26] ==> depositNonce

• At this point now we know the exact offset where the depositNonce is located at for all the possible deposit options.
• Now is time to analyze the offset that are been read depending on the flag in the anyFallback() and validate that the correct offset is been read.

1. For flags 0x00 , 0x01 & 0x02, the depositNonce is been read from the offset data[PARAMS_START:PARAMS_TKN_START], which is the same as data[1:5] (PARAMS_START == 1 & PARAMS_TKN_START == 5), these 3 flags read correctly the depositNonce

2. For flag 0x03, the depositNonce is been read from the offset data[PARAMS_START + PARAMS_START:PARAMS_TKN_START + PARAMS_START], which is the same as data[2:6] (PARAMS_START == 1 & PARAMS_TKN_START == 5), this flag also reads correctly the depositNonce

3. For flag 0x04 & 0x05, the depositNonce is been read from the offset data[PARAMS_START_SIGNED:PARAMS_START_SIGNED + PARAMS_TKN_START], which is the same as data[21:26] (PARAMS_START_SIGNED == 21 & PARAMS_TKN_START == 5), these flags are reading INCORRECTLY the depositNonce, from the above analyzis to detect where the depositNonce is located at, we got that for flags 0x04 & 0x05, the depositNonce is located at the offset data[21:25]

PoC to demonstrate the correct offset of the depositNonce when data is encoded similar to how flags 0x04 & 0x05 encode it (See the above analysis for more details)

1. call the generateData() function and copy+paste the generated bytes on the rest of the functions
2. Notice how the readNonce() returns the correct value of the nonce, and is reading the offset data[21:25]

pragma solidity 0.8.18;

contract offset {
    uint32 nonce = 3;

    function generateData() external view returns (bytes memory) {
        bytes memory packedData = abi.encodePacked(
            bytes1(0x01),
            msg.sender,
            nonce
        );
        return packedData;
    }

    function readFlag(bytes calldata data) external view returns(bytes1) {
        return data[0];
    }

    function readMsgSender(bytes calldata data) external view returns (address) {
        return address(uint160(bytes20(data[1:21])));
    }

    function readNonce(bytes calldata data) external view returns (uint32) {
        return uint32(
            bytes4(data[21:25])
        );
    }   

}

3. For flag 0x06, the depositNonce is been read from the offset data[PARAMS_START_SIGNED + PARAMS_START:PARAMS_START_SIGNED + PARAMS_TKN_START + PARAMS_START], which is the same as data[22:27] (PARAMS_START_SIGNED == 21, PARAMS_START == 1 & PARAMS_TKN_START == 5), this flag is also reading INCORRECTLY the depositNonce, from the above analyzis to detect where the depositNonce is located at, we got that for flag 0x06, the depositNonce is located at the offset data[22:26]

PoC to demonstrate the correct offset of the depositNonce when data is encoded similar to how flag 0x06 encode it (See the above analysis for more details)

1. call the generateData() function and copy+paste the generated bytes on the rest of the functions
2. Notice how the readNonce() returns the correct value of the nonce, and is reading the offset data[22:26]

pragma solidity 0.8.18;

contract offset {
    uint32 nonce = 3;

    function generateData() external view returns (bytes memory) {
        bytes memory packedData = abi.encodePacked(
            bytes1(0x01),
            msg.sender,
            uint8(1),
            nonce
        );
        return packedData;
    }

    function readFlag(bytes calldata data) external view returns(bytes1) {
        return data[0];
    }

    function readMsgSender(bytes calldata data) external view returns (address) {
        return address(uint160(bytes20(data[1:21])));
    }

    function readThirdParameter(bytes calldata data) external view returns(uint8) {
        return uint8(bytes1(data[21]));
    }

    function readNonce(bytes calldata data) external view returns (uint32) {
        return uint32(
            bytes4(data[22:26])
        );
    }

}

Tools Used

Manual Audit

Recommended Mitigation Steps

• Make sure to read the depositNonce from the correct offset, depending on the flag it will be the offset where depositNonce is located at:

• For flags 0x04 & 0x05, read the offset as follows, either of the two options are correct:

  • depositNonce is located at: data[21:25]

_depositNonce = uint32(bytes4(data[PARAMS_START_SIGNED : PARAMS_START_SIGNED]));
_depositNonce = uint32(bytes4(data[21:25]));

• For flag 0x06, read the offset as follows, either of the two options are correct:
  • depositNonce is located at: data[22:26]

_depositNonce = uint32(bytes4(data[PARAMS_START_SIGNED + PARAMS_START : PARAMS_START_SIGNED + PARAMS_TKN_START]));
_depositNonce = uint32(bytes4(data[22:26]));

Assessed type

en/de-code

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L565-L634

Vulnerability details

Impact

• token addresses could be computed wrong which could lead to get stuck the tokens in the root chain

Proof of Concept

• The function clearTokens() is called from the BranchBridgeAgentExecutor::executeWithSettlementMultiple() function, which is used when the settlement flag is 2 "Multiple Settlements"

• As per the documentation about the messaging layer written in the IBranchBridgeAgent contract, when the flag is 2, the structure of the token info is as follows:

 *           - ht = hToken
 *           - t = Token
 *           - A = Amount
 *           - D = Destination
 *           - b = bytes
 *           - n = number of assets
 *           ________________________________________________________________________________________________________________________________
 *          |            Flag               |           Deposit Info           |             Token Info             |   DATA   |  Gas Info   |
 *          |           1 byte              |            4-25 bytes            |        (105 or 128) * n bytes      |   ---	   |  16 bytes   |
 *          |                               |                                  |            hT - t - A - D          |          |             |
 *          |_______________________________|__________________________________|____________________________________|__________|_____________|

 *          | callOutMulti = 0x2            |  1b(n) + 20b(recipient) + 4b     |   	     32b + 32b + 32b + 32b      |   ---	   |     16b     |

• 3 of the 4 parameters encoded in _sParams (hTokens, amounts and deposits) reads the whole 32 bytes and tokens read only 20 bytes.

Tools Used

Manual Audit

Recommended Mitigation Steps

• Standardize the way to read parameters from the received _sParams, if all parameters are bytes32, make sure to read all the bytes corresponding to such a parameter, and from there do the required conversions to another data types.

function clearTokens(bytes calldata _sParams, address _recipient)
    ...
{
    ...
        _tokens[i] = address(
            uint160(
                bytes20(
                    bytes32(
                        _sParams[
                            PARAMS_TKN_START + PARAMS_ENTRY_SIZE * uint16(i + numOfAssets) + 12:
                                PARAMS_TKN_START + PARAMS_ENTRY_SIZE * uint16(PARAMS_START + i + numOfAssets)
                        ]
                    )     
                )
            )
        );
    ...
}

Assessed type

en/de-code