Inconsistently reading the encoded parameters received in the _sParams argument in the BranchBridgeAgent::clearTokens()

[code423n4]

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

[c4-judge]

trust1995 marked the issue as unsatisfactory: Insufficient proof

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as primary issue

[c4-sponsor]

0xBugsy marked the issue as disagree with severity

[c4-sponsor]

0xBugsy marked the issue as sponsor confirmed

[c4-judge]

trust1995 marked the issue as selected for report

[0xLightt]

Addressed https://github.com/Maia-DAO/eco-c4-contest/commit/762067df869fdf6e21fdd125524e93c51ead6db2