Description

There is a lack of validation on the _loan object.

1. The ExternalLongStrategy._rebalanceExternally function calls _getLoan in order to retrieve the _loan associated with the specified tokenId and verify that the msg.sender is the creator of the loan.

function _getLoan(uint256 tokenId) internal virtual view returns(LibStorage.Loan storage _loan) {
     _loan = s.loans[tokenId];
     if(tokenId != uint256(keccak256(abi.encode(msg.sender, address(this), _loan.id)))) {
         revert Forbidden();
     }
 }

However, there is currently no check in place to ensure that the _loan exists and that the _loan.id is not a zero value.

Therefore, to bypass the check that the msg.sender is the loan's creator, a caller can pass the tokenId equal to uint256(keccak256(abi.encode(msg.sender, address(this), _loan.id)) where _loan.id equals zero.

This would cause the function to return an empty _loan object.

2. There is not a check that the _loan associated with tokenId exists inside the ExternalLiquidationStrategy._liquidateExternally function.

function _liquidateExternally(uint256 tokenId, uint128[] calldata amounts, uint256 lpTokens, address to, bytes calldata data) external override lock virtual returns(uint256 loanLiquidity, uint256[] memory refund) {
     uint128[] memory tokensHeld;
     uint256 writeDownAmt;
     uint256 collateral;

     LibStorage.Loan storage _loan = s.loans[tokenId];

     (loanLiquidity, collateral, tokensHeld, writeDownAmt) = getLoanLiquidityAndCollateral(_loan, s.cfmm);
 ...
}

As a result, the empty _loan object can be used inside this function.

Impact

Calling these functions with an empty _loan object will result in a reversion with an out-of-bounds error when trying to iterate over the empty tokensHeld array stored within the _loan object. However, even though this error provides a measure of protection, it is still crucial to perform explicit verification of data to prevent unexpected behavior during function execution.

Recommendations

Add a check that the _loan associated with tokenId is not empty.

function _getLoan(uint256 tokenId) internal virtual view returns(LibStorage.Loan storage _loan) {
     _loan = s.loans[tokenId];
+    require(_loan.id != 0)
     if(tokenId != uint256(keccak256(abi.encode(msg.sender, address(this), _loan.id)))) {
         revert Forbidden();
     }
 }

function _liquidateExternally(uint256 tokenId, uint128[] calldata amounts, uint256 lpTokens, address to, bytes calldata data) external override lock virtual returns(uint256 loanLiquidity, uint256[] memory refund) {
     uint128[] memory tokensHeld;
     uint256 writeDownAmt;
     uint256 collateral;

     LibStorage.Loan storage _loan = s.loans[tokenId];
+    require(_loan.id != 0)
     (loanLiquidity, collateral, tokensHeld, writeDownAmt) = getLoanLiquidityAndCollateral(_loan, s.cfmm);
 ...
}

Remediation

GammaSwap acknowledged this finding and implemented a fix in commit ec1a429e↗.