`receiveApproval` allows to decrease withdrawals amounts and to invest on the behalf of others

[montyly]

Severity: High
Difficulty: Low

Description

AssetPool.receiveApproval allows to make a deposit on the behalf of an arbitrary address, and can lead to making withdrawal worthless, or make undesired investments.

AssetPool.receiveApproval allows arbitrary from:

coverage-pools/contracts/AssetPool.sol

Lines 85 to 97 in 4aee098

	function receiveApproval(
	address from,
	uint256 amount,
	address token,
	bytes calldata
	) external {
	require(
	IERC20(token) == collateralToken,
	"Unsupported collateral token"
	);
	_deposit(from, amount);
	}

Which will lead to execute the mint of tokens and a transferFrom operation:

coverage-pools/contracts/AssetPool.sol

Lines 257 to 258 in 4aee098

	underwriterToken.mint(depositor, toMint);
	collateralToken.safeTransferFrom(depositor, address(this), amount);

Anyone can thus invest on the behalf of users that would approve AssetPool.

Moreover, several high-profile tokens do not decrease the approval on transferFrom if the caller is the spender. If collateralToken is one of these tokens, collateralToken.safeTransferFrom(depositor, address(this), amount) will be a NOP if depositor is the AssetPool's address.

As a result, an attacker can mint unlimited underwriterToken tokens. While the minted tokens will be trapped in the AssetPool it will also lead the withdrawal amounts to be significantly smaller than excepted (underwriterToken.totalSupply will increase, but not collateralToken.balanceOf(address(this));):

coverage-pools/contracts/AssetPool.sol

Lines 167 to 177 in 4aee098

	uint256 covSupply = underwriterToken.totalSupply();
	delete withdrawalInitiatedTimestamp[underwriter];
	delete pendingWithdrawal[underwriter];
	// slither-disable-next-line reentrancy-events
	rewardsPool.withdraw();
	uint256 collateralBalance = collateralToken.balanceOf(address(this));
	uint256 amountToWithdraw =
	covAmount.mul(collateralBalance).div(covSupply);

Exploit Scenario

• Bob deploys an AssetPool, where the collateral token is UNI.
• Bob invest $100,000 worth of token. Bob has 10,000 underwriterToken tokens
• Later on, Bob starts a withdrawal.
• Eve calls AssetPool.receiveApproval, and mints 1,000,000 underwriterToken.
• As a result, Bob share is worthless, and Bob receives almost nothing from his withdrawal

Recommendations

Short term, remove AssetPool.receiveApproval

Long term, investigate the token integration checklist, and ensure the system is robust against the ERC20 edge cases.

[pdyraga]

I was looking at different scenarios and I am afraid I am not fully following this attack. When Eve invests on behalf of Bob, he receives COV and can withdraw his UNI later. Eve can no longer invest on behalf of Bob because UNI lowered the approval. Bob needs to do additional withdrawal but at the end he is not losing any tokens.

From UNI contract code:

function transferFrom(address src, address dst, uint rawAmount) external returns (bool) {
  address spender = msg.sender;
   // (...)

  if (spender != src && spenderAllowance != uint96(-1)) {
    // update allowance
  }

  _transferTokens(src, dst, amount);
  return true;
}

What is happening:

• Bob has 1,000,000 UNI.
• Bob calls uniToken.approve for 100 UNI.
• Bob calls assetPool.receiveApproval for 100 UNI. Bob receives 100 COV.
• Bob calls assetPool.initiateWithdrawal, 100 COV is transferred to Asset Pool.
• Eve calls assetPool.receiveApproval for 100 UNI of Bob's. This is possible because UNI token did not lower the approval for Bob's assetPool.receiveApproval call in the third step. Bob receives 100 COV.
• Withdrawal delay passes. Bob calls assetPool.completeWithdrawal and receives 100 UNI.
• Bob has 100 UNI more in the Asset Pool - they were invested by Eve on behalf of Bob. Eve can invest no more for Bob because UNI token lowered the approval when Eve called assetPool.receiveApproval on behalf of Bob.
• Bob calls assetPool.initiateWithdrawal, 100 COV is transferred to Asset Pool.
• Withdrawal delay passes. Bob calls assetPool.completeWithdrawal and receives 100 UNI.
• Bob has his original 1,000,000 UNI.

Agreed that the situation when receiveApproval allows third party to invest on behalf of Bob is not ideal. I was thinking that maybe it is enough to require msg.sender == address(collateralToken) in receiveApproval as suggested in #69? In that case, Eve would not be able to invest on behalf of Bob but Bob would still be able to invest in one transaction (approveAndCall instead of approve+deposit).

[montyly]

There are two consequences of the issue
1 - Anyone can invest on behalf of others directly
2 - Anyone can increase the total supply of the underwriterToken without depositing collateral.

The first one is clear I think.

The second one occurs in scenarios when the collateralToken does not decrease the allowance in case of self-transferFrom. From the UNI code you pasted, the condition spender != src falls under this scenario:

  if (spender != src && spenderAllowance != uint96(-1)) {
    // update allowance
  }

What it means, is that anyone can call receiveApproval(AssetPool_address, UNI.balanceOf(AssetPool_address), UNI_address, ""), which will lead to:

• Mint underwriterToken to AssetPool_address
• Do not increase the AssetPool's Uni balance (because uni.transferFrom(AssetPool, AssetPool, amount) does nothing)

So the total supply of underwriterToken increases, while the AssetPool's Uni balance does not. It will decrease the shares' values of everyone who invested in the pool. While an attacker can not make a direct profit from it, he can make significant loss for all the users at almost no cost.

Does that make more sense?

#78 should fix the issue. You could consider adding also depositor != address(this) as a safeguard in _deposit.

[pdyraga]

Yes, I see it now. UNI is not even checking if the approval exists in this case. receiveApproval(AssetPool_address, UNI.balanceOf(AssetPool_address), UNI_address, "") , in _deposit calls UNI.safeTransferFrom(AssetPool_address, AssetPool_address, UNI.balanceOf(AssetPool_address)) and spender == src. Adding a safeguard is definitely a good idea.