Any arbitraryCall gathered airdrop can be stolen with recoverTokens

[code423n4]

Handle

hyh

Vulnerability details

Impact

Any airdrop gathered with arbitraryCall will be immediately lost as an attacker can track arbitraryCall transactions and back run them with calls to recoverTokens, which doesn't track any tokens besides reward, deposit and incentive tokens, and will give the airdrop away.

Proof of Concept

arbitraryCall requires that tokens to be gathered shouldn't be reward, deposit or incentive tokens:
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L735

Also, the function doesn't mark gathered tokens in any way. Thus, the airdrop is freely accessible for anyone to be withdrawn with recoverTokens:
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L687

Recommended Mitigation Steps

Add airdrop tokens balance mapping, record what is gathered in arbitraryCall and prohibit their free withdrawal in recoverTokens similarly to incentives[].

Now:

mapping (address => uint112) public incentives;
...
function recoverTokens(address token, address recipient) public lock {
...
		if (incentives[token] > 0) {
			...
			uint256 excess = ERC20(token).balanceOf(address(this)) - incentives[token];
			...
		}

To be:

mapping (address => uint112) public incentives;
mapping (address => uint112) public airdrops;
...
function recoverTokens(address token, address recipient) public lock {
...
		if (incentives[token] > 0) {
			...
			uint256 excess = ERC20(token).balanceOf(address(this)) - incentives[token];
			...
		}
		if (airdrops[token] > 0) {
			...
			uint256 excess = ERC20(token).balanceOf(address(this)) - airdrops[token];
			...
		}
...
// we do know what airdrop token will be gathered
function arbitraryCall(address who, bytes memory data, address token) public lock externallyGoverned {
		...

		// get token balances
		uint256 preDepositTokenBalance = ERC20(depositToken).balanceOf(address(this));
		uint256 preRewardTokenBalance = ERC20(rewardToken).balanceOf(address(this));
		uint256 preAirdropBalance = ERC20(token).balanceOf(address(this));

		(bool success, bytes memory _ret) = who.call(data);
		require(success);
		
		uint256 postAirdropBalance = ERC20(token).balanceOf(address(this));
		require(postAirdropBalance <= type(uint112).max, "air_112");
		uint112 amt = uint112(postAirdropBalance - preAirdropBalance);
		require(amt > 0, "air");
		airdrops[token] += amt;

[brockelmore]

The intention is that the claim airdrop + transfer is done atomically. Compound-style governance contracts come with this ability out of the box.

[0xean]

Going to agree with the warden that as the code is written this is an appropriate risk to call out and be aware of. Downgrading in severity because it relies on external factors but there is no on chain enforcement that this call will be operated correctly and therefore believe it represent a valid concern even if the Sponsor has a mitigation plan in place.