0x52 - Multiple functions may leave excess funds in the contract that should be returned

[sherlock-admin]

0x52

medium

Multiple functions may leave excess funds in the contract that should be returned

Summary

Periphery#combine may leave excess underlying in the contract due to _fromTarget unwrapping to underlying and the quote may not swap them all.

When using arbitrary tokens to swap to underlying the contract always moves in the full amount specified. There is no guarantee that the quote will consume all tokens. As a result the contract may leave excess sell tokens in the contract but it should return then to the receiver.

These functions include:

RollerPeriphery 1) deposit

Periphery 1) swapForPTs 2) addLiquidity 3) issue

RollerPeriphery#RollermintFromUnderlying uses adapter.scale and previewMint to determine the amount of underlying to transfer. The roller code will mean that previewMint will always perfectly reflect the exact exchange rate into the roller. However adapter.scale varies by adapter and isn't guaranteed to be exact. The result is that _transferFrom may take too much underlying. Since this underlying is wrapped to target the contract should return all excess target to receiver.

Vulnerability Detail

See summary.

Impact

Token may be left in the contract and lost

Code Snippet

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

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

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

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

Tool used

Manual Review

Recommendation

Return excess tokens at the end of the function

[jparklev]

We have this feature, for example, on _swapSenseToken, but not on the cases mentioned.

Our fix will be: Transfer non-used tokens back to the user.

[jparklev]

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

We returned excess sellTokens funds in the from and to target functions

[IAm0x52]

Fixes look good. Excess sell tokens are now returned inside _toTarget and _fromTarget for both periphery and rollerPeriphery