code-423n4 / 2022-12-Stealth-Project-findings

0 stars 0 forks source link

Approved user will be denied transfer liquidity #32

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/maverick-v1/contracts/models/Pool.sol#L87

Vulnerability details

Impact

It seems contract misses to also allow spender who are approved for all transactions by an owner. This will cause DOS for all these users

Proof of Concept

  1. Observe the checkPositionAccess function
function checkPositionAccess(uint256 tokenId) internal view {
        require(msg.sender == position.ownerOf(tokenId) || msg.sender == position.getApproved(tokenId), "P");
    }
  1. Observe that this does not check, isApprovedForAll(owner, spender) which means even if a spender is allowed for all, still he will be denied access to functions using checkPositionAccess (like transfer liquidity)

Recommended Mitigation Steps

Kindly revise the function as shown below:

function checkPositionAccess(uint256 tokenId) internal view {
        require(msg.sender == position.ownerOf(tokenId) || msg.sender == position.getApproved(tokenId) || isApprovedForAll(position.ownerOf(tokenId), msg.sender), "P");
    }
c4-judge commented 1 year ago

kirk-baird marked the issue as duplicate of #9

kirk-baird commented 1 year ago

For the same reasons as #9 I'm going to downgrade this to QA.

c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2022-12-Stealth-Project-findings/issues/33