sherlock-audit / 2023-03-sense-judging

4 stars 0 forks source link

0x52 - Multiple functions aren't payable so quotes that require protocol fees won't work correctly #28

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

0x52

medium

Multiple functions aren't payable so quotes that require protocol fees won't work correctly

Summary

There are multiple functions that use quotes but that aren't payable. This breaks their compatibility with some quotes. As the 0x docs state: Certain quotes require a protocol fee, in ETH, to be attached to the swap call.

The following flows use a quote but the external/public starting function isn't payable:

RollerPeriphery 1) redeem

Periphery 1) removeLiquidity 2) combine 3) swapPT 4) swapYT 5) issue

Vulnerability Detail

See summary.

Impact

Functions won't be compatible with certain quotes causing wasted gas fees or bad rates for users

Code Snippet

https://github.com/sherlock-audit/2023-03-sense/blob/main/auto-roller/src/RollerPeriphery.sol#L104

https://github.com/sherlock-audit/2023-03-sense/blob/main/sense-v1/pkg/core/src/Periphery.sol#L325

https://github.com/sherlock-audit/2023-03-sense/blob/main/sense-v1/pkg/core/src/Periphery.sol#L409

https://github.com/sherlock-audit/2023-03-sense/blob/main/sense-v1/pkg/core/src/Periphery.sol#L433

https://github.com/sherlock-audit/2023-03-sense/blob/main/sense-v1/pkg/core/src/Periphery.sol#L240

https://github.com/sherlock-audit/2023-03-sense/blob/main/sense-v1/pkg/core/src/Periphery.sol#L263

Tool used

Manual Review

Recommendation

Add payable to these external/public functions

jparklev commented 1 year ago

Confirmed: We've forgotten to add payable to the functions mentioned

jparklev commented 1 year ago

Fixed here for sense-v1: https://github.com/sense-finance/sense-v1/pull/345 And here for the auto-roller: https://github.com/sense-finance/auto-roller/pull/33

We took the suggested fix and added payable to the mentioned functions

IAm0x52 commented 1 year ago

Fixes look good. Payable has been added to Periphery#removeLiquidity, combine, swapPT, swapYT and issue. Also adds payable to RollerPeriphery#redeem