Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lock.sol::totalLocked public variable is not updated in the Lock.sol::extendLock() function. #264

Closed
code423n4 opened this issue Dec 15, 2022 · 8 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-23 partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") upgraded by judge Original issue severity upgraded from QA/Gas by judge

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L19
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L84

Vulnerability details

Impact

The extendLock() function helps to change the BondNFT lock parameters (amount, period). The problem is totalLocked is not increased.

The totalLocked is a public variable, so there may be wrong information for those who consult that specific information:

  • Users
  • Other protocols
  • Bots

Proof of Concept

I created this test in test/09.Bond.js where you can see the extendLock() does not increase the totalLocked variable::

// npx hardhat test --grep "Adding amount and time to lock the totalLocked has not changed" test/09.Bonds.js
it("Adding amount and time to lock the totalLocked has not changed", async function () {
    await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("100"));
    await lock.connect(user).lock(StableToken.address, ethers.utils.parseEther("100"), 10);

    await network.provider.send("evm_increaseTime", [432000]); // Skip 5 days
    await network.provider.send("evm_mine");

    // Get the Bond 1
    [id, _owner, asset, amount, mintEpoch, mintTime, expireEpoch, pending, shares, period, expired] = await bond.idToBond(1);
    expect(amount).to.be.equals(ethers.utils.parseEther("100"));
    expect(period).to.be.equals(10);

    // Total locked is the amount == 100
    expect(await lock.totalLocked(asset)).to.be.equals(ethers.utils.parseEther("100"));

    // ExtendLock for 100 more and 10 more period.
    await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("100"));
    await lock.connect(user).extendLock(1, ethers.utils.parseEther("100"), 10);  // add 100, 10

    //Total Locked is now the amount before + the new amount == 200
    expect(await lock.totalLocked(asset)).to.be.equals(ethers.utils.parseEther("200"));

    [id, _owner, asset, amount, mintEpoch, mintTime, expireEpoch, pending, shares, period, expired] = await bond.idToBond(1);
    expect(amount).to.be.equals(ethers.utils.parseEther("200"));  // new amount
    expect(period).to.be.equals(20); // New period
    //Total Locked should be amount before + the new amount == 200
    expect(await lock.totalLocked(asset)).to.be.equals(ethers.utils.parseEther("200"));
});

Tools used

VsCode/Hardhat

Recommended Mitigation Steps

Add the totalLocked incremention in the Lock.sol::extendLock() function

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 15, 2022
code423n4 added a commit that referenced this issue Dec 15, 2022
@GalloDaSballo
Copy link

Making primary because of POC

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Dec 22, 2022
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as primary issue

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jan 3, 2023
@c4-sponsor
Copy link

TriHaz marked the issue as sponsor confirmed

@GalloDaSballo
Copy link

Effectively the "low impact" version of #23

@GalloDaSballo
Copy link

Awarding 25% for that reason

@c4-judge c4-judge added the partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) label Jan 16, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as partial-25

@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as duplicate of #23

@c4-judge c4-judge added duplicate-23 and removed primary issue Highest quality submission among a set of duplicates labels Jan 16, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo changed the severity to 3 (High Risk)

@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-23 partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Development

No branches or pull requests

4 participants