LensHandle._hasTokenGuardianEnabled HAS STRICT EQUALITY THAT CAN BE MANIPULATED BY ATTACKERS

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/namespaces/LensHandles.sol#L256

Vulnerability details

Impact

function _hasTokenGuardianEnabled is using comparison that can be compromised because of it's strict behaviour.

Proof of Concept

Here is the highlighted vulnerable code

function _hasTokenGuardianEnabled(address wallet) internal view returns (bool) {
        return
            !wallet.isContract() &&
            (_tokenGuardianDisablingTimestamp[wallet] == 0 ||
                block.timestamp < _tokenGuardianDisablingTimestamp[wallet]);
    }

Possible Scenario How It Can Be Exploited

• Firstly, There is no check for Block.timestamp == comparison and in result the comparison will not revert if anything malicious happened because there should be a check for Block.timestamp == comparison. See Reference_By_Bot_Report_Of_This_Contest Here.
• Any malicious actor can manipulate block.timestamp to be earlier than the actual current time i.e block.timestamp < _tokenGuardianDisablingTimestamp[wallet]. It would evaluate to true even if the token guardian is still supposed to be active. This would allow any attacker to bypass the Guardian MEchanism and perform actions that should have been restricted.
• Above function is returning (without revert or require) the strict quality that wallet address should not be contract and concatenating OR Comparison with !wallet.isContract() in AND comparison.
• Although, No revert or require condition is used for this comparison which might lead to the functionality that it will not revert with a proper error.
• This Vulnerablity can be further exploited but due to time constraints couldn't manage it.

Tools Used

Manual Review / Static Code Analyzer

Recommended Mitigation Steps

Oracle can be used i.e chainlink oracle or using block.number because miners has less control over block.number compared to timestamp

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as primary issue

[141345]

seems exaggerate, at most QA no significant loss

[c4-sponsor]

donosonaumczuk marked the issue as sponsor disputed

[donosonaumczuk]

It does not apply, we are considering the specific timestamp to be the date where the change takes affect. Also, is not taking into account the context, where the secuirty period is 7 days already.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)