Unbounded assets[] array length could cause DoS

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L300-L305
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L50-L53
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L64-L67
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L76-L78
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L89-L95

Vulnerability details

Impact

It's possible to render the GovNFT.sol contract inoperable due to DoS. All the mint()/_bridgeMint()/_burn()/_transfer() could fail to function.

Proof of Concept

There is no upper limit for the assets[] array, and there is no way to decrease the length of the array. If the length of it grow too large, it is possible that in the loop when iterating all the elements, the gas required exceeds the block gas limit and cause DoS.

There is no limit on the size on the array when add new asset into it. But no way to remove items.

File: contracts/GovNFT.sol
300:     function addAsset(address _asset) external onlyOwner {
301:         require(assets.length == 0 || assets[assetsIndex[_asset]] != _asset, "Already added");
302:         assetsIndex[_asset] = assets.length;
303:         assets.push(_asset);
304:         _allowedAsset[_asset] = true;
305:     }

In mint()/_bridgeMint()/_burn()/_transfer(), the entire assets[] array will be iterated. So if the size become too large after a long time, the contract could be inoperable.

50:     function _mint(address to, uint256 tokenId) internal override {

53:         for (uint i=0; i<assetsLength(); i++) {


64:     function _bridgeMint(address to, uint256 tokenId) public {

67:         for (uint i=0; i<assetsLength(); i++) {


76:     function _burn(uint256 tokenId) internal override {

78:         for (uint i=0; i<assetsLength(); i++) {

89:     function _transfer() internal override {

95:         for (uint i=0; i<assetsLength(); i++) {

Tools Used

Manual analysis.

Recommended Mitigation Steps

Set a maximum length for assets[] array.

[GalloDaSballo]

Missing example and math, but is probably valid

[c4-judge]

GalloDaSballo marked the issue as duplicate of #24

[c4-judge]

GalloDaSballo marked the issue as duplicate of #377

[c4-judge]

GalloDaSballo marked the issue as satisfactory