code-423n4 / 2023-05-maia-findings

24 stars 13 forks source link

Id not saved when adding a vault with `addVault` or partner with `addPartner` #808

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/factories/PartnerManagerFactory.sol#L58-L64 https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/factories/PartnerManagerFactory.sol#L67-L73

Vulnerability details

Impact

In the PartnerManagerFactory contract when adding a new vault with the addVault or adding a new partner with addPartner function, both functions does not save the id of the added vault (or partner), this will cause function like migratePartnerVault to revert when they are not supposed to.

Proof of Concept

The issue occurs in both addVault and addPartner functions below :

File: PartnerManagerFactory.sol Line 58-64

function addPartner(PartnerManager newPartnerManager) external onlyOwner {
    uint256 id = partners.length;
    partners.push(newPartnerManager);
    // @audit not saving the new partner id
    partnerIds[newPartnerManager] == id;

    emit AddedPartner(newPartnerManager, id);
}

File: PartnerManagerFactory.sol Line 67-73

function addVault(IBaseVault newVault) external onlyOwner {
    uint256 id = vaults.length;
    vaults.push(newVault);
    // @audit not saving the new vault id
    vaultIds[newVault] == id;

    emit AddedVault(newVault, id);
}

As you can see when trying to save the id of the new vault or partner, the functions use the comparison operator == instead of the assignement operator =.

So both functions will just compare partnerIds[newPartnerManager] or vaultIds[newVault] with the id value, and regardless of the result (true or false) the functions call will succeed (will not revert) and thus any added vault or partner will have the dafault id of 0.

This will cause problems for the protocol functions who rely on those ids, for example the function migratePartnerVault in ERC4626PartnerManager contains the following check statement :

File: ERC4626PartnerManager.sol Line 189

if (factory.vaultIds(IBaseVault(newPartnerVault)) == 0) revert UnrecognizedVault();

Because of the metioned error, all the added vaults will have an id equal to 0 and so the function migratePartnerVault will always revert making it impossible to migrate assets to new Partner Vault.

Tools Used

Manual review

Recommended Mitigation Steps

Correct the error in both addVault and addPartner functions by using = instead of == :

function addPartner(PartnerManager newPartnerManager) external onlyOwner {
    uint256 id = partners.length;
    partners.push(newPartnerManager);
    // @audit save the new partner id
    partnerIds[newPartnerManager] = id;

    emit AddedPartner(newPartnerManager, id);
}
function addVault(IBaseVault newVault) external onlyOwner {
    uint256 id = vaults.length;
    vaults.push(newVault);
    // @audit save the new vault id
    vaultIds[newVault] = id;

    emit AddedVault(newVault, id);
}

Assessed type

Error

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Invalid