search

code-423n4 / 2021-11-unlock-findings

0 stars 0 forks source link

Refund mechanism doesn't take into account that key price can change #53

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

kenzo

Vulnerability details

Lock manager can change key pricing. The refund mechanism calculates refund according to current key price, not price actually paid.

Impact

A user refunding can get less (or more) funds than deserved.

Proof of Concept

Refund only takes the current price into account: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinRefunds.sol#L144:#L152 Lock manager can update key price at any point, and the old price is not saved anywhere: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinLockCore.sol#L183 So if for example a key price has gone down, a user who tried to refund will get less funds than deserved.

Recommended Mitigation Steps

Consider saving the amount the user paid, and refund according to that. Or having a kind of a price snapshot/version mechanism.

julien51 commented 3 years ago

This is a known issue... but indeed we should show things in the UI to indicate things to users.

0xleastwood commented 2 years ago

Agree this sounds like an issue! However, I don't think this can be justified as a high risk issue. But it does seem that the protocol could leak value and impact users, so marking this as medium.

julien51 commented 2 years ago

We actually are adding a new mechanism to keep track of the last price paid by any user which means we could use it in the next version to solve this issue!