Summary
Shorter's can avoid being liquidated by front running the liquidation or flagging call and transferring their shortRecord to another EOA, as the address of the shorter is part of the reference to the shortRecord. Therefore, the call to flag or liquidation will revert after transferring the shortRecord to another EOA, as the given one is no longer a valid short record.
Vulnerability Details
When trying to liquidate a shortRecord, or flagging it for liquidation, it is first checked if the given shortRecord is valid:
function flagShort(address asset, address shorter, uint8 id, uint16 flaggerHint)
external
isNotFrozen(asset)
nonReentrant
**onlyValidShortRecord(asset, shorter, id)**
{
...
}
function liquidate(
address asset,
address shorter,
uint8 id,
uint16[] memory shortHintArray
)
external
isNotFrozen(asset)
nonReentrant
**onlyValidShortRecord(asset, shorter, id)**
returns (uint88, uint88)
{
...
}
The address of the shorter is passed to onlyValidShortRecord and used to check if the shortRecord is valid:
function _onlyValidShortRecord(address asset, address shorter, uint8 id)
internal
view
{
uint8 maxId = s.assetUser[asset][shorter].shortRecordId;
if (id >= maxId) revert Errors.InvalidShortId();
if (id < Constants.SHORT_STARTING_ID) revert Errors.InvalidShortId();
if (s.shortRecords[asset][shorter][id].status == SR.Cancelled) {
revert Errors.InvalidShortId();
}
}
modifier onlyValidShortRecord(address asset, address shorter, uint8 id) {
_onlyValidShortRecord(asset, shorter, id);
_;
}
Therefore, if the address of the shorter changes right before the flagShort, or liquidate call, the function would revert. As there is no check inside transferFrom and transferShortRecord, which would prevent the user from front running a flagShort, or liquidate call, it would be possible for shorters to front run margin callers and transfer the shortRecord with unhealthy CR to another EOA account so that the margin callers attempt to liquidate the shortRecord is reverted.
function transferFrom(address from, address to, uint256 tokenId) public {
if (tokenId > type(uint40).max) revert Errors.InvalidTokenId();
if (
msg.sender != from && !s.isApprovedForAll[from][msg.sender]
&& msg.sender != s.getApproved[tokenId]
) revert Errors.ERC721InsufficientApproval(msg.sender, tokenId);
address owner = ownerOf(tokenId);
if (owner != from) {
revert Errors.ERC721IncorrectOwner(from, tokenId, owner);
}
if (to == address(0)) {
revert Errors.ERC721InvalidReceiver(address(0));
}
STypes.NFT memory nft = s.nftMapping[tokenId];
address asset = s.assetMapping[nft.assetId];
LibShortRecord.transferShortRecord(asset, from, to, uint40(tokenId), nft);
delete s.getApproved[tokenId];
emit Events.Transfer(from, to, tokenId);
}
There is a check inside transferShortRecord which prevents transferring the shortRecord if it is already flagged, but this does not prevent front running the flagShort call:
function transferShortRecord(
address asset,
address from,
address to,
uint40 tokenId,
STypes.NFT memory nft
) internal {
AppStorage storage s = appStorage();
STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId];
if (short.status == SR.Cancelled) revert Errors.OriginalShortRecordCancelled();
if (short.flaggerId != 0) revert Errors.CannotTransferFlaggedShort();
deleteShortRecord(asset, from, nft.shortRecordId);
uint8 id = createShortRecord(
asset,
to,
SR.FullyFilled,
short.collateral,
short.ercDebt,
short.ercDebtRate,
short.zethYieldRate,
tokenId
);
if (id == Constants.SHORT_MAX_ID) {
revert Errors.ReceiverExceededShortRecordLimit();
}
s.nftMapping[tokenId].owner = to;
s.nftMapping[tokenId].shortRecordId = id;
}
Impact
Shorter's can avoid being liquidated without providing further collateral to keep the assets in a healthy CR.
Tools Used
Manual Review
Recommendations
Revert in transferShortRecord if the shortRecord is in an unhealthy CR.
