Kuiper contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

The function handleFees() will revert if feePct >BASE ==> feePct = timeDiff * licenseFee / ONE_YEAR; ==> BASE < timeDiff * licenseFee / ONE_YEAR; ==> licenseFee > BASE * ONE_YEAR / timeDiff So licenseFee should have a maximum value to prevent this. Probably there is also a reasonable upperlimit to licenseFee the let the protocol function properly.

Proof of Concept

https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L110 function handleFees() private { .. uint256 timeDiff = (block.timestamp - lastFee); uint256 feePct = timeDiff * licenseFee / ONE_YEAR; uint256 fee = startSupply * feePct / (BASE - feePct); // will revert if feePct > BASE

Tools Used

Recommended Mitigation Steps

Define an upperlimit for licenseFee Enforce this in the function proposeBasketLicense() of Factory.sol

Handle

gpersoon

Vulnerability details

Impact

When using the Openzeppelin upgradability pattern, the initialize() function should you the modifier initializer. However the initialize() function of Basket.sol doesn't have this modifier.

This won't give problems in practice because __ERC20_init() does have this modifier and prevents initialize() from being called twice. However forks of the projects or future developers might not be aware of this any make risky changes.

Proof of Concept

https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L36 import { ERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";

contract Basket is IBasket, ERC20Upgradeable { .. function initialize(IFactory.Proposal memory proposal, IAuction auction_) public override { ...

    __ERC20_init(proposal.tokenName, proposal.tokenSymbol);
}

Tools Used

Recommended Mitigation Steps

Add the modifier initializer to the function initialize()

Handle

gpersoon

Vulnerability details

Impact

Anyone can provide bounties with addBounty(). However anyone can add malicious tokens. If someone gets the bounty via settleAuction() they will probably try to sell them on an exchange. The malicious token might trick users in signing a transaction which drains their wallet or point them to a malicious website via an error message.

Proof of Concept

https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L126 function addBounty(IERC20 token, uint256 amount) public override returns (uint256) { // add bounty to basket token.safeTransferFrom(msg.sender, address(this), amount); // could be any token, including a malicious token _bounties.push(Bounty({ token: address(token), amount: amount, active: true }));

    uint256 id = _bounties.length - 1;
    emit BountyAdded(token, amount, id);
    return id;
}

Tools Used

Recommended Mitigation Steps

Create a whitelist for bounty tokens.

Handle

gpersoon

Vulnerability details

Impact

The function settleAuction() doesn't reset auctionBonder to 0. Although this does not give any problems for the logic, the value is no longer relevant after the auction and can be set to 0. Setting to 0 saves a bit of gas and also prevents mistakes in future code updates.

Proof of Concept

https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L69 function settleAuction( .. ) public override { .. require(msg.sender == auctionBonder); .. auctionOngoing = false; hasBonded = false;

Tools Used

Recommended Mitigation Steps

Set auctionBonder to 0 at the end of the function settleAuction()

Handle

gpersoon

Vulnerability details

Impact

If the functions mintTo and burn of Basket.sol are called twice in the same block then block.timestamp will stay the same and timeDiff ==0. Then it is not necessary to _mint () tokens, as this will be 0 tokens anyway.

So checking for timeDiff ==0 could save a bit of gas.

Proof of Concept

https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L110 function handleFees() private { if (lastFee == 0) { lastFee = block.timestamp; } else { uint256 startSupply = totalSupply();

        uint256 timeDiff = (block.timestamp - lastFee);
        uint256 feePct = timeDiff * licenseFee / ONE_YEAR;
        uint256 fee = startSupply * feePct / (BASE - feePct);

        _mint(publisher, fee * (BASE - factory.ownerSplit()) / BASE);
        _mint(Ownable(address(factory)).owner(), fee * factory.ownerSplit() / BASE);
        lastFee = block.timestamp;
        uint256 newIbRatio = ibRatio * startSupply / totalSupply();
        ibRatio = newIbRatio;

        emit NewIBRatio(ibRatio);
    }
}

Tools Used

Recommended Mitigation Steps

Add an extra if in the following way:

function handleFees() private { if (lastFee == 0) { lastFee = block.timestamp; } else { uint256 startSupply = totalSupply(); uint256 timeDiff = (block.timestamp - lastFee); if (timeDiff !=0) { // ===> extra if uint256 feePct = timeDiff * licenseFee / ONE_YEAR; uint256 fee = startSupply * feePct / (BASE - feePct); _mint(publisher, fee * (BASE - factory.ownerSplit()) / BASE); _mint(Ownable(address(factory)).owner(), fee * factory.ownerSplit() / BASE); lastFee = block.timestamp; uint256 newIbRatio = ibRatio * startSupply / totalSupply(); ibRatio = newIbRatio; emit NewIBRatio(ibRatio); } } }