BranchPort.withdraw transfers the incorrect amount

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L948-L952 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L984 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L212

Vulnerability details

Impact

Underlying token amounts are incorrect when withdrawing deposits.

Proof of Concept

In _redeemDeposit() of BranchBridgeAgent.sol, a user can withdraw their failed deposit. First, the deposits amounts are fetched by the _depositNonce. Next, clearToken() is called to mint and withdraw the local port and underlying tokens, respectively. When withdraw() is called, the _deposit amount is first converted using _denormalizeDecimals(). However, the result of the conversion is incorrect when token decimals are not 18.

Here are some examples when _denormalizeDecimals() is called:

• _deposit of 1e6, decimal 6: 1e6 * 1 ether / (10 ** 6) = 1e18
• _deposit of 1e20, decimal 20: 1e20 * 1 ether / (10 ** 20) = 1e18

In both cases, the result is 1e18 which is incorrect because the underlying amount should be in their respective decimals. In both cases, using _deposit amount would be correct.

Tools Used

Manual

Recommended Mitigation Steps

Consider using the stored _deposit amount to transfer the underlying amount.

Assessed type

Decimal

[c4-judge]

trust1995 marked the issue as duplicate of #758

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as partial-50

[c4-judge]

trust1995 marked the issue as full credit

[c4-judge]

trust1995 marked the issue as satisfactory