Canto contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

updateBaseRate is marked public and can be called by any user to update the base rate as per their choice Attacker can simply call this function to change the base rate per year to incur more profits

Proof of Concept

1. Observe that updateBaseRate places no restriction and is marked public

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);
        }
    }

Recommended Mitigation Steps

Add below check so that only Admin can call this function

require(msg.sender == admin, "only the admin may set the update frequency");

Lines of code

https://github.com/Plex-Engineer/manifest/blob/688e9b4e7835854c22ef44b045d6d226b784b4b8/contracts/Proposal-Store.sol#L46

Vulnerability details

Impact

On calling AddProposal function with same propId , the older proposal will get overridden. This can become a problem. User can see a proposal and approve it. But before proposal is queue/executed, user can change it using same proposal id

Proof of Concept

1. Malicious user creates a genuine proposal id 1
2. User approves this proposal
3. queue is called on this proposal using queue function at GovernorBravoDelegate.sol#L37
4. Admin who is watching mempool, frontrun this transaction and calls AddProposal function again with same proposal id 1 but with malicious parameters
5. Now user request completes which queue a malicious proposal unknowingly
6. Once timelock completes the malicious proposal gets executed

Recommended Mitigation Steps

If proposal with same id is already existing then AddProposal function should revert

Lines of code

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

Vulnerability details

Impact

The queue function marks the transaction execution as true which is incorrect. newProposal.executed should not be set to true as this transaction could never be executed since state(proposalId) will not be equal to ProposalState.Queued, thus failing at execute function

Proof of Concept

1. Queue function is called on proposal id 1
2. This sets proposals[unigovProposal.id].executed = true in GovernorBravoDelegate.sol#L63
3. Once timelock is over execute function is called
4. Execute function fails since state(proposalId) == ProposalState.Queued is not true

Recommended Mitigation Steps

Change GovernorBravoDelegate.sol#L63 to

newProposal.executed = false;

Lines of code

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

Vulnerability details

Impact

It was observed that in repayBorrowFresh function, User is asked to send repayAmount instead of repayAmountFinal. This can lead to loss of user funds as user might be paying extra

Proof of Concept

1. User is making a repayment which eventually calls repayBorrowFresh function

2. Assuming repayAmount == type(uint).max, so repayAmountFinal becomes accountBorrowsPrev

3. This means User should only transfer in accountBorrowsPrev instead of repayAmount but that is not true. Contract is transferring repayAmount instead of repayAmountFinal as seen at CNote.sol#L129

uint actualRepayAmount = doTransferIn(payer, repayAmount);

Recommended Mitigation Steps

Revise CNote.sol#L129 to below:

uint actualRepayAmount = doTransferIn(payer, repayAmountFinal);

Lines of code

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

Vulnerability details

Impact

Admin can set a very high value of update frequency which will make sure that updateBaseRate can be never be called. This way only Admin would be sole decider of base rate using _setBaseRatePerYear

Proof of Concept

1. Observe that Admin can set a very large block.number value for update frequency using _setUpdateFrequency function

2. Now updateBaseRate cannot be called since the interval set in step 1 is too large to be reached

3. Admin becomes sole decider of changing base rate whenever Admin wishes using _setBaseRatePerYear function

Recommended Mitigation Steps

Add a max cap on updated frequency value