osmanozdemir1 - Anyone who has allowance can transfer LPs even if they are not approved transferors.

[sherlock-admin]

osmanozdemir1

high

Anyone who has allowance can transfer LPs even if they are not approved transferors.

Summary

transferLP method in the LPActions.sol library checks if the new owner is msg.sender and if they are approved transferors, but anyone can approve themselves as a transferor and pass the checks.

Vulnerability Detail

The transferLP method in the LPActions.sol library takes 6 parameters where three of which are storage variables of the pool and the other three are ownerAddress_, newOwnerAddress_ and indexes_. The method checks the given parameters with this statement:

// revert if msg.sender is not the new owner and is not approved as a transferor by the new owner
if (newOwnerAddress_ != msg.sender && !approvedTransferors_[newOwnerAddress_][msg.sender]) revert TransferorNotApproved();

The method expects the newOwnerAddress is the msg.sender and this method should be called by the new owner. If this statement is true, the method also expects approvedTransferors_[newOwnerAddress_][msg.sender] to be true too. Otherwise, it will revert.
If the new owner is msg.sender, this means that the second check is actually approvedTransferors_[msg.sender][msg.sender]. So, anyone can approve themselves as a transferor, call this function by inputting their address as the new owner and pass both of these checks.

The intention is to check if the msg.sender is approved as a transferor by the owner of the LPs, and revert if not approved. It should be approvedTransferors_[ownerAddress][msg.sender] or approvedTransferors_[ownerAddress][newOwnerAddress_]

I acknowledge that there is another check in the method to get the allowance amount. If someone has an allowance but is not approved as a transferor, that person can transfer the LPs even if they are not approved.

Impact

Anyone can approve themselves as an approved transferor, initiate the transferLP function by passing their own address as the new owner, and pass the checks.

Code Snippet

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/libraries/external/LPActions.sol#L217-L218

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/libraries/external/LPActions.sol#L156-L162

Tool used

Manual Review

Recommendation

I recommend changing this !approvedTransferors_[newOwnerAddress_][msg.sender] to this: !approvedTransferors_[ownerAddress][msg.sender]

[grandizzy]

the mechanism for transfer LP is

• owner approves allowance to new owner
• new owner transfer LP by himself or approve another address to execute the transfer The rationale is that transfer should be executed only by the new owner or addresses approved by new owner and avoid spamming deposits by unwanted LP transfers

for reference see https://github.com/sherlock-audit/2023-01-ajna-judging/issues/156

[0xffff11]

As discussed with sponsor currently the way on how transferLP works is the intended behaviour as the approach was changed according to the previous contest https://github.com/sherlock-audit/2023-01-ajna-judging/issues/156 Invalid