observer design is very gas in-efficiency

zlsecure3 commented 1 year ago

subject

description

observer take the push way to write all the cache data into various contracts, such as vault, lpManager etc. this method has some advantage, for example, when u want to change the lpManager address, you can just call the setLpManager function, and it will update the new lpManager address into all the related contracts. however, it is especially gas costly during the initialize process. consider the following schema: after deploy, when initialize:

call setFuturesManager, the futureManager cache data for 1st time
call setLpManager, the futureManager cache data for 2nd time, lpManager cache data for 1st time
call setPriceOracle, the futureManager cache data for 3rd time, lpManager cache data for 2nd time, priceOracle cache data for 1st time
call setVault, the futureManager cache data for 4th time, lpManager cache data for 3rd time, priceOracle cache data for 2nd time, vault cache data for 1st time
call setMasterRouter, the futureManager cache data for 5th time, lpManager cache data for 4th time, priceOracle cache data for 3rd time, vault cache data for 2nd time, masterRouter cache data for 1st time
call setInsuranceManager, the futureManager cache data for 6th time, lpManager cache data for 5th time, priceOracle cache data for 4th time, vault cache data for 3rd time, masterRouter cache data for 2nd time, sethInsuranceManager cache data for 1st time

recommendation

set the corresponding address, such as vault, lpManager, futuresManager, masterRouter, insuranceManager, priceOracle inside the initialize function, to avoid the recursive cache data. you may refer to the following modification:

function initialize(address WETH_, ...) public reinitializer(1) {
        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
        WETH = WETH_;

        futuresManager = futuresManager_;
        emit SetManager(futuresManager_);
        lpManager = lpManager_;
        emit SetLpPool(lpManager_);
        priceOracle = priceOracleAddress_;
        emit SetPriceOracle(priceOracleAddress_);
        vault = vault_;
        emit SetVault(vault_);
        masterRouter = masterRouter_;
        emit SetMasterRouter(masterRouter_);
       ...
    }

locations

code/contracts/core/common/Observer.sol#L40

severity

Low

damage

exploitability

grading (edit)

submission_id:1765494217

review_type:GRADING

result: TBD-yes,no

rating: TBD-123

comment: TBD-Rejected,Accepted by Secure3.

severity: TBD-Critical,Medium,Low,Informational

category:

description:

zlsecure3 commented 1 year ago

client feedback (manual copy)

submission_id:1765494217

review_type:CLIENT_FEEDBACK

result: TBD-yes,no

severity: TBD-Critical,Medium,Low,Informational

comment:

zlsecure3 commented 1 year ago

client feedback decision(edit)

submission_id:1765494217

review_type:CLIENT_FEEDBACK_DECISION

result: TBD-yes,no,yes-honored,no-honored

severity: TBD-Critical,Medium,Low,Informational

comment:

zlsecure3 / review_Aark