c4-bot-7 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/main/src/core/allocator.cairo#L136-L167

Vulnerability details

Impact

According to the allocation module implementation, it is evident that the total percent should be equal to RAY. But this can be violated by incorrectly passing duplicate recipients. This either blocks distribution of equalizer assets or distributing less than intended.

Proof of Concept

    #[test]
    fn test_allocation_not_incorrect() {
        let allocator = equalizer_utils::allocator_deploy(
            equalizer_utils::initial_recipients(), equalizer_utils::initial_percentages(), Option::None
        );
        start_prank(CheatTarget::One(allocator.contract_address), shrine_utils::admin());

        let one: ContractAddress = contract_address_try_from_felt252('new recipient 1').unwrap();
        let two: ContractAddress = contract_address_try_from_felt252('new recipient 2').unwrap();
        let three: ContractAddress = contract_address_try_from_felt252('new recipient 3').unwrap();

        let recipients = array![
            one,
            two,
            three,
            one,
        ].span();

        let percentages = array![
            150000000000000000000000000_u128.into(), // 15% (Ray)
            300000000000000000000000000_u128.into(), // 30% (Ray)
            300000000000000000000000000_u128.into(), // 30% (Ray)
            250000000000000000000000000_u128.into(), // 25% (Ray)
        ].span();

        allocator.set_allocation(recipients, percentages);

        let (recipients_after, percentages_after) = allocator.get_allocation();
        assert(recipients_after.len() == 4, 'wrong len');
        assert(percentages_after.len() == 4, 'wrong len');
        stop_prank(CheatTarget::One(allocator.contract_address));
    }

Corresponding Percentages of recipients:

new recipient 1 => 25%
new recipient 2 => 30%
new recipient 3 => 30% So total is not 1 RAY.

Also while distribution this will be blocked as the distribution will be as follows:

new recipient 1 => 25%
new recipient 2 => 30%
new recipient 3 => 30%
new recipient 1 => 25% Which is more than 100% i.e 1 RAY.

Tools Used

Manual Review

Recommended Mitigation Steps

Enforce checks to validate no duplicates provided and the address is 0.

        fn set_allocation_helper(ref self: ContractState, recipients: Span<ContractAddress>, percentages: Span<Ray>) {
            let recipients_len: u32 = recipients.len();
            assert(recipients_len.is_non_zero(), 'AL: No recipients');
            assert(recipients_len == percentages.len(), 'AL: Array lengths mismatch');

            let mut total_percentage: Ray = RayZeroable::zero();
            let mut idx: u32 = LOOP_START;

            let mut recipients_copy = recipients;
            let mut percentages_copy = percentages;
            loop {
                match recipients_copy.pop_front() {
                    Option::Some(recipient) => {
+                       assert(*recipient.is_non_zero(), 'Recipient cannot be zero address');
                        self.recipients.write(idx, *recipient);

                        let percentage: Ray = *(percentages_copy.pop_front().unwrap());
+                       assert(self.percentages.read(*recipient).is_zero(), 'Percentage already added');

                        self.percentages.write(*recipient, percentage);

                        total_percentage += percentage;

                        idx += 1;
                    },
                    Option::None => { break; }
                };
            };

            assert(total_percentage == RAY_ONE.into(), 'AL: sum(percentages) != RAY_ONE');

            self.recipients_count.write(recipients_len);

            self.emit(AllocationUpdated { recipients, percentages });
        }

Assessed type

Invalid Validation

c4-pre-sort commented 7 months ago

bytes032 marked the issue as sufficient quality report

c4-pre-sort commented 7 months ago

bytes032 marked the issue as duplicate of #122

c4-judge commented 7 months ago

alex-ppg marked the issue as duplicate of #19