Malt Finance contest - Meta0xNull's results

Handle

Meta0xNull

Vulnerability details

Impact

Is Not Uncommon Normal Users Accidentally Send Tokens into Contract.

ENS Airdrop is a Good Example Normal Users Accidentally Send Tokens into Contract: https://discuss.ens.domains/t/social-amend-airdrop-proposal-to-include-accidentally-returned-funds/6975

In UniswapHandler.sol, sellMalt(), addLiquidity() and removeLiquidity() Have No Access Control. When Normal Users Accidently Deposit Tokens into the Contract, Any Random Persons/Bot Can Withdraw the Tokens because it will safeTransfer to msg.sender who find out there is token balance in the contract.

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/DexHandlers/UniswapHandler.sol#L185-L219 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/DexHandlers/UniswapHandler.sol#L221-L245

Tools Used

Manual Review

Recommended Mitigation Steps

Add relevant access control, probably Only StabilizerNode and Admin have Access to this contract various functions like sellMalt(), addLiquidity() and removeLiquidity() etc.

Handle

Meta0xNull

Vulnerability details

Impact

A wrong user input or wallets defaulting to the zero addresses for a missing input can lead to the contract needing to redeploy or Users'FUND Locked inside the Contract.

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Permissions.sol#L63-L77

Tools Used

Manual Review

Recommended Mitigation Steps

requires Addresses is not zero.

require(_admin != address(0), "Address Can't Be Zero")

Handle

Meta0xNull

Vulnerability details

Impact

<code> function initialize( </code> <code> address _timelock,</code> <code> address initialAdmin</code> <code> ) external initializer {</code> <code> _adminSetup(_timelock);</code> <code> _setupRole(ADMIN_ROLE, initialAdmin);</code>

The are multiple contracts using Initializable requires to call the initialize() function with suitable parameters.

However, there is no access control in place to avoid an attacker to front-run this call. As a result of that, an attacker could temporarily block the initialization front-running each deployment, using invalid parameters in the initialize call.

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/TransferService.sol#L21-L27 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Malt.sol#L22-L40 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/PoolTransferVerification.sol#L28-L42 More...

Tools Used

Manual Review

Recommended Mitigation Steps

1. Use Constructor Rather Than initialize()

2. If initialize() Is Unavoidable, then Hardcode an ETH Address in initialize(): require( msg.sender == 0xYOURETHADDRESS );

Handle

Meta0xNull

Vulnerability details

Impact

 * [WARNING]
 * ====
 * This function should only be called from the constructor when setting
 * up the initial roles for the system.
 *
 * Using this function in any other way is effectively circumventing the admin
 * system imposed by {AccessControl}.
 * ====
 *
 * NOTE: This function is deprecated in favor of {_grantRole}.

There are multiple contracts that import Permissions.sol and using Deprecated Function _setupRole() with Security Problem that Applicable to all these contracts because all of the contracts use initialize() Rather Than Constructor.

Proof of Concept

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L174-L186 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Permissions.sol#L53 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Permissions.sol#L117 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Permissions.sol#L121

Tools Used

Manual Review

Recommended Mitigation Steps

Replace _setupRole() with _grantRole()

Handle

Meta0xNull

Vulnerability details

Impact

The current ownership transfer process involves the current TIMELOCK_ROLE calling reassignGlobalAdmin(). If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the TIMELOCK_ROLE modifier.

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Permissions.sol#L63-L77

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing a two step process where the TIMELOCK_ROLE nominates an account and the nominated account needs to call an accept_TIMELOCK_ROLE() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

Handle

Meta0xNull

Vulnerability details

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/RewardSystem/RewardDistributor.sol#L275 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/DAO.sol#L56 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L223 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L230 More...

Tools Used

Manual Review

Recommended Mitigation Steps

Shorten the revert strings to fit in 32 bytes.

Or consider using Custom Errors (solc >=0.8.4).

Handle

Meta0xNull

Vulnerability details

Impact

The local variable used as for loop index need not be initialized to 0 because the default value is 0. Avoiding this anti-pattern can save a few opcodes and therefore a tiny bit of gas.

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/TransferService.sol#L87 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/AuctionBurnReserveSkew.sol#L54 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Malt.sol#L34-L37 More...

Tools Used

Manual Review

Recommended Mitigation Steps

Remove explicit 0 initialization of for loop index variable.

Before: for (uint i = 0; After for (uint i;

Handle

Meta0xNull

Vulnerability details

Impact

removeVerifier() loops follows this for-each pattern: for (uint256 i = 0; i < array.length; i++) { // do something with array[i] }

In such for loops, the array.length is read on every iteration, instead of caching it once in a local variable and read it again using the local variable.

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/TransferService.sol#L87-L88

Tools Used

Manual Review

Recommended Mitigation Steps

Read these values from memory once, cache them in local variables and then read them again using the local variables. For example:

Before: for (uint i = 0; i < verifierList.length - 1; i = i + 1) { if (verifierList[i] == _address) {

After: uint256 verifierList_temp = verifierList

for (uint i = 0; i < verifierList_temp.length - 1; i = i + 1) { if (verifierList_temp[i] == _address) {