Use of `msg.value` in batch action

[code423n4]

Handle

cmichel

Vulnerability details

Vulnerability Details

The BatchAction.batchBalanceAction function allows batching and executing several actions in the same transaction. Actions can be a deposit action for ETH which uses the msg.value field of the transaction, see BalanceHandler.depositUnderlyingToken:

if (underlyingToken.tokenType == TokenType.Ether) {
    // @audit this is called from a batch deposit
    require(underlyingAmountExternal == int256(msg.value), "Invalid ETH balance");
}

Impact

There is no straightforward way to exploit it as it tries to mint cTokens in BalanceHandler.depositUnderlyingToken => assetToken.mint(uint256(underlyingAmountExternal)) in each iteration which should fail on the second iteration as the BatchAction contract does not keep any Ether in it?

Recommendation

Care must be taken to ensure that a single msg.value transaction is not counted several times in batch actions. We recommend using WETH that requires a .transferFrom in each iteration and does not rely on msg.value.

[jeffywu]

It is not possible to have msg.value be used twice.

We only allow so when we initalize the system we list ETH/cETH as the first currency id: https://github.com/code-423n4/2021-08-notional/blob/main/contracts/external/Router.sol#L72-L85

And then we require in both the batch action calls that currency ids are sorted ascending: https://github.com/code-423n4/2021-08-notional/blob/main/contracts/external/actions/BatchAction.sol#L121-L123

So it should not be possible to specify ETH twice within a single loop and msg.value is only accessed here: https://github.com/code-423n4/2021-08-notional/blob/main/contracts/internal/balances/BalanceHandler.sol#L82-L86

We also check that ETH can only be set once as currency id == 1 https://github.com/code-423n4/2021-08-notional/blob/main/contracts/internal/balances/TokenHandler.sol#L59-L74

[ghoul-sol]

There is no exploit found so I'll make this non-critical as this has been an issue historically.