Tigris Trade contest - 0xbepresent's results

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

Lines of code

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

Vulnerability details

Impact

The GovNFT.sol::maxBridge variable is not used in the "bridging process". The lzReceive() function can run out of gas if there are many Tokens passing to another chain.

The GovNFTs can get caught in a chain because they can be burned in chain A but in Chain B the lzReceive can not process the mint.

Proof of Concept

GovNFT.sol::maxBridge limit is not used in crossChain()

Tools used

VsCode

Recommended Mitigation Steps

Add a limit of GovNFT before sending the NFTs to another chain.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L952 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L163 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L255 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L480 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L689

Vulnerability details

Impact

The Trading.initiateMarketOrder(), Trading.addPosition() and Trading.executeLimitOrder() pay fees to the DAO, Referrer and Bot with help of the _handleOpenFees() function. The setFees() help to set the fees by the owner.

The initiateMarketOrder(), addPosition() and executeLimitOrder() transactions can be frontrun with a fee increase. Furthermore, the user can create a limitOrder with X fee in mind and when the LimitOrder is executed by executeLimitOrder() function, the fee could have changed in that period of time

This Admin Privilege issue can be used to raise up the fees and the protocol users could get stronger security guarantees by having the fee increases behind governance timelock.

Proof of Concept

The setFees function can be called by the owner at any time.

function setFees(bool _open, uint _daoFees, uint _burnFees, uint _referralFees, uint _botFees, uint _percent) external onlyOwner {
    unchecked {
        require(_daoFees >= _botFees+_referralFees*2);
        if (_open) {
            openFees.daoFees = _daoFees;
            openFees.burnFees = _burnFees;
            openFees.referralFees = _referralFees;
            openFees.botFees = _botFees;
        } else {
            closeFees.daoFees = _daoFees;
            closeFees.burnFees = _burnFees;
            closeFees.referralFees = _referralFees;
            closeFees.botFees = _botFees;                
        }
        require(_percent <= DIVISION_CONSTANT);
        vaultFundingPercent = _percent;
    }
}

Tools used

VsCode

Recommended Mitigation Steps

The setFees() should be behind a TimeLock process and add a variable "MAX_FEE" to ensure fees can not go up above the threshold.