Donate can lock funds in certain situations

[andreiashu]

Description

donate function seems to allow users to donate an _amount of tokens of _asset to the margin pool:

review-opyn-gamma-2021-08/code/contracts/core/Controller.sol

Lines 316 to 322 in 879977f

	* @notice send asset amount to margin pool
	* @dev use donate() instead of direct transfer() to store the balance in assetBalance
	* @param _asset asset address
	* @param _amount amount to donate to pool
	*/
	function donate(address _asset, uint256 _amount) external {
	pool.transferToPool(_asset, msg.sender, _amount);

transferToPool is used in order to transfer the tokens from the user to the pool. This function takes care of the internal accounting inside the MarginPool contract and updates the assetBalance accordingly:

review-opyn-gamma-2021-08/code/contracts/core/MarginPool.sol

Lines 74 to 83 in 879977f

	function transferToPool(
	address _asset,
	address _user,
	uint256 _amount
	) public onlyController {
	require(_amount > 0, "MarginPool: transferToPool amount is equal to 0");
	assetBalance[_asset] = assetBalance[_asset].add(_amount);
	// transfer _asset _amount from _user to pool
	ERC20Interface(_asset).safeTransferFrom(_user, address(this), _amount);

We believe that the farm function was intended to be the one that would allow a designated farm account to withdraw the funds that were donated into the pool (ie. the difference between what is in the contract and assetBalance):

review-opyn-gamma-2021-08/code/contracts/core/MarginPool.sol

Lines 173 to 178 in 879977f

	uint256 externalBalance = ERC20Interface(_asset).balanceOf(address(this));
	uint256 storedBalance = assetBalance[_asset];
	require(_amount <= externalBalance.sub(storedBalance), "MarginPool: amount to farm exceeds limit");
	ERC20Interface(_asset).safeTransfer(_receiver, _amount);

However, the issue is that because transferToPool also updates the internal accounting in the assetBalance mapping, the require at line L176 in MarginPool contract will fail for any value of _amount higher than 0. Because the difference between the funds in the contract (externalBalance ) and the amount accounted in the assetBalance mapping (storedBalance) will be 0 (*). This leads to the situation where the donated funds are stuck in the contract:

review-opyn-gamma-2021-08/code/contracts/core/MarginPool.sol

Line 176 in 879977f

require(_amount <= externalBalance.sub(storedBalance), "MarginPool: amount to farm exceeds limit");

(*) this is true as long as users follow the recommendation in the donate documentation and only send transfers via this function. Any tokens sent directly to the MarginPool by use of transfer can actually be withdrawn / farmed.

Recommendation

Since this is a live contract and other systems might already be aware of the external donate function, it might make sense to just do a ERC20Interface(_asset).safeTransfer call directly, without calling the transferToPool. This way, when the farm function is called, the ERC20Interface(_asset).balanceOf call in MarginPool contract will indeed be higher than the assetBalance[_asset] and the difference can be farmed.

After further discussion with the development team, we understood the purpose of the donate method was to add funds to the protocol when it is under collateralized to cover for the missing funds. Anyone can call this method because they plan to have an emergency plan to add funds in the vault.