Backd contest - TerrierLover's results

AddressProvider.sol

Finding1: initialize() function can be called by anybody

initialize() function can be called anybody when the contract is not initialized.

Here is a definition of initialize() function.

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/AddressProvider.sol#L53-L57

function initialize(address roleManager) external initializer {
    AddressProviderMeta.Meta memory meta = AddressProviderMeta.Meta(true, true);
    _addressKeyMetas.set(AddressProviderKeys._ROLE_MANAGER_KEY, meta.toUInt());
    _setConfig(AddressProviderKeys._ROLE_MANAGER_KEY, roleManager);
}

The function does not check who the caller is, and it has a potential of frontrunning attack. Here are similar reports in other code4rena conpetitions. https://github.com/code-423n4/2021-08-notional-findings/issues/7 https://github.com/code-423n4/2021-08-notional-findings/issues/98

It should limit who can call initialize() function at least by using role functionalities that AddressProvider.sol currently uses.

Finding2: Add != address(0) check at constructor and various functions

constructor and following functions (addAction, initialize, addFeeHandler, initializeAddress, initializeAndFreezeAddress, prepareAddress) take address in their arguments, but these address can be 0.

---

BkdLocker.sol

Finding1: Usage of != 0 instead of > 0 can reduce gas cost

Following uint256 variables have > 0 check. Using != 0 reduces gas cost, so it is worth considering doing so.

depositFees function https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L90 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L91

executeUnlocks function https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L136 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L138

claimableFees function https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L253

_userCheckpoint function https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L300 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L308

Finding2: No need to set 0 for uint256 variables

The default value of uint256 variable is 0. Following codebase sets 0, but they are not needed.

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L133 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L310

The above codebase can be rewritten as follows:

uint256 totalAvailableToWithdraw;

for (uint256 i; i < length; i++) {

Finding3: Add unchecked for the decrement of i in while loop

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L137-L147

uint256 i = length;
while (i > 0) {
    i = i - 1;
    if (stashedWithdraws[i].releaseTime <= block.timestamp) {
        totalAvailableToWithdraw += stashedWithdraws[i].amount;

        stashedWithdraws[i] = stashedWithdraws[stashedWithdraws.length - 1];

        stashedWithdraws.pop();
    }
}

It is obvious that i will not be underflown since while has i > 0 check. So the above codebase can be rewritten like this:

uint256 i = length;
while (i > 0) {
    unchecked {
      i = i - 1;
    }
    if (stashedWithdraws[i].releaseTime <= block.timestamp) {
        totalAvailableToWithdraw += stashedWithdraws[i].amount;

        stashedWithdraws[i] = stashedWithdraws[stashedWithdraws.length - 1];

        stashedWithdraws.pop();
    }
}

Finding4: No need to define uint256 i at executeUnlocks function

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L135-L147

uint256 length = stashedWithdraws.length;
require(length > 0, "No entries");
uint256 i = length;
while (i > 0) {
    i = i - 1;
    if (stashedWithdraws[i].releaseTime <= block.timestamp) {
        totalAvailableToWithdraw += stashedWithdraws[i].amount;

        stashedWithdraws[i] = stashedWithdraws[stashedWithdraws.length - 1];

        stashedWithdraws.pop();
    }
}

uint256 i is defined, but this is not needed. This part can be rewritten like this (In this case, I recommend renaming length to something more appropriate).

uint256 length = stashedWithdraws.length;
require(length > 0, "No entries");
while (length > 0) {
    length = length - 1;
    if (stashedWithdraws[length].releaseTime <= block.timestamp) {
        totalAvailableToWithdraw += stashedWithdraws[length].amount;

        stashedWithdraws[length] = stashedWithdraws[stashedWithdraws.length - 1];

        stashedWithdraws.pop();
    }
}

Finding5: Duplicate userBalance > 0 check at _userCheckpoint function

At following codebase, it has duplicate userBalance > 0 check.

uint256 userBalance = balances[user];
if (userBalance > 0) {
    curRewardTokenData.userShares[user] += (curRewardTokenData.feeIntegral -
        curRewardTokenData.userFeeIntegrals[user]).scaledMul(
            userBalance.scaledMul(boostFactors[user])
        );
}

// Update values for previous rewardTokens
if (lastUpdated[user] < lastMigrationEvent && userBalance > 0) {

It can be rewritten to reduce gas cost by simply combiming the if check like this.

uint256 userBalance = balances[user];
if (userBalance > 0) {
    curRewardTokenData.userShares[user] += (curRewardTokenData.feeIntegral -
        curRewardTokenData.userFeeIntegrals[user]).scaledMul(
            userBalance.scaledMul(boostFactors[user])
        );

    // Update values for previous rewardTokens
    if (lastUpdated[user] < lastMigrationEvent) { // <--- Remove userBalance > 0 put this if statement with the other one
      ...
    }
}

Finding 6: Add unchecked at prepareUnlock function

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L118-L122

require(
    totalStashed[msg.sender] + amount <= balances[msg.sender],
    "Amount exceeds locked balance"
);
totalStashed[msg.sender] += amount;

It is obvious that totalStashed[msg.sender] + amount at line 122 will not be overflown. So it can be rewritten like this to reduce gas cost.

require(
    totalStashed[msg.sender] + amount <= balances[msg.sender],
    "Amount exceeds locked balance"
);
unchecked {
    totalStashed[msg.sender] += amount;
}

---

Controller.sol

Finding1: Usage of != 0 instead of > 0 can reduce gas cost

Following uint256 variables have > 0 check. Using != 0 reduces gas cost, so it is worth considering doing so.

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/Controller.sol#L114

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/Controller.sol#L117

---