Alchemix contest - oyc_109's results

flash fee modification

description

The admin can change the flashFee in AlchemicTokenV2 using setFlashFee. There are no checks in place to limit how high this fee can be set, and the change takes affect immediately.

A malicious admin could front run a flashLoan transaction and cause the user to burn more tokens than they intended.

recommend adding a require statement to limit how high this fee could be set and/or a time lag for when the change takes affect.

/2022-05-alchemix/contracts-full/AlchemicTokenV2.sol
92:   function setFlashFee(uint256 newFee) external onlyAdmin {
93:     flashMintFee = newFee;
94:     emit SetFlashMintFee(flashMintFee);
95:   }

separation of privilege

description

in the constructor of AlchemicTokenV2 both the ADMIN and SENTINEL roles are set to be msg.sender

recommend setting different addresses for these two roles

/2022-05-alchemix/contracts-full/AlchemicTokenV2.sol
55:   constructor(string memory _name, string memory _symbol, uint256 _flashFee) ERC20(_name, _symbol) {
56:     _setupRole(ADMIN_ROLE, msg.sender);
57:     _setupRole(SENTINEL_ROLE, msg.sender);
58:     _setRoleAdmin(SENTINEL_ROLE, ADMIN_ROLE);
59:     _setRoleAdmin(ADMIN_ROLE, ADMIN_ROLE);
60:     flashMintFee = _flashFee;
61:   }

named returns and a return statement isn’t necessary

description

Removing unused named returns variables can reduce gas usage (MSTOREs/MLOADs) and improve code clarity. To save gas and improve code quality: consider using only one of those.

findings

/2022-05-alchemix/contracts-full/AlchemistV2.sol
143:     function accounts(address owner)
159:     function positions(address owner, address yieldToken)
205:     function getMintLimitInfo()
221:     function getRepayLimitInfo(address underlyingToken)
238: function getLiquidationLimitInfo(address underlyingToken)

/2022-05-alchemix/contracts-full/TransmuterBuffer.sol
130: function getWeight(address weightToken, address token)

Don't Initialize Variables with Default Value

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

findings

/2022-05-alchemix/contracts-full/AlchemistV2.sol
990: for (uint256 i = 0; i < depositedTokens.values.length; i++) {
1458: uint256 totalValue = 0;
1461: for (uint256 i = 0; i < depositedTokens.values.length; i++) {
1524: for (uint256 i = 0; i < depositedTokens.values.length; i++) {

/2022-05-alchemix/contracts-full/TransmuterBuffer.sol
186: for (uint256 i = 0; i < tokens.length; i++) {
235: for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
242: for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
272: for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
382: for (uint256 j = 0; j < registeredUnderlyings.length; j++) {
387: for (uint256 i = 0; i < numYTokens; i++) {
534: uint256 want = 0;

cache in variables instead of loading

description

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas).

findings

/2022-05-alchemix/contracts-full/AlchemistV2.sol
990: for (uint256 i = 0; i < depositedTokens.values.length; i++) { //@audit cache depositedTokens.values.length
1461: for (uint256 i = 0; i < depositedTokens.values.length; i++) {
1524: for (uint256 i = 0; i < depositedTokens.values.length; i++) {

/2022-05-alchemix/contracts-full/TransmuterBuffer.sol
186: for (uint256 i = 0; i < tokens.length; i++) {
235: for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
242: for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
272: for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
382: for (uint256 j = 0; j < registeredUnderlyings.length; j++) {

using prefix increments save gas

description

Prefix increments are cheaper than postfix increments, eg ++i rather than i++

findings

/2022-05-alchemix/contracts-full/AlchemistV2.sol
990: for (uint256 i = 0; i < depositedTokens.values.length; i++) { 
1461: for (uint256 i = 0; i < depositedTokens.values.length; i++) {
1524: for (uint256 i = 0; i < depositedTokens.values.length; i++) {

/2022-05-alchemix/contracts-full/TransmuterBuffer.sol
186: for (uint256 i = 0; i < tokens.length; i++) {
235: for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
242: for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
272: for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
382: for (uint256 j = 0; j < registeredUnderlyings.length; j++) {
387: for (uint256 i = 0; i < numYTokens; i++) {

do not cache variable used only once

description

for variables only used once, changing it to inline saves gas

findings

/2022-05-alchemix/contracts-full/AlchemistV2.sol
1539: uint256 balance = _accounts[owner].balances[yieldToken];

use calldata instead of memory

description

Use calldata instead of memory for function parameters saves gas In some cases, having function arguments in calldata instead of memory is more optimal.

Consider the following generic example:

    contract C {
        function add(uint[] memory arr) external returns (uint sum) {
            uint length = arr.length;
            for (uint i = 0; i < arr.length; i++) {
                sum += arr[i];
            }
        }
    }

In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:

    contract C {
        function add(uint[] calldata arr) external returns (uint sum) {
            uint length = arr.length;
            for (uint i = 0; i < arr.length; i++) {
                sum += arr[i];
            }
        }
    }

In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.

In short, use calldata instead of memory if the function argument is only read.

Note that in older Solidity versions, changing some function arguments from memory to calldata may cause "unimplemented feature error". This can be avoided by using a newer (0.8.*) Solidity compiler.

findings

/2022-05-alchemix/contracts-full/TransmuterBuffer.sol
180: address[] memory tokens,
181: uint256[] memory weights