No Input Validation On _execute or safeTranferFrom

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L114
https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L169

Vulnerability details

Impact

An attacker can pass malicious tokenContract and encodedMetaData values.

Proof of Concept

The _execute function is called by safeTransferFrom and does not perform any input validation on the tokenContract and encodedMetaData arguments - neither at the _execute or safeTransferFrom levels.

function _execute(
address tokenContract, // collateral token sending the fake nft
address to, // buyer
uint256 encodedMetaData, //retrieve token address from the encoded data
uint256 // space to encode whatever is needed,
) internal {
IAstariaRouter ASTARIA_ROUTER = IAstariaRouter(_getArgAddress(0)); // get the router from the immutable arg

ClearingHouseStorage storage s = _getStorage();
address paymentToken = bytes32(encodedMetaData).fromLast20Bytes();

uint256 currentOfferPrice = _locateCurrentAmount({
    startAmount: s.auctionStack.startAmount,
    endAmount: s.auctionStack.endAmount,
    startTime: s.auctionStack.startTime,
    endTime: s.auctionStack.endTime,
    roundUp: true //we are a consideration we round up
});
uint256 payment = ERC20(paymentToken).balanceOf(address(this));

require(payment >= currentOfferPrice, "not enough funds received");

uint256 collateralId = _getArgUint256(21);
// pay liquidator fees here

ILienToken.AuctionStack[] storage stack = s.auctionStack.stack;

uint256 liquidatorPayment = ASTARIA_ROUTER.getLiquidatorFee(payment);

ERC20(paymentToken).safeTransfer(
    s.auctionStack.liquidator,
    liquidatorPayment
);

ERC20(paymentToken).safeApprove(
    address(ASTARIA_ROUTER.TRANSFER_PROXY()),
    payment - liquidatorPayment
);

ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse(
    paymentToken,
    collateralId,
    payment - liquidatorPayment,
    s.auctionStack.stack
);

if (ERC20(paymentToken).balanceOf(address(this)) > 0) {
    ERC20(paymentToken).safeTransfer(
    ASTARIA_ROUTER.COLLATERAL_TOKEN().ownerOf(collateralId),
    ERC20(paymentToken).balanceOf(address(this))
    );
}
ASTARIA_ROUTER.COLLATERAL_TOKEN().settleAuction(collateralId);
}


function safeTransferFrom(
address from, // the from is the offerer
address to,
uint256 identifier,
uint256 amount,
bytes calldata data //empty from seaport
) public {
//data is empty and useless
_execute(from, to, identifier, amount);
}

Recommended Mitigation Steps

Perform input validation on either functions. A comment mentions that the data is useless; if that is the case, then either remove the function altogether or perform input validation regardless - as the function might be adopted further without the proper security mechanisms in place.

[Picodes]

You need to develop what could be the impact of this with regards to user funds

[c4-judge]

Picodes marked the issue as duplicate of #564

[c4-judge]

Picodes marked the issue as partial-25

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as duplicate of #521