code-423n4 / 2022-08-frax-findings

2 stars 1 forks source link

Unbounded loop while iterating `deployedPairsArray` #340

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairDeployer.sol#L122-L134 https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairDeployer.sol#L255

Vulnerability details

Impact

If deployedPairsArray has a large amount of items, calls to getAllPairAddresses() can result in a out of gas scenario, which would result in a DoS condition while retrieving the addresses.

Proof of Concept

  1. A large amount of items are pushed into deployedPairsArray
File: contracts/FraxlendPairDeployer.sol
255: deployedPairsArray.push(_name);

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairDeployer.sol#L255

  1. The loop inside getAllPairAddresses will run out of gas and the addresses are not going to be available to external calls.
File: contracts/FraxlendPairDeployer.sol
function getAllPairAddresses() external view returns (address[] memory) {
    string[] memory _deployedPairsArray = deployedPairsArray;
    uint256 _lengthOfArray = _deployedPairsArray.length;
    address[] memory _addresses = new address[](_lengthOfArray);
    uint256 i;
    for (i = 0; i < _lengthOfArray; ) {
        _addresses[i] = deployedPairsByName[_deployedPairsArray[i]];
        unchecked {
            i++;
        }
    }
    return _addresses;
}

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairDeployer.sol#L122-L134

Recommended Mitigation Steps

Consider modifying the getAllPairAddresses() function to accept arguments that enable pagination while iterating deployedPairsArray. E.g. adding a fromIndex and a toIndex arguments.

DrakeEvans commented 2 years ago

This is a known issue, getAllPairAddresses() is a convenience function for the UI. Same data is accessible via other functions which take an index argument or by reading the events.

gititGoro commented 1 year ago

No functions in the protocol rely on getAllPairAddresses for operation and so no unbounded loop consuming all gas situation can occur in the scope of this project. This function does not constitute conforming to a standard such as an EIP so no downstream protocols are unwittingly at risk for relying on this function. It would be their responsibility to read the source code of this project. Marking invalid.