addressShares introduction made further shares accounting error prone

[code423n4]

Handle

hyh

Vulnerability details

Impact

Now both NFT ids and address have shares accounted to them, while the former is managed directly, the latter is managed on token transfer events. This way the only one sequence of operations stay correct.

Currently this sequence isn't enforced anyhow and can be easily messed up on further evolution of the system.

Proof of Concept

addressShares were introduces in the PR:

https://github.com/sherlock-protocol/sherlock-v2-core/pull/7/files

As it injects another layer of share accounting, the only correct sequence of actions now looks to be:

For shares addition:

stakeShares[_id] = stakeShares_;
totalStakeShares += stakeShares_;
_safeMint(_receiver, _id);

For shares removal:

_burn(_id);
_redeemShares (stakeShares[_id] and totalStakeShares decrease)

Any other combination will cause shares miscalculation, which leads directly to an exploitation of the system.

Recommended Mitigation Steps

Consider of moving this two pieces of logic, stakeStashes++ -> mint and burn -> stakeShares--, to separate functions to guarantee atomic execution in the right order all the time.

[Evert0x]

0 non criticial