panprog - PositionManager will revert when trying to return back to user excess of the premium transferred from the user when minting position

[sherlock-admin2]

panprog

medium

PositionManager will revert when trying to return back to user excess of the premium transferred from the user when minting position

Summary

PositionManager.mint calculates preliminary premium to be paid for buying the option and transfers it from the user. The actual premium paid may differ, and if it's smaller, excess is returned back to user. However, it is returned using the safeTransferFrom:

    if (obtainedPremium > premium) {
        baseToken.safeTransferFrom(address(this), msg.sender, obtainedPremium - premium);
    }

The problem is that PositionManager doesn't approve itself to transfer baseToken to msg.sender, and USDC transferFrom implementation requires approval even if address is transferring from its own address. Thus the transfer will revert and user will be unable to open position.

Vulnerability Detail

Both transferFrom implementations in USDC on Arbitrum (USDC and USDC.e) require approval from any address, including when doing transfers from your own address. https://arbiscan.io/address/0x1efb3f88bc88f03fd1804a5c53b7141bbef5ded8#code

    function transferFrom(address sender, address recipient, uint256 amount) public virtual override returns (bool) {
        _transfer(sender, recipient, amount);
        _approve(sender, _msgSender(), _allowances[sender][_msgSender()].sub(amount, "ERC20: transfer amount exceeds allowance"));
        return true;
    }

https://arbiscan.io/address/0x86e721b43d4ecfa71119dd38c0f938a75fdb57b3#code

    function transferFrom(
        address from,
        address to,
        uint256 value
    )
        external
        override
        whenNotPaused
        notBlacklisted(msg.sender)
        notBlacklisted(from)
        notBlacklisted(to)
        returns (bool)
    {
        require(
            value <= allowed[from][msg.sender],
            "ERC20: transfer amount exceeds allowance"
        );
        _transfer(from, to, value);
        allowed[from][msg.sender] = allowed[from][msg.sender].sub(value);
        return true;
    }

PositionManager doesn't approve itself to do transfers anywhere, so baseToken.safeTransferFrom(address(this), msg.sender, obtainedPremium - premium); will always revert, preventing the user from opening position via PositionManager, breaking important protocol function.

Impact

User is unable to open positions via PositionManager in certain situations as all such transactions will revert, breaking important protocol functionality and potentially losing user funds / profit due to failure to open position.

Code Snippet

PositionManager.mint transfers base token back to msg.sender via safeTransferFrom: https://github.com/sherlock-audit/2024-02-smilee-finance/blob/main/smilee-v2-contracts/src/periphery/PositionManager.sol#L139-L141

Tool used

Manual Review

Recommendation

Consider using safeTransfer instead of safeTransferFrom when transferring token from self.

[sherlock-admin2]

2 comment(s) were left on this issue during the judging contest.

santipu_ commented:

Invalid - code will never execute bc actual premium is always >= obtainedPremium due to using the worst price between oracle and swap.

takarez commented:

valid, medium(2)

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/dverso/smilee-v2-contracts/commit/84174d20544970309c862a2bf35ccfa3046d6bd9.

[panprog]

Fix review: Fixed

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.

[santipu03]

Escalate

This issue is LOW because the bug will never be triggered.

PositionManager calculates the variable obtainedPremium calling DVP.premium, that uses the oracle price to calculate the premium:

https://github.com/sherlock-audit/2024-02-smilee-finance/blob/main/smilee-v2-contracts/src/IG.sol#L104

premium_ = _getMarketValue(financeParameters.currentStrike, amount_, true, price);

Later, when minting the position, the actual premium is calculated on DVP._mint, using the oracle price and the swap price:

https://github.com/sherlock-audit/2024-02-smilee-finance/blob/main/smilee-v2-contracts/src/DVP.sol#L155

uint256 swapPrice = _deltaHedgePosition(strike, amount, true);
uint256 premiumOrac = _getMarketValue(strike, amount, true, IPriceOracle(_getPriceOracle()).getPrice(sideToken, baseToken));
uint256 premiumSwap = _getMarketValue(strike, amount, true, swapPrice);
premium_ = premiumSwap > premiumOrac ? premiumSwap : premiumOrac;

The actual premium to pay will be the result of the higher value between the swap-priced premium and the oracle-priced premium. So, in the scenario where the swap price is lower than the oracle price, the oracle price will be used to compute the final premium.

Therefore, it's not possible that the final premium is a lower value than obtainedPremium, so the bug will never be triggered because the condition obtainedPremium > premium will never be true.

[sherlock-admin2]

Escalate

This issue is LOW because the bug will never be triggered.

PositionManager calculates the variable obtainedPremium calling DVP.premium, that uses the oracle price to calculate the premium:

https://github.com/sherlock-audit/2024-02-smilee-finance/blob/main/smilee-v2-contracts/src/IG.sol#L104

premium_ = _getMarketValue(financeParameters.currentStrike, amount_, true, price);

Later, when minting the position, the actual premium is calculated on DVP._mint, using the oracle price and the swap price:

https://github.com/sherlock-audit/2024-02-smilee-finance/blob/main/smilee-v2-contracts/src/DVP.sol#L155

uint256 swapPrice = _deltaHedgePosition(strike, amount, true);
uint256 premiumOrac = _getMarketValue(strike, amount, true, IPriceOracle(_getPriceOracle()).getPrice(sideToken, baseToken));
uint256 premiumSwap = _getMarketValue(strike, amount, true, swapPrice);
premium_ = premiumSwap > premiumOrac ? premiumSwap : premiumOrac;

The actual premium to pay will be the result of the higher value between the swap-priced premium and the oracle-priced premium. So, in the scenario where the swap price is lower than the oracle price, the oracle price will be used to compute the final premium.

Therefore, it's not possible that the final premium is a lower value than obtainedPremium, so the bug will never be triggered because the condition obtainedPremium > premium will never be true.

You've created a valid escalation!

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.

[panprog]

Yes, the way it is now, it can never actually happen but that's only due to another issue which makes it revert if any DEX slippage occurs. When it works as intended, the remainder has to be transferred back to the sender.

So yeah, up to judge to decide. I consider it borderline, because if we isolate "how it is supposed to work", returning back the remainder should be possible, it's impossible only due to a different issue.

[cvetanovv]

It seems that the report is invalid. I think we should judge according to the code that is at the moment, not if it works properly.

[nevillehuang]

Based on head of judging comments in a previous contest for such issues, I believe this should remain valid.

[Czar102]

I agree with @panprog and @nevillehuang – the fix to the other issue will not fix this one. Hence, these are separate issues and both of them may be valid since they present discrepancies from the expected behavior of the contracts.

I'm planning to reject the escalation and leave the issue as is.

[Czar102]

Result: Medium Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• santipu03: rejected