Sushi Miso contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

function createEscrow first assigns an index for the new isChildEscrow and only then pushes the struct to the array. When first escrow is being created, the array contains 0 elements so escrows.length-1 will underflow and return a max uint value: isChildEscrow[address(newEscrow)] = Fermenter(true,_templateId,escrows.length-1); escrows.push(newEscrow);

Recommended Mitigation Steps

isChildEscrow[address(newEscrow)] = Fermenter(true,_templateId,escrows.length); escrows.push(newEscrow);

Handle

pauliax

Vulnerability details

Impact

function lockTokens in contract TokenVault should check that _withdrawer is not empty (0x0) to prevent accidentally locked forever (burned) tokens.

Recommended Mitigation Steps

require(_withdrawer != address(0));

Handle

pauliax

Vulnerability details

Impact

No need for this line in function initMISOMarket as it gets this value by default: auctionTemplateId = 0;

Recommended Mitigation Steps

Consider removing useless initialization.

Handle

pauliax

Vulnerability details

Impact

It is unclear why MISOTokenFactory contract needs its own TOKEN_MINTER_ROLE when similar role is already defined in MISOAccessControls: bytes32 public constant TOKEN_MINTER_ROLE = keccak256("TOKEN_MINTER_ROLE"); // MISOTokenFactory bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); // MISOAccessControls Especially considering that both of these roles have the same privileges, that is to deployToken when the contract is locked:

    /// @dev If the contract is locked, only admin and minters can deploy. 
    if (locked) {
        require(accessControls.hasAdminRole(msg.sender) 
                || accessControls.hasMinterRole(msg.sender)
                || hasTokenMinterRole(msg.sender),
            "MISOTokenFactory: Sender must be minter if locked"
        );
    }

Similarly with MARKET_MINTER_ROLE in MISOMarket and other contracts.

Recommended Mitigation Steps

Consider removing this surplus role to eliminate the confusion and improve gas usage.

Handle

pauliax

Vulnerability details

Impact

WETH state variable in MISOLauncher is practically useless as it is not used in any meaningful way. Similarly, SECONDS_PER_DAY is not used in PostAuctionLauncher.

Recommended Mitigation Steps

Consider removing unused variables.

Handle

pauliax

Vulnerability details

Impact

The purpose of allDepositIds in TokenVault is unclear, it gets pushed every _id and these values are never deleted so it basically contains all the numbers starting from 1 to depositId. I do not understand why you cannot just rely on depositId and create this array in the frontend if needed. This will improve gas usage.

Recommended Mitigation Steps

Consider getting rid of allDepositIds.

Handle

pauliax

Vulnerability details

Impact

There are structs that could be optimized for storage slots (packed tightly) to reduce gas usage. For instance, struct Item in TokenVault, we know that a max value for unlockTime is 10000000000 so this does not need uint256 space, userIndex also won't exceed billions in practice. I see other structs are more optimized, so consider doing the same with this one.

Recommended Mitigation Steps

Pack 'em even tighter.

Handle

pauliax

Vulnerability details

Impact

No need to check that _startTime < 10000000000 as it is later checked against _endTime which is also < 10000000000 : require(_startTime < 10000000000, "Crowdsale: enter an unix timestamp in seconds, not miliseconds"); require(_endTime < 10000000000, "Crowdsale: enter an unix timestamp in seconds, not miliseconds"); ... require(_endTime > _startTime, "Crowdsale: end time must be older than start price");

Recommended Mitigation Steps

Remove this line: require(_startTime < 10000000000, "Crowdsale: enter an unix timestamp in seconds, not miliseconds");

Handle

pauliax

Vulnerability details

Impact

contract MISORecipe01 incorrectly declares interfaces for IMISOTokenFactory function createToken and IMISOFarmFactory function createFarm. The actual implementations have different parameters so these functions will not be found at runtime.

Recommended Mitigation Steps

To make it consistent across different contracts, you should extract interfaces separately and enforce implementations to implement them as "implements" is checked during compile time and could prevent such mistakes.