InterchainProposalExecutor.sol:`_executeProposal` should not be called in `_execute`.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/main/contracts/interchain-governance-executor/InterchainProposalExecutor.sol#L62

Vulnerability details

Impact

_executeProposal should not be in _execute function because _executeProposal may require some ether, and AxelarExecutable#execute which calls _execute is not payable(and should not be payable), so the only option left is that native value should be in the InterchainProposalExecutor contract before execute is called. But this opens risks of frontrunning proposals.

Proof of Concept

Currently, the internal function _execute calls _executeProposal.

_executeProposal makes a low level call to an external target, attaching a native value with the call.

_execute is called by execute, which is a function in AxelarExecutable contract(which InterchainProposalExecutor inherits).

Since AxelarExecutable#execute is not payable, users cannot attach a native value with the call when trying to execute a proposal

It won't be right to modify AxelarExecutable#execute function because it is a more matured contract, and used by many contracts in the protocol.

Based on the current intended architecture, the only option left for a user, who wants to execute a proposal with some native value, is to send some native tokens to InterchainProposalExecutor before calling execute.

But this opens possibilities of front-running:

• A proposal is sent from Arbitrum to Ethereum with following params:
  • target: WETH token contract
  • value:500 ether
  • callData:deposit selector
• When Alice wants to execute this proposal on Ethereum,
  • She can't directly call InterchainProposalExecutor#execute, specifying eth value because AxelarExecutable#execute is not payable
  • Assuming this is working as intended, Alice's other option is to send 500 ether directly to InterchainProposalExecutor, then call InterchainProposalExecutor#execute function
• Bob's proposal which requires 100 ether has not been executed (Probably due to insufficient ether balance)
• Bob notices in mempool that Alice sent 500 ether, and made a InterchainProposalExecutor#execute call.
• Bob frontruns Alice's execute call with his own execute call
• Bob's transaction gets executed while Alice who actually sent the ether has her transaction reverted.

Tools Used

Manual Review

Recommended Mitigation Steps

Just like InterchainGovernance, _executeProposal should be outside of execute function, and should be called separately There should be a mapping of payloads(or encoded proposals), to a boolean which could be named executable. Within the execute function, instead of calling _executeProposal, the boolean value for that payload should be set to true, which means it is executable. There should be a payable external function called executeProposal that checks if the boolean value for that payload, which is about to be executed is set to true, and then should call _executeProposal, then set executable to false.

Alternatively, instead of setting executable in execute function, elligible voters should be allowed to vote on the proposal, and when votes meet a threshold, executable should be set to true.

Assessed type

Error

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #319

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #319

[c4-judge]

berndartmueller marked the issue as satisfactory