`absorber.set_reward` should calls `absorber.bestow`

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/absorber.cairo#L365-L389 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/absorber.cairo#L914-L975

Vulnerability details

Impact

absorber.set_reward can be used to updating a existing reward, but before updating, the function doesn't call bestow to claiming the reward in original blesser, in such case, if there's some reward in the original blesser, the reward will lost.

Proof of Concept

As show in code above, the absorber.set_reward function doesn't call absorber.bestow to claim the existing rewards before update an existing blesser.

        fn set_reward(ref self: ContractState, asset: ContractAddress, blesser: ContractAddress, is_active: bool) {
            self.access_control.assert_has_role(absorber_roles::SET_REWARD);

            assert(asset.is_non_zero() && blesser.is_non_zero(), 'ABS: Address cannot be 0');

            let reward: Reward = Reward { asset, blesser: IBlesserDispatcher { contract_address: blesser }, is_active };

            // If this reward token hasn't been added yet, add it to the list
            let reward_id: u8 = self.reward_id.read(asset);

            if reward_id == 0 {
                let current_count: u8 = self.rewards_count.read();
                let new_count = current_count + 1;

                self.rewards_count.write(new_count);
                self.reward_id.write(asset, new_count);
                self.rewards.write(new_count, reward);
            } else {
                // Otherwise, update the existing reward
                self.rewards.write(reward_id, reward);<<<--- self.bestow() should be called before this line
            }

            // Emit event
            self.emit(RewardSet { asset, blesser, is_active });
        }

Tools Used

VIM

Recommended Mitigation Steps

diff --git a/src/core/absorber.cairo b/src/core/absorber.cairo
index 473275f..8278488 100644
--- a/src/core/absorber.cairo
+++ b/src/core/absorber.cairo
@@ -381,6 +381,7 @@ mod absorber {
                 self.rewards.write(new_count, reward);
             } else {
                 // Otherwise, update the existing reward
+                self.bestow();
                 self.rewards.write(reward_id, reward);
             }

Assessed type

Other

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-sponsor]

tserg (sponsor) disputed

[tserg]

The Blesser should deal with how leftover tokens are to be handled.

[alex-ppg]

This particular exhibit concerns how an out-of-scope contract (IBlesser) will behave in a particular system flow when updating rewards.

I am inclined to align with the Sponsor's statement in this particular exhibit as it is logical for the Blesser (i.e. vesting contract) to be able to handle tokens that are not ultimately vested.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid