code-423n4 / 2022-12-backed-findings

1 stars 3 forks source link

`PaprController.sol` doesn't support ERC20 Tokens with fee on transfer in `increaseDebtAndSell()` #295

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/with-backed/papr/blob/ba47c98f18116666e0da7d1ec5a7878b3522c6dc/src/PaprController.sol#L200-L204 https://github.com/with-backed/papr/blob/ba47c98f18116666e0da7d1ec5a7878b3522c6dc/src/PaprController.sol#L512-L516

Vulnerability details

PaprController.sol doesn't support ERC20 Tokens with fee on transfer in increaseDebtAndSell()

Vulnerability details

There are ERC20 tokens that charge fee for every transfer() / transferFrom().

PaprController.sol#increaseDebtAndSell() assumes that the received amount is the same as the transfer amount, and uses it to transfer a expected value of debt. While the actual transferred amount can be lower for those tokens.

Proof of Concept

https://github.com/with-backed/papr/blob/ba47c98f18116666e0da7d1ec5a7878b3522c6dc/src/PaprController.sol#L200-L204 https://github.com/with-backed/papr/blob/ba47c98f18116666e0da7d1ec5a7878b3522c6dc/src/PaprController.sol#L512-L516

        if (hasFee) {
            uint256 fee = amountOut * params.swapFeeBips / BIPS_ONE;
            underlying.transfer(params.swapFeeTo, fee);
            underlying.transfer(proceedsTo, amountOut - fee); //@audit fee can be less than expected
        }

Recommendation

Consider comparing before and after balance to get the actual transferred amount.

c4-judge commented 1 year ago

trust1995 changed the severity to QA (Quality Assurance)

trust1995 commented 1 year ago

Misbehaving ERC20 tokens have been discussed in the contest repo.

c4-judge commented 1 year ago

trust1995 marked the issue as grade-c