Canto contest - 0xDjango's results

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/NoteInterest.sol#L118-L129

Vulnerability details

Impact

The updateBaseRate() function is public and lacks access control, so anyone can set the critical variable baseRatePerYear once the block delta has surpassed the updateFrequency variable. This will have negative effects on the borrow and supply rates used anywhere else in the protocol.

The updateFrequency is explained to default to 24 hours per the comments, so this vulnerability will be available every day. Important to note, the admin can fix the baseRatePerYear by calling the admin-only _setBaseRatePerYear() function. However, calling this function does not set the lastUpdateBlock so users will still be able to change the rate back after the 24 hours waiting period from the previous change.

Proof of Concept

    function updateBaseRate(uint newBaseRatePerYear) public {
        // check the current block number
        uint blockNumber = block.number;
        uint deltaBlocks = blockNumber.sub(lastUpdateBlock);


        if (deltaBlocks > updateFrequency) {
            // pass in a base rate per year
            baseRatePerYear = newBaseRatePerYear;
            lastUpdateBlock = blockNumber;
            emit NewInterestParams(baseRatePerYear);
        }
    }

Tools Used

Manual review.

Recommended Mitigation Steps

I have trouble understanding the intention of this function. It appears that the rate should only be able to be set by the admin, so the _setBaseRatePerYear() function seems sufficient. Otherwise, add access control for only trusted parties.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/WETH.sol#L85-L101

Vulnerability details

Impact

The approval logic is incorrect in WETH.sol. Given that there are no references to msg.sender, the approval parameters are simply inputs. This means that an attacker can set approvals on anyone's tokens.

Proof of Concept

An attacker calls:

approve(tokenOwner, attacker);

This calls:

_approve(tokenOwner, attacker, _balanceOf[tokenOwner]);

Which sets the allowance for the attacker to spend the owner's entire balance:

_allowance[tokenOwner][attacker] = amount;

Tools Used

Manual review.

Recommended Mitigation Steps

The top-level approve() function needs to take msg.sender as the owner parameter so that the owner is not set arbitrarily.

[L-01] It's not possible for msg.sender to equal address(0)

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L164

[N-01] Add to in mint() event emission similar to burn()

https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L257

[N-02] Consants should be all caps

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L9-L12

[N-03] Interfaces should start with "I"

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L30

[N-04] External/public functions should not start with "_". Reserved for internal/private functions.

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L162 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Accountant/AccountantDelegator.sol#L42 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Treasury/TreasuryDelegator.sol#L29

[N-05] Extra space in parameters

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Accountant/AccountantDelegator.sol#L54