[DOS] WHEN INTRODUCING A SWAP FEE DIFFERENT FROM 0 ON buyAndReduceDebt
, THE FUNCTION WILL ALWAYS REVERT
#60
Labels
2 (Med Risk)
Assets not at direct risk, but function/availability of the protocol could be impacted or leak value
bug
Something isn't working
duplicate-196
satisfactory
satisfies C4 submission criteria; eligible for awards
Lines of code
https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L225-L227
https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/test/paprController/BuyAndReduceDebt.t.sol#L12-L43
Vulnerability details
Impact
When the User specifies a swap fee
swapFeeBips
different from 0, the functionbuyAndReduceDebt
always reverts leaving the swap fee feature useless [DOS].Furthermore, the current implementation gives the user the ability to transfer any amount of the underlying balance of
PaprCopntroller
to theswapFeeTo
specified, even though the controller is not supposed to hold any underlying balance is important to fix this issue for future features where it could hold or hidden possible states.I ranked this vulnerability as medium risk because of the DOS impact but, the user being able to transfer any amount of the underlying balance of
PaprCopntroller
if it somehow would hold any underlying, even though is not very probable, in my opinion makes it eligible for high risk.Proof of Concept
Reusing
testBuyAndReduceDebtReducesDebtWithFee()
test:https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/test/paprController/BuyAndReduceDebt.t.sol#L12-L43
I have written the following PoC test initializing the fee variable to a value different from 0, to prove that the execution will always revert:
This PoC test always reverts.
Based on the tests on the
BuyAndReduceDebt.t.sol
file:https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/test/paprController/BuyAndReduceDebt.t.sol
My interpretation of the desired functionality of the swap fee is to be transferred directly from the user to the fee receiver. Because on both tests, before initializing the
SwapParams
object to callbuyAndReduce()
, thecontroller
is approved the (SwapAmount plus the FeeAmount):underlying.approve(address(controller), underlyingOut + underlyingOut * fee / 1e4);
.As the
fee
variable was not initialized on the tests, this scenario was not being tested.Therefore the
transfer()
function on line 226 ofPaprController.sol
.https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L226
Should be changed to a
transferFrom
function withmsg.sender
as thefrom
parameter in order to make this functionality work and prevent future exploits if any time the controller holds underlying balance:Tools Used
Foundry, Visual Studio
Recommended Mitigation Steps
As said earlier the
transfer
function should be changed to atransferFrom
function with msg.sender as first parameter.The text was updated successfully, but these errors were encountered: