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

20 stars 12 forks source link

NO CHECK TO VERIFY THE ELEMENTS OF `assetsAmounts[]` ARRAY IS IN THE SAME ORDER AS `assets[]` ARRAY, IF MISCONFIGURED COULD BREAK THE INTERNAL ACCOUNTING OF SHARE CALCULATION #867

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/erc-4626/ERC4626MultiToken.sol#L194-L208

Vulnerability details

Impact

In the ERC4626MultiToken.convertToShares() function, assetsAmounts are used to calculate the shares to mint. Here the assetsAmounts are expected to be passed in the order of the assets array. If there is any misconfiguration in the order, then it will affect the internal shares calculation since the weights[i] would be different for the respective assetsAmounts[i]. Hence the minted share amount could be erroneous which could impact the overall accounting of the protocol.

Proof of Concept

    function convertToShares(uint256[] calldata assetsAmounts) public view virtual returns (uint256 shares) {
        uint256 _totalWeights = totalWeights;
        uint256 length = assetsAmounts.length;

        if (length != assets.length) revert InvalidLength();

        shares = type(uint256).max;
        for (uint256 i = 0; i < length;) {
            uint256 share = assetsAmounts[i].mulDiv(_totalWeights, weights[i]);
            if (share < shares) shares = share;
            unchecked {
                i++;
            }
        }
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-4626/ERC4626MultiToken.sol#L194-L208

Tools Used

VSCode and Manual Review

Recommended Mitigation Steps

Hence it is recommended to implement a validation to verify that the elements of the assetsAmounts[] array are in the correct order as the assets[] array. This can be done by passing in a struct array when calling the ERC4626MultiToken.deposit() function. Each struct should include the address of the respective asset and the corresponding assetsAmounts. This struct array should be passed into the ERC4626MultiToken.convertToShares() function from the deposit function. Inside the convertToShares function inside the for loop it can be checked if each of the address of the asset is in the correct order as of asset[] array. If the order is different the transaction should revert.

Assessed type

Other

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Invalid