Bip Manual Review Findings

BIP-01M: Ineffectual Usage of SafeMath

Type	Severity	Location
Mathematical Operations		Bip.sol:L21

Description:

The SafeMath library by OpenZeppelin is defined solely for the uint256 data type. If it is utilised via the using SafeMath for uint32 statement, all "safe" operations performed will actually indirectly cast the uint32 data types to uint256. As a result, the operations performed are actually unsafe given that if down-casting to uint32 is performed the amounts can overflow via bit truncation.

Example:

Copy
8import "@openzeppelin/contracts/math/SafeMath.sol";
9import "../../AppStorage.sol";
10import "../../../C.sol";
11import "../../../libraries/Decimal.sol";
12import "../../../libraries/LibDiamond.sol";
13
14/**
15 * @author Publius
16 * @title BIP
17**/
18contract Bip {
19
20    using SafeMath for uint256;
21    using SafeMath for uint32;

Recommendation:

We strongly advise the Beanstalk team to introduce a custom SafeMath32 implementation in the codebase that is utilised for such safe arithmetic purposes as currently the season operations for BIP status assertion are performed unsafely. We should note that this finding is replicated across the codebase in multiple files and we have not included them to avoid cluttering.

Alleviation:

A dedicated LibSafeMath32 implementation has been introduced to the codebase and is now applied throughout it in place of SafeMath for the uint32 data type.

BIP-02M: Indeterminate BIP Status Edge Case

Type	Severity	Location
Logical Fault		Bip.sol:L141, L145

Description:

The linked code segments attempt to classify a BIP as either "active" or "ended", however, the equality (==) edge case is not covered leading to a simultaneous inactive and not-ended status.

Example:

Copy
140function isEnded(uint32 bipId) internal view returns (bool) {
141    return season() > startFor(bipId).add(periodFor(bipId)) || s.g.bips[bipId].executed;
142}
143
144function isActive(uint32 bipId) internal view returns (bool) {
145    return season() < startFor(bipId).add(periodFor(bipId));
146}

Recommendation:

We advise the equality to be covered by either status codes to minimise undefined behaviour.

Alleviation:

The isEnded function has been omitted from the codebase thereby preventing the indeterminate state.

BIP-03M: Inexistent Sanitisation of Diamond Compliant Data

Type	Severity	Location
Standard Conformity		Bip.sol:L96, L97

Description:

The BIP creation code does not properly validate that the Diamond standard initialisation data is compliant, allowing the creation of a BIP that can never be fulfilled.

Example:

Copy
75function createBip(
76    IDiamondCut.FacetCut[] memory _diamondCut,
77    address _init,
78    bytes memory _calldata,
79    uint8 pauseOrUnpause,
80    uint32 period,
81    address account
82)
83    internal
84    returns (uint32)
85{
86    uint32 bipId = s.g.bipIndex;
87    s.g.bipIndex += 1;
88    s.g.bips[bipId].start = season();
89    s.g.bips[bipId].period = period;
90    s.g.bips[bipId].timestamp = uint128(block.timestamp);
91    s.g.bips[bipId].proposer = account;
92
93    s.g.bips[bipId].pauseOrUnpause = pauseOrUnpause;
94    for (uint i = 0; i < _diamondCut.length; i++)
95        s.g.diamondCuts[bipId].diamondCut.push(_diamondCut[i]);
96    s.g.diamondCuts[bipId].initAddress = _init;
97    s.g.diamondCuts[bipId].initData = _calldata;
98    s.g.activeBips.push(bipId);
99    return bipId;
100}

Recommendation:

We advise the code to ensure that if the _init address is zero so is the length of the _calldata as otherwise the nested initializeDiamondCut hook will fail within LibDiamond when the BIP is attempted to be activated.

Alleviation:

The diamond-related data is now correctly validated by the createBip function as per the Diamond standard.

BIP-04M: Mismatching Calculation of White Paper

Type	Severity	Location
Logical Fault		Bip.sol:L186

Description:

The incentiveTime calculation divides the time elapsed by 6 whereas the whitepaper states it should be divided by 5 in the 6.4.5 section Commit.

Example:

Copy
183function incentiveTime(uint32 bipId) internal view returns (uint256) {
184    uint256 time = block.timestamp.sub(s.g.bips[bipId].timestamp);
185    if (time > 1800) time = 1800;
186    return time / 6;
187}

Recommendation:

We advise either the code or whitepaper to be updated accordingly.

Alleviation:

The whitepaper formula at the now-labelled 6.5.5 Commit section has been updated to properly reflect the division by 6 instead of 5.