it's possible that two different tokens assigned the same assetId in `ReserveStorage::setReserveConfig` function

zlsecure3 commented 1 year ago

subject

it's possible that two different tokens assigned the same assetId in ReserveStorage::setReserveConfig function

description

If two different tokens are assigned the same assetId(e.g, btc and doge are assigned the same assetId), serious result may occur(e.g, one can deposit doge as btc).

But current implementation doesn't check against this situation, it's quite trivial to add it back.

change:

        uint32 registeredId = reserveIdMap[tokenAddress];
        DataTypes.ReserveConfigMap memory reserveConfig = reserveConfigs[
            assetId
        ].data;
        if (registeredId != 0) {
            require(assetId == registeredId);
        } else {
            ...
        }

to

        uint32 registeredId = reserveIdMap[tokenAddress];
        DataTypes.ReserveData memory reserveConfigData = reserveConfigs[
            assetId
        ];
        DataTypes.ReserveConfigMap memory reserveConfig = reserveConfigData.data;
        if (registeredId != 0) {
            require(assetId == registeredId);
            // ensure this assetId hasn't been assigned to another tokenAddress
            require(tokenAddress == reserveConfigData.tokenAddress);
        } else {
          // assetId can't be 0
           require(assetId != 0);
            ...
        }

The above also ensures that assetId can't be 0, which current implementation misses checking in the else branch.

recommendation

Apply the above fix.

locations

code/contracts/core/storages/ReserveStorage.sol#L100-L115

severity

Medium

damage

exploitability

grading (edit)

submission_id:1761909491

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:1761909491

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:1761909491

review_type:CLIENT_FEEDBACK_DECISION

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

severity: TBD-Critical,Medium,Low,Informational

comment:

zlsecure3 / review_Aark