Missing payable keyword in function receiveFromBridge() in DecentBridgeAdapter

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/bridge_adapters/DecentBridgeAdapter.sol#L127

Vulnerability details

Impact

Currently, the DecentBridgeExecutor has two functions, namely, _executeWeth() and _executeEth(). The contract approves the WETH correctly to the adapter but it is not able to send the ETH since the receiveFromBridge() function is not marked payable. Due to this, the user would lose funds since the call to the payload (i.e. receiveFromBridge) always reverts.

Proof of Concept

1. The function execute() is called from the decentEthRouter contract. If the user originates the call from the bridge() function in the router and sets deliverEth to true, we enter the else block on Line 80. Note: This requires gasCurrencyIsEth to be true as well i.e. the chain should have it's native token to be ether.

File: DecentBridgeExecutor.sol
68:     function execute(
69:         address from,
70:         address target,
71:         bool deliverEth,
72:         uint256 amount,
73:         bytes memory callPayload
74:     ) public onlyOwner {
75:         weth.transferFrom(msg.sender, address(this), amount);
76: 
77:         if (!gasCurrencyIsEth || !deliverEth) {
78:             _executeWeth(from, target, amount, callPayload);
79:         } else {
80:             _executeEth(from, target, amount, callPayload);
81:         }
82:     }

2. The call on Line 80 above, leads us to the _executeEth() function. This function transfers msg.value = amount to the adapter contract and calls the payload. The payload is set to the receiveFromBridge() function in the adapter as seen here. But since the function is not payable, the call would revert. The call would never succeed which causes the user funds to be permanently locked on the destination chain.

File: DecentBridgeExecutor.sol
54:     function _executeEth(
55:         address from,
56:         address target,
57:         uint256 amount,
58:         bytes memory callPayload
59:     ) private {
60:         weth.withdraw(amount);
61:         (bool success, ) = target.call{value: amount}(callPayload);
62:         if (!success) {
63:             payable(from).transfer(amount);
64:         }
65:     }

Tools Used

Manual Review

Recommended Mitigation Steps

Add the payable keyword to the function receiveFromBridge() to allow accepting ETH when sent from executor.

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[raymondfam]

Intended design as IUTB(utb).receiveFromBridge() isn't payable either.

[c4-pre-sort]

raymondfam marked the issue as primary issue

[alex-ppg]

The confusion in this submission and the code's structure arises from the lack of access control in DecentEthRouter.

Specifically, the cross-chain transfers between DecentBridgeAdapter implementations will correctly execute when handled by a UTB top-level call as the DecentBridgeAdapter::bridge function hard-codes the deliverEth value to false.

As the DecentEthRouter lacks access control, direct usage of the router to transfer funds along with a misconfiguration of the call will cause funds to be permanently lost as the DecentBridgeAdapter cannot handle native funds, causing the non-blocking transaction to fail forever even if retried.

As the vulnerability will solely arise from misuse of the DecentEthRouter by the user, I consider it to be capped at QA per the relevant SC verdict and thus QA (L).

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[alex-ppg]

Based on other QA reports submitted in the contest I am inclined to award this a C grade.

[c4-judge]

alex-ppg marked the issue as grade-c

[mcgrathcoutinho]

Hi @alex-ppg, thank you for the detailed response. I believe issue this is valid since:

Based on the comment I've provided on #647, keeping the bridge() and bridgeWithPayload() function publicly accessible in the DecentEthRouter is intentional since decent bridge charges no bridge fees.

Additionally, there is no misconfiguration of the call happening here since the issue is talking about delivering ETH on a chain where ETH is the gas currency, which is why this condition if (!gasCurrencyIsEth || !deliverEth) in function execute() would have both gasCurrencyIsEth and deliverEth set to true, meaning we will enter the else block to call function _executeEth(). This is where the error occurs, which I have pointed out in the report.

Please consider re-evaluating this issue.

Thank you.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for following up on your original concerns in #647. You can consult #647 again to confirm that these functions should not be publicly accessible. Based on the above, my original ruling stands and can be considered final.