Refund of protocol fee is being to wrong user

Summary

There is one function, _fillQuote(), which is handling swap from 0x. Ideally If there is any remaining protocol fee (in ETH) then it will be returned to sender aka msg.sender. There are scenarios when fee can be sent to receiver of swap instead.

Vulnerability Detail

Periphery and RollerPeriphery both are using almost identical logic in _fillQuote() hence this vulnerability affect both contracts. It exist if qupte.buyToken is ETH and there is any remaining protocol fee.

Here are pieces of puzzle

After swap if buyToken == ETH then store contract ETH balance in boughtAmount

// RollerPeriphery.sol
251:   boughtAmount = address(quote.buyToken) == ETH ? address(this).balance : quote.buyToken.balanceOf(address(this));

Next it store refundAmt

// RollerPeriphery.sol
257:        // Refund any unspent protocol fees (paid in ether) to the sender.
258:        uint256 refundAmt = address(this).balance;

Calculate actual refundAmt and transfer to sender

259:        if (address(quote.buyToken) == ETH) refundAmt = refundAmt - boughtAmount;
260:        payable(msg.sender).transfer(refundAmt);

This is clear that due to line 251, 258 and 259, refundAmt is 0. So sender is not getting refund.

Later on in logic flow buyToken will be transferred to receiver

110:        address(quote.buyToken) == ETH
111:                ? payable(receiver).transfer(amtOut)
112:                : ERC20(address(quote.buyToken)).safeTransfer(receiver, amtOut); // transfer bought tokens to receiver

Impact

Sender is not getting protocol fee refund.

Code Snippet

RollerPeriphery.sol#L251-L260

Periphery.sol#L921-L930

Tool used

Manual Review

Recommendation

Consider intercepting refund amount properly when buyToken is ETH or else just handle refund when buyToken is NOT ETH and write some explanation around it.

jparklev commented 1 year ago

While it is valid that when quote.buyToken is ETH, there's an error. This line:

if (address(quote.buyToken) == ETH) refundAmt = refundAmt - boughtAmount;

Always returns 0.

However, since the boughtAmount is set with the Periphery's ETH balance, the user will receive not only the bought amount from 0x but also the refunded fees.

As a result, we don't think there is any value at risk. However, it's a great callout and we're open to judge's perspective on this

hrishibhat commented 1 year ago

Considering this issue as low as the boughtAmount is inclusive of the refunded fees.

spyrosonic10 commented 1 year ago

Escalate for 10 USDC

Intention in code is to send refund fee back to msg.sender and send boughtAmount(NOT inclusive refund fee) to receiver(check point 5 from my report). So this is actually an issue if bounghtAmount is inclusive of refund fee. Different entities are supposed to get fee and boughtAmount. Assume by mistake a msg.sender send 1000x of fee, who should get remaining fee? Definitely not receiver.

For example:

redeem() which take receiver as a param
later in the logic in fillQuote() refundAmt/fee should be refunded to msg.sender
msg.sender is different than recipient of boughtAmount
as last step in redeem() (https://github.com/sherlock-audit/2023-03-sense/blob/main/auto-roller/src/RollerPeriphery.sol#L107-L113) boughtAmount aka amtOut is being send to receiver

So in the end if boughtAmount is inclusive of fee then that's a loss for msg.sender. So this issue deserve to be medium

sherlock-admin commented 1 year ago

Escalate for 10 USDC

Intention in code is to send refund fee back to msg.sender and send boughtAmount(NOT inclusive refund fee) to receiver(check point 5 from my report). So this is actually an issue if bounghtAmount is inclusive of refund fee. Different entities are supposed to get fee and boughtAmount. Assume by mistake a msg.sender send 1000x of fee, who should get remaining fee? Definitely not receiver.

For example:

redeem() which take receiver as a param

later in the logic in fillQuote() refundAmt/fee should be refunded to msg.sender

msg.sender is different than recipient of boughtAmount

as last step in redeem() (https://github.com/sherlock-audit/2023-03-sense/blob/main/auto-roller/src/RollerPeriphery.sol#L107-L113) boughtAmount aka amtOut is being send to receiver

So in the end if boughtAmount is inclusive of fee then that's a loss for msg.sender. So this issue deserve to be medium

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

hrishibhat commented 1 year ago

Escalation accepted

Issue is a valid medium After looking into this further, agree with the escalation. Given that the refunded fee is being sent to the receiver instead of the msg.sender, this is a valid issue, considering this issue as a valid medium.

sherlock-admin commented 1 year ago

Escalation accepted

Issue is a valid medium After looking into this further, agree with the escalation. Given that the refunded fee is being sent to the receiver instead of the msg.sender, this is a valid issue, considering this issue as a valid medium.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

jparklev commented 1 year ago

Fixed here: https://github.com/sense-finance/sense-v1/pull/348

We removed the protocol fee refunds entirely, since 0x no longer uses them, which obviated the need another fix