code-423n4 / 2023-05-ajna-findings

2 stars 0 forks source link

Use of msg.sender in mayInteract Modifier #414

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L98-L110

Vulnerability details

Impact

The use of msg.sender in the mayInteract modifier in the contract. I have identified a potential vulnerability in the use of msg.sender. In this case, the vulnerability is caused by using msg.sender to authorize interactions with the contract. Since msg.sender can be set by an attacker, it is possible for a malicious contract to exploit this vulnerability and call the vulnerable contract in a reentrant manner. This can allow the attacker to execute additional code in the vulnerable contract's context, potentially accessing sensitive data or manipulating contract state.

Proof of Concept

In the mayInteract modifier of the msg.sender is used to check if the caller of the function is authorized to interact with the contract. However, msg.sender can be set by an attacker to the address of a malicious smart contract. This can potentially allow the attacker to perform a reentrancy attack on the contract.

mayInteract modifier

    modifier mayInteract(address pool_, uint256 tokenId_) {

        // revert if token id is not a valid / minted id
        _requireMinted(tokenId_);

        // revert if sender is not owner of or entitled to operate on token id
        if (!_isApprovedOrOwner(msg.sender, tokenId_)) revert NoAuth();

        // revert if the token id is not minted for given pool address
        if (pool_ != poolKey[tokenId_]) revert WrongPool();

        _;
    }

The mayInteract modifier uses msg.sender to check if the caller of the function is authorized to interact with the contract. However, msg.sender can be manipulated by an attacker, allowing them to potentially perform a reentrancy attack on the contract.

Tools Used

Manual review, vscode

Recommended Mitigation Steps

Use tx.origin instead of msg.sender for authorization checks. tx.origin always represents the address that initiated the transaction, even if the transaction was initiated by a contract, this ensures that the original sender's address is always used for authorization checks, preventing an attacker from manipulating the msg.sender value.

Assessed type

Access Control

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid