bitsurfer - `enrollCourier` lack of share check open for user owning share to loss their reward

[sherlock-admin2]

bitsurfer

medium

enrollCourier lack of share check open for user owning share to loss their reward

Summary

User's reward may lost due to change status from a regular user (or address) to courier

Vulnerability Detail

by design, couriers cannot claim rewards due to accounting issue, as described in the following function:

File: Factory.sol
228:     function claimRewards(Lender[] calldata lenders, address beneficiary) external returns (uint256 earned) {
229:         // Couriers cannot claim rewards because the accounting isn't quite correct for them. Specifically, we
230:         // save gas by omitting a `Rewards.updateUserState` call for the courier in `Lender._burn`
231:         require(!isCourier[msg.sender]);
232: 
233:         unchecked {
234:             uint256 count = lenders.length;
235:             for (uint256 i = 0; i < count; i++) {
236:                 // Make sure it is, in fact, a `Lender`
237:                 require(peer[address(lenders[i])] != address(0));
238:                 earned += lenders[i].claimRewards(msg.sender);
239:             }
240:         }
241: 
242:         rewardsToken.safeTransfer(beneficiary, earned);
243:     }

but in reality, there is open for possibility for a user (or address) who holds shares at somepoint intend to be a couriers. Noted that when they become a courier they can't claim rewards due to accounting issue. But, since they holds a share, before enrolling as Courier, they need be a way to ensure they already claim their rewards. This may seems like a user's mistakes not 'claimRewards' before

File: Factory.sol
254:     function enrollCourier(uint32 id, uint16 cut) external {
255:         // Requirements:
256:         // - `id != 0` because 0 is reserved as the no-courier case
257:         // - `cut != 0 && cut < 10_000` just means between 0 and 100%
258:         require(id != 0 && cut != 0 && cut < 10_000);
259:         // Once an `id` has been enrolled, its info can't be changed
260:         require(couriers[id].cut == 0);
261: 
262:         couriers[id] = Courier(msg.sender, cut);
263:         isCourier[msg.sender] = true;
264: 
265:         emit EnrollCourier(id, msg.sender, cut);
266:     }

In short, when performing enrollCourier, the function need to check whether the proposed address holds some shares, if yes, then need to return their reward first before finally assign them as Courier.

Impact

User's reward may lost due to change status from a regular user (or address) to courier

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Factory.sol#L254-L266

Tool used

Manual Review

Recommendation

Add a check on enrollCourier if they have shares, they reward need to be distributed first before changing their status to be a courier

File: Factory.sol
254:     function enrollCourier(uint32 id, uint16 cut) external {

++           // check if they have shares
++           // claim their reward

255:         // Requirements:
256:         // - `id != 0` because 0 is reserved as the no-courier case
257:         // - `cut != 0 && cut < 10_000` just means between 0 and 100%
258:         require(id != 0 && cut != 0 && cut < 10_000);
259:         // Once an `id` has been enrolled, its info can't be changed
260:         require(couriers[id].cut == 0);
261: 
262:         couriers[id] = Courier(msg.sender, cut);
263:         isCourier[msg.sender] = true;
264: 
265:         emit EnrollCourier(id, msg.sender, cut);
266:     }

Duplicate of #134

[sherlock-admin2]

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

panprog commented:

low, because this is the developers choice and the issue of courier being unable to claim rewards is documented and known beforehand, so if user is enrolled as courier he is assumed to know the possible problem

MohammedRizwan commented:

low severity as its a user mistake and user must understand before calling any function