[Under Audit] Add Bucket-Lender #357
Conversation
BrendanChou
force-pushed
the
bc_bucket2
branch
8 times, most recently
from
8d3b67c to
bf5322d
Compare
Pull Request Test Coverage Report for Build 6110
💛 - Coveralls |
BrendanChou
force-pushed
the
bc_bucket2
branch
7 times, most recently
from
9779dd6 to
6792c1c
Compare
| // Available token to lend | ||
| PerBucket public available; | ||
|
|
||
| function available2(uint256 bucket) external view returns(uint256) { |
There was a problem hiding this comment.
space after returns returns(uint256) -> returns (uint256)
There was a problem hiding this comment.
Wait actually what are these functions even lol?
|
|
||
| // ============ Structs ============ | ||
|
|
||
| struct PerBucket { |
There was a problem hiding this comment.
It's very unclear what these are from the naming
There was a problem hiding this comment.
both the struct and variable names
| } | ||
|
|
||
| // Current allocated principal for each bucket | ||
| PerBucket public principal; |
There was a problem hiding this comment.
principal is a really confusing name for this. I'd expect that to just be a number
| } | ||
|
|
||
| // Bucket accounting for which accounts have deposited into that bucket | ||
| mapping(uint256 => PerAccount) public weight; |
There was a problem hiding this comment.
weight? I'm also pretty confused what this is.
There was a problem hiding this comment.
Maybe bucketWeights? Or just bucketAmounts? Also the comment saying "Bucket accounting for..." makes it sound like this is itself a bucket, which I believe it is not...? This is like how much each account has deposited into each bucket?
| // ============ Constants ============ | ||
|
|
||
| // Address of the token being lent | ||
| address public OWED_TOKEN; |
There was a problem hiding this comment.
All caps is not the style we used for other constants like this. See SharedLoan.sol:
// Address of the position's owedToken. Cached for convenience and lower-cost withdrawals
address public owedToken;
There was a problem hiding this comment.
I'm kind of indifferent on which is better, but it should all be the same, and I'm more inclined to just going with what we had before and not changing it
|
|
||
| // ============ Structs ============ | ||
|
|
||
| struct LoanOffering { |
There was a problem hiding this comment.
This is different than the LoanOffering in MarginCommon which is confusing. It's nice this doesn't use the dumb rates thing, but we should make them both the same
|
|
||
| // find highest bucket with outstanding principal | ||
| uint256 bucket = getBucketNumber(); | ||
| while (principalForBkt[bucket] == 0) { |
There was a problem hiding this comment.
Why don't you just store a pointer to this rather than finding it each time? That seems more gas efficient (assuming bucket levels are crossed infrequently)
| } | ||
|
|
||
| // (available up / principal down) starting at the highest bucket | ||
| uint256 p_total = principalRemoved; |
There was a problem hiding this comment.
we don't this naming convention anywhere else, so it's kinda weird here
| * Updates the state variables at any time. Only does anything after the position has been | ||
| * closed or partially-closed since the last time this function was called. | ||
| * | ||
| * - Increases the available amount in the highest bucket with outstanding principal |
There was a problem hiding this comment.
This isn't exactly right because you can touch multiple buckets
| while (p_total > 0) { | ||
| uint256 p_i = Math.min256(p_total, principalForBkt[bucket]); | ||
| if (p_i == 0) { | ||
| continue; |
There was a problem hiding this comment.
I'm confused, if p_i is 0 won't this infinite loop? did you mean to break instead?
BrendanChou
force-pushed
the
bc_bucket2
branch
9 times, most recently
from
07c2632 to
6c5efee
Compare
| * - Go into a particular bucket, determined by time since the start of the position | ||
| * - If the position has not started: bucket = 0 | ||
| * - If the position has started: bucket = ceiling(time_since_start / BUCKET_TIME) | ||
| * - This is always the highest bucket; that is, all higher buckets have no Weight, AA or OP |
There was a problem hiding this comment.
don't they just straight up not exist?
| * | ||
| * - Token Withdrawals: | ||
| * - Can be from any bucket with available amount | ||
| * - Decrease the bucket's AA (if it has enough, otherwise throw) |
There was a problem hiding this comment.
the stuff in parenthesis seems to be satisfied by your first point
| * is responsible for. That is, each bucket with OP is owed (OP)*E^(RT) owedTokens in repayment. | ||
| */ | ||
| // OP for each bucket | ||
| mapping(uint256 => uint256) public principalForBucket; |
There was a problem hiding this comment.
it's confusing that the name of the variable doesn't match either of the things in the comment (Outstanding Principal / OP)
There was a problem hiding this comment.
Likewise for the available stuff too
|
|
||
| /** | ||
| * The critical bucket is: | ||
| * - Greater-than-or-equal-to The highest bucket with OP |
There was a problem hiding this comment.
Ok I've decided that I find the AA/OP abbreviations to be confusing haha. it's not too much more to just say outstanding principal / available amount, or the variable versions outstandingPrincipal/availableAmount
It's not a huge deal though
| signature | ||
| ); | ||
|
|
||
| // CHECK POSITIONID |
| // CHECK ADDRESSES | ||
| require(loanOffering.owedToken == OWED_TOKEN); | ||
| require(loanOffering.heldToken == HELD_TOKEN); | ||
| require(loanOffering.payer == address(this)); |
There was a problem hiding this comment.
this should be an assert too right?
| * This function initializes this contract and returns this address to indicate to Margin | ||
| * that it is willing to take ownership of the loan. | ||
| * | ||
| * @param from (unused) |
| ); | ||
|
|
||
| // CHECK POSITIONID | ||
| require(positionId == POSITION_ID); |
There was a problem hiding this comment.
use your onlyPosition modifier
| * @param payer Address that loaned the additional tokens | ||
| * @param positionId Unique ID of the position | ||
| * @param principalAdded Amount that was added to the position | ||
| * param lentAmount (unused) |
There was a problem hiding this comment.
not unused (just check all these comments / not sure if you've updated them all yet)
| "BucketLender#marginCallOnBehalfOf: Margin-caller must be trusted" | ||
| ); | ||
| require( | ||
| depositAmount == 0, |
There was a problem hiding this comment.
We should put in the comment of Margin#marginCall that making depositAmount 0 makes it so the position owner can't cancel it by depositing. It took some digging for me to confirm that
|
|
||
| uint256 marginPrincipal = getCurrentPrincipalFromMargin(); | ||
|
|
||
| accountForClose(principalTotal.sub(marginPrincipal)); |
There was a problem hiding this comment.
So principalTotal will always be >= marginPrincipal? I think that's right, because we would know if any increases happened
| * Helper function to make sure allowance is set on the dYdX proxy. Callable by anyone. | ||
| */ | ||
| function setMaximumAllowanceOnProxy() | ||
| public |
There was a problem hiding this comment.
why not do something like how we do ensure allowance in the 0x exchange wrapper?
There was a problem hiding this comment.
We could, but that also requires an external call and a state read every time. If we really are trying to enforce the same loanhash every time, then we shouldn't need to do this more than once (in the constructor) because the loanFills amount in the base protocol would overflow at the exact same time.
I really don't feel like it's a problem to set it to max int once. You would need a token with 18 decimal places and then a supply of like 10^80. I realize that the same tokens can be lent multiple times, but honestly I think you would run out of ether to lend that many tokens of anything.
Consider a gas cost of 1 gwei and that lending costs at least 1000 gas (a huge underestimate). Then a transaction costs 10^-6 eth. Since there is 10^8 eth in existence, then you could make 10^14 loans if you had all the eth in existence. Since 2**256 is around 10^77, then there would need to be at least 10^60 supply of a coin which is unheard of.
There was a problem hiding this comment.
I'd think it'd be more likely with a token with like 64 decimal places or something. The convention of 18 decimal places is just a convention. I know most tokens follow this, but could be a confusing edge case if it starts throwing errors for specific weird tokens
Also, does anything actually enforce the same loan hash each time?
| // loop over buckets in reverse order starting with the critical bucket | ||
| for ( | ||
| uint256 bucket = criticalBucket; | ||
| principalToSub > 0 && bucket <= criticalBucket; |
There was a problem hiding this comment.
How would bucket ever be greater than criticalBucket? Aren't we always just subtracting from bucket?
There was a problem hiding this comment.
See below. This is the underflow protection
There was a problem hiding this comment.
In general, negative for-loops with unsigned ints are very annoying. Safemath could be used, but would need to add an if-statement at the bottom of the for loop
if (bucket == 0) {
break;
}
| for ( | ||
| uint256 bucket = criticalBucket; | ||
| principalToSub > 0 && bucket <= criticalBucket; | ||
| bucket-- |
There was a problem hiding this comment.
we should assert that bucket doesn't underflow, so maybe use safemath. I know it shouldn't because we should have always accounted for more principal than is paid back, but it's a good assert to have
There was a problem hiding this comment.
bucket doesn't underflow because of the bucket <= criticalBucket part of it
| if (principalTemp == 0) { | ||
| continue; | ||
| } | ||
| uint256 availableTemp = MathHelpers.getPartialAmount( |
There was a problem hiding this comment.
So, it's possible we get rounding error on this right? I guess the last time we add available amount it will use the rest of it, so if anything this would slightly skew the payout towards earlier buckets
| event BucketLenderCreated( | ||
| address indexed creator, | ||
| address at, | ||
| bytes32 positionId |
There was a problem hiding this comment.
you should index the positionId this will likely be a thing people search by
| * @param owedToken Address of the token being lent by the BucketLender | ||
| * @param parameters Values corresponding to: | ||
| * | ||
| * [0] = number of seconds per bucket |
There was a problem hiding this comment.
nit: to be consistent with Margin align these with 2 spaces after the *
| // ============ Events ============ | ||
|
|
||
| event Deposit( | ||
| address beneficiary, |
| ); | ||
|
|
||
| event Withdraw( | ||
| address withdrawer, |
| ReentrancyGuard | ||
| { | ||
| using SafeMath for uint256; | ||
| using TokenInteract for address; |
There was a problem hiding this comment.
this is cool, do we want to do this for other contracts too?
| */ | ||
| // Available Amount for each bucket | ||
| mapping(uint256 => uint256) public availableForBucket; | ||
| // Total Available Amount |
There was a problem hiding this comment.
nit: newline before this
| * These tokens are also available to be withdrawn by the accounts that have weight in the | ||
| * bucket. | ||
| */ | ||
| // Available Amount for each bucket |
There was a problem hiding this comment.
Put this in the multiline comment above, it's weird they're separate. i.e.:
/**
* Available Amount for each bucket
*
* Available Amount is the amount of tokens that is available to be lent by each bucket. These tokens are also available to be withdrawn by the accounts that have weight in the bucket.
*/
| } | ||
|
|
||
| // Set maximum allowance on proxy | ||
| OWED_TOKEN.approve( |
There was a problem hiding this comment.
Forget if I've asked this: what happens if we eat through all this allowance? Do we refresh it anywhere?
|
|
||
| // CHECK VALUES32 | ||
| require( | ||
| loanOffering.callTimeLimit == CALL_TIMELIMIT, |
There was a problem hiding this comment.
Change these to maximum value as discussed
| "BucketLender#verifyLoanOffering: loanOffering.callTimelimit is incorrect" | ||
| ); | ||
| require( | ||
| loanOffering.maxDuration == MAX_DURATION, |
There was a problem hiding this comment.
Change to maximum value as discussed
| loanOffering.maxDuration == MAX_DURATION, | ||
| "BucketLender#verifyLoanOffering: loanOffering.maxDuration is incorrect" | ||
| ); | ||
| assert(loanOffering.rates.interestRate == INTEREST_RATE); |
There was a problem hiding this comment.
move this up top with the other asserts
There was a problem hiding this comment.
Also just want to confirm this is set by the base protocol on increase - I believe it is
|
|
||
| assert(initialPrincipal > 0); | ||
| assert(principalTotal == 0); | ||
| assert(from != address(this)); // position must be opened without lending from this position |
There was a problem hiding this comment.
just want to confirm this should be assert not require
There was a problem hiding this comment.
yes due to the first line in verifyLoanOffering()
| loanOffering.maxDuration == MathHelpers.maxUint32(), | ||
| "BucketLender#verifyLoanOffering: loanOffering.maxDuration is incorrect" | ||
| ); | ||
| assert(loanOffering.rates.interestRate == INTEREST_RATE); |
There was a problem hiding this comment.
move these up with the other asserts
| @@ -107,6 +107,20 @@ library MathHelpers { | |||
| return 2 ** 256 - 1; | |||
| } | |||
|
|
|||
| /** | |||
| * Calculates and returns the maximum value for a uint256 in solidity | |||
There was a problem hiding this comment.
Nit -- Comment needs updating to uint32.
| /** | ||
| * Calculates and returns the maximum value for a uint256 in solidity | ||
| * | ||
| * @return The maximum value for uint256 |
There was a problem hiding this comment.
Nit -- Comment needs updating to uint32.
fixes #231