Although configuredTokenContracts pushed with new address onlyAdmin it's posible that array become too large and function where configuredTokenContracts.length used for loop will DoS.
Since admin in readme file not specified as a trusted role, it's possible that admin will be compromised or malicious.
Or it can be human error, and one extra token will be added to the array.
Function: setLockDuration(), getLocked(), getLockedWeightedValue() will be affected.
Proof of Concept
function configureToken(
address _tokenContract,
ConfiguredToken memory _tokenData
) external onlyAdmin {
if (_tokenData.nftCost == 0) revert NFTCostInvalidError();
if (configuredTokens[_tokenContract].nftCost == 0) {
// new token
//@> configuredTokenContracts.push(_tokenContract);
}
configuredTokens[_tokenContract] = _tokenData;
emit TokenConfigured(_tokenContract, _tokenData);
}
Lets assume that admin is NOT compromised or malicious.
configuredTokenContracts contains a lot of tokens.
All function works fine.
Admin add one extra token to the array.
gas is not enough to execute function or it's require more that half of block gas limit.
DoS of the function mentioned above or highly expensive execution.
Tools Used
manual review
Recommended Mitigation Steps
Implement pop() mechanism or limit configuredTokenContracts array.
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L22 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L115-L127
Vulnerability details
Impact
Although
configuredTokenContracts
pushed with new addressonlyAdmin
it's posible that array become too large and function whereconfiguredTokenContracts.length
used for loop will DoS. Sinceadmin
in readme file not specified as a trusted role, it's possible thatadmin
will be compromised or malicious. Or it can be human error, and one extra token will be added to the array. Function:setLockDuration()
,getLocked()
,getLockedWeightedValue()
will be affected.Proof of Concept
Lets assume that
admin
is NOT compromised or malicious.configuredTokenContracts
contains a lot of tokens.Tools Used
manual review
Recommended Mitigation Steps
Implement
pop()
mechanism or limitconfiguredTokenContracts
array.Assessed type
DoS