c4-bot-1 commented 10 months ago

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/main/src/core/sentinel.cairo#L223

Vulnerability details

Impact

The current implementation of sentinel, allows to set sentinel.yang_asset_max arbitrary value for the yang. But while adding a yang, initial deposit of 1000 will be made. Which technically means that the yang token value can never be below 1000. But this lower bound check has not been imposed in the current implementation of set_yang_asset_max function.

Proof of Concept

        fn add_yang(...) {
            ...

            // Require an initial deposit when adding a yang to prevent first depositor from front-running
            let yang_erc20 = IERC20Dispatcher { contract_address: yang };
            // scale `asset_amt` up by the difference to match `Wad` precision of yang
            let initial_yang_amt: Wad = fixed_point_to_wad(INITIAL_DEPOSIT_AMT, yang_erc20.decimals());
            let initial_deposit_amt: u256 = INITIAL_DEPOSIT_AMT.into();

            let caller: ContractAddress = get_caller_address();
            let success: bool = yang_erc20.transfer_from(caller, gate.contract_address, initial_deposit_amt);
            assert(success, 'SE: Yang transfer failed');

            ...
        }

        fn assert_can_enter(self: @ContractState, yang: ContractAddress, gate: IGateDispatcher, enter_amt: u128) {
            ...
            let current_total: u128 = gate.get_total_assets();
            let max_amt: u128 = self.yang_asset_max.read(yang);
            assert(current_total + enter_amt <= max_amt, 'SE: Exceeds max amount allowed');
        }

Tools Used

Manual Review

Recommended Mitigation Steps

        fn set_yang_asset_max(ref self: ContractState, yang: ContractAddress, new_asset_max: u128) {
            self.access_control.assert_has_role(sentinel_roles::SET_YANG_ASSET_MAX);
+           assert(new_asset_max >= 1000, 'SE: Too Low');)

            let gate: IGateDispatcher = self.yang_to_gate.read(yang);
            assert(gate.contract_address.is_non_zero(), 'SE: Yang not added');

            let old_asset_max: u128 = self.yang_asset_max.read(yang);
            self.yang_asset_max.write(yang, new_asset_max);

            self.emit(YangAssetMaxUpdated { yang, old_max: old_asset_max, new_max: new_asset_max });
        }

Assessed type

Invalid Validation

c4-pre-sort commented 9 months ago

bytes032 marked the issue as insufficient quality report

bytes032 commented 9 months ago

QA

alex-ppg commented 9 months ago

The Warden has specified how the configuration of the maximum Yang (collateral) deposit limit is not sanitized. As its configuration is an administrative action, this submission cannot constitute an HM vulnerability and would instead be considered QA (NC) as a nice-to-have validation.

c4-judge commented 9 months ago

alex-ppg changed the severity to QA (Quality Assurance)