3th - Incorrect position ownership check throughout abstract staking asset module

[sherlock-admin2]

3th

medium

Incorrect position ownership check throughout abstract staking asset module

Summary

Throughout AbstractStakingAM, functionality related to the management of staking positions is authenticated by comparing the owner of the relevant position ID with msg.sender. However, users will be managing their positions through AccountV1.flashAction, so msg.sender will actually be MultiCall.

Vulnerability Detail

The functions increaseLiquidity, decreaseLiquidity, and claimReward all require that the calling account owns the position it is attempting to operate on. This would only be the case if the user was calling the staking asset module directly — otherwise, msg.sender will be the Action Handler that sits between AccountV1 and the asset modules it interacts with.

Impact

While this would prevent most users from interacting with any asset module that inherits AbstractStakingAM, it is actually possible to side-step this issue by withdrawing and then depositing the position NFT with every call to any of the above-listed functions (by specifying both actions in ActionData structures provided to flashAction).

It seems unlikely that this was the intended process for interacting with the staking asset module, however, since each unnecessary transfer adds risk and provides no reward. As such, the severity of this issue is marked as "medium" to reflect that much of the functionality is made unusable in its intended way by this bug, despite the fact that a workaround is possible.

Code Snippet

• AbstractStakingAM.increaseLiquidity
• AbstractStakingAM.decreaseLiquidity
• AbstractStakingAM.claimReward

Tool used

Manual Review

Recommendation

If the Arcadia account calling MultiCall were to be passed along to the asset module (either as address(this) from AccountV1 or msg.sender in MultiCall, so it cannot be manipulated), these functions could check that the provided address owns the relevant position without requiring unnecessary transfers (which should be avoided, since these positions can store huge values).

It is worth noting that this would require changes to the accessibility of these functions, since any user could provide an arbitrary address as a parameter and pretend to be the owner of its positions. This would be best handled by requiring that msg.sender is an approved MultiCall, which could be tracked by the Registry.

Duplicate of #1

[sherlock-admin2]

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

takarez commented:

invalid

[nevillehuang]

Invalid, agree with sponsors comment:

As designed: all tokens will be deposited/withdrawn to the actionhandler if they need to claim. Similar for uniswap V3 positions that need to have their liquidity changed.