Astaria contest - 0xsomeone's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L172 https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L123 https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L143-L166 https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L498 https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L523 https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L642-L643

Vulnerability details

Impact

The payment token supplied encoded to the safeTransferFrom function call is not sanitized, permitting arbitrary payment tokens of any value to be provided instead of the actual intended one that was defined in the LienToken implementation's lien.token value. As a result, it is possible to pay debts using a worthless or malicious token, significantly compromising the integrity of the system.

Proof of Concept

A direct invocation of safeTransferFrom with an identifier argument of an arbitrary token will successfully "pay" the debt of a Lien token incorrectly. The currentOfferPrice computations used within ClearingHouse will also become meaningless as they will be done on the basis of a different token than the one provided for the payment of debts.

Tools Used

Manual review of the codebase.

Recommended Mitigation Steps

We advise the payment token to be queried via the LienToken itself rather than accepted as an argument, disallowing worthless / malicious tokens from being able to repay debts.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L148-L151 https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L160-L165 https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L637-L641 https://github.com/code-423n4/2023-01-astaria/blob/main/src/CollateralToken.sol#L566-L577

Vulnerability details

Impact

The ClearingHouse implementation performs a @solmate-based safeApprove instruction ([1]) with the remaining balanceOf(address(this)) but contains code handling any remainder of funds that may remain in the contract [2]. Investigation of the payDebtViaClearingHouse function will indicate that the function may not consume the maximum approval that was set to the TRANSFER_PROXY [3].

As a result, any consequent invocation of _execute via safeTransferFrom from OpenSea with a paymentToken (i.e. identifier) such as USDT would fail. Given that the ClearingHouse of a particular collateralId is created only once, this vulnerability will impact consequent listings and cause them to fatally fail for a token that has been used in the past and is part of the non-compliant EIP-20 tokens, with USDT being the prime and most popular example.

The code in USDT that causes this complication is as follows:

require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

Proof of Concept

The vulnerability is clearly defined above, however, for testing purposes, the ClearingHouse::safeTransferFrom function of a particular clearing house instance can be invoked twice with the same arguments. The second invocation will fail provided that the first invocation provided a token balance that exceeds the number of funds necessary for the debt payment which should be denoted in USDT.

On an important note, if the code used safeApprove from @openzeppelin this issue would affect any token, however, it is limited to non-standard tokens due to the usage of the @solmate implementation of safeApprove.

Tools Used

Manual review of the codebase. Historically, findings pertaining to incorrect approval mechanisms that do not support USDT have been marked as "medium" in severity in the past in the following cases:

• Rubicon: https://code4rena.com/reports/2022-05-rubicon/#m-08-usdt-is-not-supported-because-of-approval-mechanism
• Duality Focus: https://code4rena.com/reports/2022-04-dualityfocus/#m-03-not-calling-approve0-before-setting-a-new-approval-causes-the-call-to-revert-when-used-with-tether-usdt

Recommended Mitigation Steps

We advise approvals to be properly handled by evaluating whether a non-zero approval already exists and in such an instance nullifying it before re-setting it to a non-zero value. Example below:

// Optimizing lookup
address transferProxy = address(ASTARIA_ROUTER.TRANSFER_PROXY());

// If existing approval is non-zero -> set it to zero
if (ERC20(paymentToken).allowance(address(this), transferProxy) != 0) {
    ERC20(paymentToken).safeApprove(transferProxy, 0);
}

// Set non-zero approval
ERC20(paymentToken).safeApprove(transferProxy, payment - liquidatorPayment);