search

sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

imkapadia - Approved Operator can not call several functions #126

Closed sherlock-admin2 closed 8 months ago

sherlock-admin2 commented 8 months ago

imkapadia

high

Approved Operator can not call several functions

Summary

NFT position operator who is approved by NFT owner can not adjust position or close the position.

Vulnerability Detail

Functions like announceLeverageAdjust() and announceLeverageClose() of DelayedOrder contract and announceLimitOrder() of LimitOrder have validation checks that owner of tokenId is msg.sender or not. Since a position NFT is tradable/transferrable (as confirmed by developer), if NFT owner approves another user to operate their NFT by calling approve() or setApprovalForAll() and approved operator call mentioned functions it will always revert.

if (leverageModule.ownerOf(tokenId) != msg.sender) revert FlatcoinErrors.NotTokenOwner(tokenId, msg.sender);

Proof of Concept:

  1. Assume a scenario where a position NFT owner approves another user as an operator for their positions using the approve().
  2. The approved operator attempts to call the announceLeverageAdjust() function to adjust the leverage, but the function reverts as it does not recognize the operator's approval.

    Impact

    This issue limits the functionality of the announceLeverageAdjust(), announceLeverageClose() and announceLimitOrder() as it fails to recognize the approval status of operators. Due to this users have to suffer lose as position can't be adjusted/closed at desired time.

    Code Snippet

    https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L233 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L60 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L179-L183

    Tool used

Manual Review

Recommendation

Allow operators of token's owner to call functions on behalf of owner.

sherlock-admin commented 8 months ago

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

takarez commented:

invalid

nevillehuang commented 8 months ago

Invalid, this is a feature request. There is no core functionality broken here since users themselves can and are expected to manage their own positions

rashtrakoff commented 8 months ago

Assume a scenario where a position NFT owner approves another user as an operator for their positions using the approve().

This is not possible in the current system. If you mean approval of transfer of NFT then that means another user must first transfer this position NFT to themselves followed by any kind of order announcement.